Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1191782 > unrolled thread

Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded context

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded  context Lars-Peter Clausen <lars@metafoo.de> - 2015-07-24 14:40 +0200
    Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded  context Xander Huff <xander.huff@ni.com> - 2015-08-03 22:20 +0200
      Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded  context Lars-Peter Clausen <lars@metafoo.de> - 2015-08-04 10:10 +0200
    Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded context Shubhrajyoti Datta <shubhrajyoti.datta@gmail.com> - 2015-08-04 07:40 +0200
      Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded  context Lars-Peter Clausen <lars@metafoo.de> - 2015-08-04 10:10 +0200

#1191782 — Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded context

Hi,

Sorry, but I don't think this patch has been sufficiently tested against a 
mainline kernel. The driver wont even probe the way it is right now.

On 07/21/2015 01:14 AM, Xander Huff wrote:
> The driver currently registers a pair of irq handlers using
> request_threaded_irq(), however the synchronization mechanism between the
> hardirq and the threadedirq handler is a regular spinlock.

If everything runs in threaded context we don't really need the spinlock 
anymore and can use the mutex throughout.

>
> Unfortunately, this breaks PREEMPT_RT builds, where a spinlock can sleep,
> and is thus not able to be acquired from a hardirq handler. This patch gets
> rid of the hardirq handler and pushes all interrupt handling into the
> threaded context.

We actually might as well run everything in the hardirq handler (which will 
be threaded in PREEMPT_RT). The reason why we have the threaded handler is 
because xadc_handle_event() used to sleep, but it doesn't do this anymore.

> Signed-off-by: Xander Huff <xander.huff@ni.com>
> ---
>   drivers/iio/adc/xilinx-xadc-core.c | 37 ++++++++-----------------------------
>   1 file changed, 8 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index ce93bd8..e16afdb 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -267,40 +267,15 @@ static void xadc_zynq_unmask_worker(struct work_struct *work)
>   	xadc_zynq_update_intmsk(xadc, 0, 0);
>
>   	spin_unlock_irq(&xadc->lock);
> -
> -	/* if still pending some alarm re-trigger the timer */
> -	if (xadc->zynq_masked_alarm) {
> -		schedule_delayed_work(&xadc->zynq_unmask_work,
> -				msecs_to_jiffies(XADC_ZYNQ_UNMASK_TIMEOUT));
> -	}
>   }
>
>   static irqreturn_t xadc_zynq_threaded_interrupt_handler(int irq, void *devid)
>   {
>   	struct iio_dev *indio_dev = devid;
>   	struct xadc *xadc = iio_priv(indio_dev);
> -	unsigned int alarm;
> -
> -	spin_lock_irq(&xadc->lock);
> -	alarm = xadc->zynq_alarm;
> -	xadc->zynq_alarm = 0;
> -	spin_unlock_irq(&xadc->lock);
> -
> -	xadc_handle_events(indio_dev, xadc_zynq_transform_alarm(alarm));
> -
> -	/* unmask the required interrupts in timer. */
> -	schedule_delayed_work(&xadc->zynq_unmask_work,
> -			msecs_to_jiffies(XADC_ZYNQ_UNMASK_TIMEOUT));
> -

With nobody scheduling the unmask worker interrupts will stay disabled 
indefinitely after they fired once, that's not very useful.

> -	return IRQ_HANDLED;
> -}
> -
> -static irqreturn_t xadc_zynq_interrupt_handler(int irq, void *devid)
> -{
> -	struct iio_dev *indio_dev = devid;
> -	struct xadc *xadc = iio_priv(indio_dev);
>   	irqreturn_t ret = IRQ_HANDLED;
>   	uint32_t status;
> +	unsigned int alarm;
>
>   	xadc_read_reg(xadc, XADC_ZYNQ_REG_INTSTS, &status);
>
> @@ -312,7 +287,6 @@ static irqreturn_t xadc_zynq_interrupt_handler(int irq, void *devid)
>   	spin_lock(&xadc->lock);
>
>   	xadc_write_reg(xadc, XADC_ZYNQ_REG_INTSTS, status);
> -
>   	if (status & XADC_ZYNQ_INT_DFIFO_GTH) {
>   		xadc_zynq_update_intmsk(xadc, XADC_ZYNQ_INT_DFIFO_GTH,
>   			XADC_ZYNQ_INT_DFIFO_GTH);
> @@ -330,8 +304,14 @@ static irqreturn_t xadc_zynq_interrupt_handler(int irq, void *devid)
>   		xadc_zynq_update_intmsk(xadc, 0, 0);
>   		ret = IRQ_WAKE_THREAD;
>   	}
> +
> +	alarm = xadc->zynq_alarm;
> +	xadc->zynq_alarm = 0;

With all running in the same handler we don't need those anymore.

> +
>   	spin_unlock(&xadc->lock);
>
> +	xadc_handle_events(indio_dev, xadc_zynq_transform_alarm(alarm));
> +
>   	return ret;
>   }
>
> @@ -436,7 +416,6 @@ static const struct xadc_ops xadc_zynq_ops = {
>   	.write = xadc_zynq_write_adc_reg,
>   	.setup = xadc_zynq_setup,
>   	.get_dclk_rate = xadc_zynq_get_dclk_rate,
> -	.interrupt_handler = xadc_zynq_interrupt_handler,

The corresponding field should be removed from the xadc_ops struct and then 
you'll also notice that interrupts now don't work anymore for the AXI 
interface version.

>   	.threaded_interrupt_handler = xadc_zynq_threaded_interrupt_handler,
>   	.update_alarm = xadc_zynq_update_alarm,
>   };
> @@ -1225,7 +1204,7 @@ static int xadc_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto err_free_samplerate_trigger;
>
> -	ret = request_threaded_irq(irq, xadc->ops->interrupt_handler,
> +	ret = request_threaded_irq(irq, NULL,
>   				xadc->ops->threaded_interrupt_handler,
>   				0, dev_name(&pdev->dev), indio_dev);
>   	if (ret)
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1199241

On 7/24/2015 7:38 AM, Lars-Peter Clausen wrote:
> Hi,
>
> Sorry, but I don't think this patch has been sufficiently tested against a
> mainline kernel. The driver wont even probe the way it is right now.
>
Thanks for your responses. I'm not sure why it doesn't probe for you since I was 
able to do a fair amount of tests with it on our device.

I'll get to work on your suggestions and hopefully having a v4 out sometime next 
week.

-- 
Xander Huff
Staff Software Engineer
National Instruments
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199498

On 08/03/2015 10:18 PM, Xander Huff wrote:
> On 7/24/2015 7:38 AM, Lars-Peter Clausen wrote:
>> Hi,
>>
>> Sorry, but I don't think this patch has been sufficiently tested against a
>> mainline kernel. The driver wont even probe the way it is right now.
>>
> Thanks for your responses. I'm not sure why it doesn't probe for you since I
> was able to do a fair amount of tests with it on our device.

genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 157
xadc: probe of f8007100.xadc failed with error -22

You can' request a threaded IRQ without having IRQF_ONESHOT set, since this
will effectively result in a IRQ storm.

> 
> I'll get to work on your suggestions and hopefully having a v4 out sometime
> next week.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199418 — Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded context

On Fri, Jul 24, 2015 at 6:08 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> Hi,
>
> Sorry, but I don't think this patch has been sufficiently tested against a
> mainline kernel. The driver wont even probe the way it is right now.
>
> On 07/21/2015 01:14 AM, Xander Huff wrote:
>>
>> The driver currently registers a pair of irq handlers using
>> request_threaded_irq(), however the synchronization mechanism between the
>> hardirq and the threadedirq handler is a regular spinlock.
>
>
> If everything runs in threaded context we don't really need the spinlock
> anymore and can use the mutex throughout.

that should be better from the performance point of view.

>
>>
>> Unfortunately, this breaks PREEMPT_RT builds, where a spinlock can sleep,
>> and is thus not able to be acquired from a hardirq handler. This patch
>> gets
>> rid of the hardirq handler and pushes all interrupt handling into the
>> threaded context.
>
>
> We actually might as well run everything in the hardirq handler (which will
> be threaded in PREEMPT_RT). The reason why we have the threaded handler is
> because xadc_handle_event() used to sleep, but it doesn't do this anymore.

The point is why have the hard irq. If we use hardirq then not mutex
can be used and spinlock will
be busy.

is there something i may be missing?
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199500

On 08/04/2015 07:34 AM, Shubhrajyoti Datta wrote:
> On Fri, Jul 24, 2015 at 6:08 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> Hi,
>>
>> Sorry, but I don't think this patch has been sufficiently tested against a
>> mainline kernel. The driver wont even probe the way it is right now.
>>
>> On 07/21/2015 01:14 AM, Xander Huff wrote:
>>>
>>> The driver currently registers a pair of irq handlers using
>>> request_threaded_irq(), however the synchronization mechanism between the
>>> hardirq and the threadedirq handler is a regular spinlock.
>>
>>
>> If everything runs in threaded context we don't really need the spinlock
>> anymore and can use the mutex throughout.
> 
> that should be better from the performance point of view.
> 
>>
>>>
>>> Unfortunately, this breaks PREEMPT_RT builds, where a spinlock can sleep,
>>> and is thus not able to be acquired from a hardirq handler. This patch
>>> gets
>>> rid of the hardirq handler and pushes all interrupt handling into the
>>> threaded context.
>>
>>
>> We actually might as well run everything in the hardirq handler (which will
>> be threaded in PREEMPT_RT). The reason why we have the threaded handler is
>> because xadc_handle_event() used to sleep, but it doesn't do this anymore.
> 
> The point is why have the hard irq. If we use hardirq then not mutex
> can be used and spinlock will
> be busy.

Well there is no need to use a threaded IRQ. The interrupt handler is quite
small and doesn't take too much time and doesn't have any delays or sleeps
in it either.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web