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

Groups > linux.kernel > #1559982 > unrolled thread

[PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

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

  [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-16 19:50 +0100
    Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered  interrupts Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-16 20:20 +0100
      Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-16 20:50 +0100
        Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-17 20:20 +0100
    Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Robert Jarzmik <robert.jarzmik@free.fr> - 2017-01-17 09:00 +0100
      Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-17 09:20 +0100
        Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jarkko Nikula <jarkko.nikula@linux.intel.com> - 2017-01-17 14:20 +0100
          Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-17 16:00 +0100
    Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Robert Jarzmik <robert.jarzmik@free.fr> - 2017-01-17 09:00 +0100
      Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-17 09:10 +0100
        Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Robert Jarzmik <robert.jarzmik@free.fr> - 2017-01-18 09:30 +0100
          Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-18 10:40 +0100
            Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Mark Brown <broonie@kernel.org> - 2017-01-18 13:50 +0100
              Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-19 17:10 +0100
                [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-19 20:40 +0100
                  Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered  interrupts Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-19 21:10 +0100
                    Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Mark Brown <broonie@kernel.org> - 2017-01-20 13:30 +0100
                      Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-20 16:40 +0100
                        Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Mark Brown <broonie@kernel.org> - 2017-01-20 18:40 +0100
                  Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Mark Brown <broonie@kernel.org> - 2017-01-19 21:30 +0100
                  Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Robert Jarzmik <robert.jarzmik@free.fr> - 2017-01-20 09:00 +0100
                Applied "spi: pxa2xx: Prepare for edge-triggered interrupts" to the spi tree Mark Brown <broonie@kernel.org> - 2017-01-24 19:50 +0100

Page 1 of 2  [1] 2  Next page →

#1559982 — [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

When using the a device with edge-triggered interrupts, such as MSIs,
the interrupt handler has to ensure that there is a point in time during
its execution where all interrupts sources are silent so that a new
event can trigger a new interrupt again.

This is achieved here by looping over SSSR evaluation. We need to take
into account that SSCR1 may be changed by the transfer handler, thus we
need to redo the mask calculation, at least regarding the volatile
interrupt enable bit (TIE).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/spi/spi-pxa2xx.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 0d10090..ac49b80 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -751,6 +751,7 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 	struct driver_data *drv_data = dev_id;
 	u32 sccr1_reg;
 	u32 mask = drv_data->mask_sr;
+	irqreturn_t ret = IRQ_NONE;
 	u32 status;
 
 	/*
@@ -774,24 +775,29 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 
 	sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
 
-	/* Ignore possible writes if we don't need to write */
-	if (!(sccr1_reg & SSCR1_TIE))
-		mask &= ~SSSR_TFS;
-
 	/* Ignore RX timeout interrupt if it is disabled */
 	if (!(sccr1_reg & SSCR1_TINTE))
 		mask &= ~SSSR_TINT;
 
-	if (!(status & mask))
-		return IRQ_NONE;
+	while (1) {
+		/* Ignore possible writes if we don't need to write */
+		if (!(sccr1_reg & SSCR1_TIE))
+			mask &= ~SSSR_TFS;
 
-	if (!drv_data->master->cur_msg) {
-		handle_bad_msg(drv_data);
-		/* Never fail */
-		return IRQ_HANDLED;
-	}
+		if (!(status & mask))
+			return ret;
+
+		if (!drv_data->master->cur_msg) {
+			handle_bad_msg(drv_data);
+			/* Never fail */
+			return IRQ_HANDLED;
+		}
+
+		ret |= drv_data->transfer_handler(drv_data);
 
-	return drv_data->transfer_handler(drv_data);
+		status = pxa2xx_spi_read(drv_data, SSSR);
+		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
+	}
 }
 
 /*
-- 
2.1.4

[toc] | [next] | [standalone]

#1560000 — Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

On Mon, 2017-01-16 at 19:44 +0100, Jan Kiszka wrote:
> When using the a device with edge-triggered interrupts, such as MSIs,
> the interrupt handler has to ensure that there is a point in time
> during
> its execution where all interrupts sources are silent so that a new
> event can trigger a new interrupt again.
> 
> This is achieved here by looping over SSSR evaluation. We need to take
> into account that SSCR1 may be changed by the transfer handler, thus
> we
> need to redo the mask calculation, at least regarding the volatile
> interrupt enable bit (TIE).
> 

So, more comments/questions below.

>  
>  	sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
>  
> -	/* Ignore possible writes if we don't need to write */
> -	if (!(sccr1_reg & SSCR1_TIE))
> -		mask &= ~SSSR_TFS;
> -
>  	/* Ignore RX timeout interrupt if it is disabled */
>  	if (!(sccr1_reg & SSCR1_TINTE))
>  		mask &= ~SSSR_TINT;
>  
> -	if (!(status & mask))
> -		return IRQ_NONE;
> +	while (1) {

Can we switch to do-while and move previous block here? Btw, can TINTE
bit be set again during a loop?

> +		/* Ignore possible writes if we don't need to write
> */
> +		if (!(sccr1_reg & SSCR1_TIE))
> +			mask &= ~SSSR_TFS;
>  
> -	if (!drv_data->master->cur_msg) {
> -		handle_bad_msg(drv_data);
> -		/* Never fail */
> -		return IRQ_HANDLED;
> -	}
> +		if (!(status & mask))
> +			return ret;
> +
> +		if (!drv_data->master->cur_msg) {
> +			handle_bad_msg(drv_data);
> +			/* Never fail */
> +			return IRQ_HANDLED;
> +		}
> +

> +		ret |= drv_data->transfer_handler(drv_data);

So, we might call handler several times. This needs to be commented in
the code why you do so.

>  
> -	return drv_data->transfer_handler(drv_data);
> +		status = pxa2xx_spi_read(drv_data, SSSR);

Would it be possible to get all 1:s from the register
(something/autosuspend just powered off it by timeout?) ?

> +		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
> +	}
>  }
>  
>  /*

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

#1560020

On 2017-01-16 20:07, Andy Shevchenko wrote:
> On Mon, 2017-01-16 at 19:44 +0100, Jan Kiszka wrote:
>> When using the a device with edge-triggered interrupts, such as MSIs,
>> the interrupt handler has to ensure that there is a point in time
>> during
>> its execution where all interrupts sources are silent so that a new
>> event can trigger a new interrupt again.
>>
>> This is achieved here by looping over SSSR evaluation. We need to take
>> into account that SSCR1 may be changed by the transfer handler, thus
>> we
>> need to redo the mask calculation, at least regarding the volatile
>> interrupt enable bit (TIE).
>>
> 
> So, more comments/questions below.
> 
>>  
>>  	sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
>>  
>> -	/* Ignore possible writes if we don't need to write */
>> -	if (!(sccr1_reg & SSCR1_TIE))
>> -		mask &= ~SSSR_TFS;
>> -
>>  	/* Ignore RX timeout interrupt if it is disabled */
>>  	if (!(sccr1_reg & SSCR1_TINTE))
>>  		mask &= ~SSSR_TINT;
>>  
>> -	if (!(status & mask))
>> -		return IRQ_NONE;
>> +	while (1) {
> 
> Can we switch to do-while and move previous block here?

Don't see how this would help (without duplicating more code).

> Btw, can TINTE
> bit be set again during a loop?

Nope, it's statically set, at least so far.

What we could do is simply restarting ssp_int

> 
>> +		/* Ignore possible writes if we don't need to write
>> */
>> +		if (!(sccr1_reg & SSCR1_TIE))
>> +			mask &= ~SSSR_TFS;
>>  
>> -	if (!drv_data->master->cur_msg) {
>> -		handle_bad_msg(drv_data);
>> -		/* Never fail */
>> -		return IRQ_HANDLED;
>> -	}
>> +		if (!(status & mask))
>> +			return ret;
>> +
>> +		if (!drv_data->master->cur_msg) {
>> +			handle_bad_msg(drv_data);
>> +			/* Never fail */
>> +			return IRQ_HANDLED;
>> +		}
>> +
> 
>> +		ret |= drv_data->transfer_handler(drv_data);
> 
> So, we might call handler several times. This needs to be commented in
> the code why you do so.

I can move the commit log into the code.

> 
>>  
>> -	return drv_data->transfer_handler(drv_data);
>> +		status = pxa2xx_spi_read(drv_data, SSSR);
> 
> Would it be possible to get all 1:s from the register
> (something/autosuspend just powered off it by timeout?) ?
> 

Not sure if that can happen, but I guess it would be simpler and more
readable to simply do this instead:

	while (1) {
		/*
		 * If the device is not yet in RPM suspended state and we get an
		 * interrupt that is meant for another device, check if status
		 * bits are all set to one. That means that the device is
		 * already powered off.
		 */
		status = pxa2xx_spi_read(drv_data, SSSR);
		if (status == ~0)
			return ret;

		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);

		/* Ignore RX timeout interrupt if it is disabled */
		if (!(sccr1_reg & SSCR1_TINTE))
			mask &= ~SSSR_TINT;

		/* Ignore possible writes if we don't need to write */
		if (!(sccr1_reg & SSCR1_TIE))
			mask &= ~SSSR_TFS;

		if (!(status & mask))
			return ret;

		if (!drv_data->master->cur_msg) {
			handle_bad_msg(drv_data);
			/* Never fail */
			return IRQ_HANDLED;
		}

		ret |= drv_data->transfer_handler(drv_data);
	}


i.e. preserve the current structure, just add the loop.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1560948

On 2017-01-16 20:46, Jan Kiszka wrote:
> On 2017-01-16 20:07, Andy Shevchenko wrote:
>> On Mon, 2017-01-16 at 19:44 +0100, Jan Kiszka wrote:
>>> When using the a device with edge-triggered interrupts, such as MSIs,
>>> the interrupt handler has to ensure that there is a point in time
>>> during
>>> its execution where all interrupts sources are silent so that a new
>>> event can trigger a new interrupt again.
>>>
>>> This is achieved here by looping over SSSR evaluation. We need to take
>>> into account that SSCR1 may be changed by the transfer handler, thus
>>> we
>>> need to redo the mask calculation, at least regarding the volatile
>>> interrupt enable bit (TIE).
>>>
>>
>> So, more comments/questions below.
>>
>>>  
>>>  	sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
>>>  
>>> -	/* Ignore possible writes if we don't need to write */
>>> -	if (!(sccr1_reg & SSCR1_TIE))
>>> -		mask &= ~SSSR_TFS;
>>> -
>>>  	/* Ignore RX timeout interrupt if it is disabled */
>>>  	if (!(sccr1_reg & SSCR1_TINTE))
>>>  		mask &= ~SSSR_TINT;
>>>  
>>> -	if (!(status & mask))
>>> -		return IRQ_NONE;
>>> +	while (1) {
>>
>> Can we switch to do-while and move previous block here?
> 
> Don't see how this would help (without duplicating more code).
> 
>> Btw, can TINTE
>> bit be set again during a loop?
> 
> Nope, it's statically set, at least so far.
> 
> What we could do is simply restarting ssp_int
> 
>>
>>> +		/* Ignore possible writes if we don't need to write
>>> */
>>> +		if (!(sccr1_reg & SSCR1_TIE))
>>> +			mask &= ~SSSR_TFS;
>>>  
>>> -	if (!drv_data->master->cur_msg) {
>>> -		handle_bad_msg(drv_data);
>>> -		/* Never fail */
>>> -		return IRQ_HANDLED;
>>> -	}
>>> +		if (!(status & mask))
>>> +			return ret;
>>> +
>>> +		if (!drv_data->master->cur_msg) {
>>> +			handle_bad_msg(drv_data);
>>> +			/* Never fail */
>>> +			return IRQ_HANDLED;
>>> +		}
>>> +
>>
>>> +		ret |= drv_data->transfer_handler(drv_data);
>>
>> So, we might call handler several times. This needs to be commented in
>> the code why you do so.
> 
> I can move the commit log into the code.
> 
>>
>>>  
>>> -	return drv_data->transfer_handler(drv_data);
>>> +		status = pxa2xx_spi_read(drv_data, SSSR);
>>
>> Would it be possible to get all 1:s from the register
>> (something/autosuspend just powered off it by timeout?) ?
>>
> 
> Not sure if that can happen, but I guess it would be simpler and more
> readable to simply do this instead:
> 
> 	while (1) {
> 		/*
> 		 * If the device is not yet in RPM suspended state and we get an
> 		 * interrupt that is meant for another device, check if status
> 		 * bits are all set to one. That means that the device is
> 		 * already powered off.
> 		 */
> 		status = pxa2xx_spi_read(drv_data, SSSR);
> 		if (status == ~0)
> 			return ret;
> 
> 		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
> 
> 		/* Ignore RX timeout interrupt if it is disabled */
> 		if (!(sccr1_reg & SSCR1_TINTE))
> 			mask &= ~SSSR_TINT;
> 
> 		/* Ignore possible writes if we don't need to write */
> 		if (!(sccr1_reg & SSCR1_TIE))
> 			mask &= ~SSSR_TFS;
> 
> 		if (!(status & mask))
> 			return ret;
> 
> 		if (!drv_data->master->cur_msg) {
> 			handle_bad_msg(drv_data);
> 			/* Never fail */
> 			return IRQ_HANDLED;
> 		}
> 
> 		ret |= drv_data->transfer_handler(drv_data);
> 	}
> 
> 
> i.e. preserve the current structure, just add the loop.
> 

OK, please let me know if you want a v3 of this patch with the structure
above. If there are further concerns/questions, just let me know as
well, but I'd like to close this topic if possible.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1560315

Jan Kiszka <jan.kiszka@siemens.com> writes:

> When using the a device with edge-triggered interrupts, such as MSIs,
> the interrupt handler has to ensure that there is a point in time during
> its execution where all interrupts sources are silent so that a new
> event can trigger a new interrupt again.
>
> This is achieved here by looping over SSSR evaluation. We need to take
> into account that SSCR1 may be changed by the transfer handler, thus we
> need to redo the mask calculation, at least regarding the volatile
> interrupt enable bit (TIE).

I'd like moreover to add a question here.

In pxa architecture, SPI interrupts are already edge-triggered, and it's working
well. The interrupt source disabling is not disabled, but the interrupt
controller doesn't trigger an interrupt anymore (as it is masked), yet it marks
it as pending if an interrupt arrives while the interrupt handler is running.

All of this is handled by the interrupt core. My question is why for Intel MSI's
is it necessary to make a change in the driver instead or relying on the
interrupt core as for the pxa ?

Cheers.

--
Robert

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

#1560335

On 2017-01-17 08:58, Robert Jarzmik wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> When using the a device with edge-triggered interrupts, such as MSIs,
>> the interrupt handler has to ensure that there is a point in time during
>> its execution where all interrupts sources are silent so that a new
>> event can trigger a new interrupt again.
>>
>> This is achieved here by looping over SSSR evaluation. We need to take
>> into account that SSCR1 may be changed by the transfer handler, thus we
>> need to redo the mask calculation, at least regarding the volatile
>> interrupt enable bit (TIE).
> 
> I'd like moreover to add a question here.
> 
> In pxa architecture, SPI interrupts are already edge-triggered, and it's working
> well. The interrupt source disabling is not disabled, but the interrupt
> controller doesn't trigger an interrupt anymore (as it is masked), yet it marks
> it as pending if an interrupt arrives while the interrupt handler is running.
> 
> All of this is handled by the interrupt core. My question is why for Intel MSI's
> is it necessary to make a change in the driver instead or relying on the
> interrupt core as for the pxa ?

If someone was using this driver with edge-triggered interrupt sources
so far, it was probably slower hardware and some luck (I've seen this
when driving fast-clocked devices vs. slower ones - only the latter
exposed the bug). Or that hardware did some temporary masking at
interrupt controller level while the handler was running. But that is
also not by design. It's the driver's task to ensure that all interrupt
sources are addressed once when returning from an edge-triggered
handler, and that is missing in this one.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1560577

On 01/17/2017 10:10 AM, Jan Kiszka wrote:
> On 2017-01-17 08:58, Robert Jarzmik wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>
>>> When using the a device with edge-triggered interrupts, such as MSIs,
>>> the interrupt handler has to ensure that there is a point in time during
>>> its execution where all interrupts sources are silent so that a new
>>> event can trigger a new interrupt again.
>>>
>>> This is achieved here by looping over SSSR evaluation. We need to take
>>> into account that SSCR1 may be changed by the transfer handler, thus we
>>> need to redo the mask calculation, at least regarding the volatile
>>> interrupt enable bit (TIE).
>>
>> I'd like moreover to add a question here.
>>
>> In pxa architecture, SPI interrupts are already edge-triggered, and it's working
>> well. The interrupt source disabling is not disabled, but the interrupt
>> controller doesn't trigger an interrupt anymore (as it is masked), yet it marks
>> it as pending if an interrupt arrives while the interrupt handler is running.
>>
>> All of this is handled by the interrupt core. My question is why for Intel MSI's
>> is it necessary to make a change in the driver instead or relying on the
>> interrupt core as for the pxa ?
>
> If someone was using this driver with edge-triggered interrupt sources
> so far, it was probably slower hardware and some luck (I've seen this
> when driving fast-clocked devices vs. slower ones - only the latter
> exposed the bug). Or that hardware did some temporary masking at
> interrupt controller level while the handler was running. But that is
> also not by design. It's the driver's task to ensure that all interrupt
> sources are addressed once when returning from an edge-triggered
> handler, and that is missing in this one.
>
Are you seeing actual problem here or adding loop just in case? Is it 
really so that PCI bridge doesn't generate another MSI interrupt if SPI 
controller has interrupt pending when handler returns? I don't know but 
I would expect irq line between SPI controller and PCI bridge is still 
level sensitive even PCI bridge issues MSIs to the CPU.

-- 
Jarkko

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

#1560678

On 2017-01-17 14:11, Jarkko Nikula wrote:
> On 01/17/2017 10:10 AM, Jan Kiszka wrote:
>> On 2017-01-17 08:58, Robert Jarzmik wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> When using the a device with edge-triggered interrupts, such as MSIs,
>>>> the interrupt handler has to ensure that there is a point in time
>>>> during
>>>> its execution where all interrupts sources are silent so that a new
>>>> event can trigger a new interrupt again.
>>>>
>>>> This is achieved here by looping over SSSR evaluation. We need to take
>>>> into account that SSCR1 may be changed by the transfer handler, thus we
>>>> need to redo the mask calculation, at least regarding the volatile
>>>> interrupt enable bit (TIE).
>>>
>>> I'd like moreover to add a question here.
>>>
>>> In pxa architecture, SPI interrupts are already edge-triggered, and
>>> it's working
>>> well. The interrupt source disabling is not disabled, but the interrupt
>>> controller doesn't trigger an interrupt anymore (as it is masked),
>>> yet it marks
>>> it as pending if an interrupt arrives while the interrupt handler is
>>> running.
>>>
>>> All of this is handled by the interrupt core. My question is why for
>>> Intel MSI's
>>> is it necessary to make a change in the driver instead or relying on the
>>> interrupt core as for the pxa ?
>>
>> If someone was using this driver with edge-triggered interrupt sources
>> so far, it was probably slower hardware and some luck (I've seen this
>> when driving fast-clocked devices vs. slower ones - only the latter
>> exposed the bug). Or that hardware did some temporary masking at
>> interrupt controller level while the handler was running. But that is
>> also not by design. It's the driver's task to ensure that all interrupt
>> sources are addressed once when returning from an edge-triggered
>> handler, and that is missing in this one.
>>
> Are you seeing actual problem here or adding loop just in case? Is it
> really so that PCI bridge doesn't generate another MSI interrupt if SPI
> controller has interrupt pending when handler returns? I don't know but
> I would expect irq line between SPI controller and PCI bridge is still
> level sensitive even PCI bridge issues MSIs to the CPU.
> 

Yes, I'm seeing real problems, e.g. on the Galileo board when running
against a slower SPI device and using MSI: An interrupt is raised
because the TX queue was flushed towards the device (threshold
underrun). While handling that interrupt, the device starts to respond
and an RX event occurs as well. This raises the related interrupt reason
before the TX source was satisfied. Therefore, the interrupt output of
the SPI master will never go down, and there will be no additional edge
generated. Using level-interrupts, this is no problem, but with
edge-triggered we get stuck.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1560317

Jan Kiszka <jan.kiszka@siemens.com> writes:

> When using the a device with edge-triggered interrupts, such as MSIs,
> the interrupt handler has to ensure that there is a point in time during
> its execution where all interrupts sources are silent so that a new
> event can trigger a new interrupt again.
>
> This is achieved here by looping over SSSR evaluation. We need to take
> into account that SSCR1 may be changed by the transfer handler, thus we
> need to redo the mask calculation, at least regarding the volatile
> interrupt enable bit (TIE).
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Hi Jan,

> +	while (1) {
This bit worries me a bit, as this can be either :
 - hogging the SoC's CPU, endlessly running
 - or even worse, blocking the CPU for ever

The question behind is, should this be done in a top-half, or moved to a irq
thread ?

> +		/* Ignore possible writes if we don't need to write */
> +		if (!(sccr1_reg & SSCR1_TIE))
> +			mask &= ~SSSR_TFS;
>  
> -	if (!drv_data->master->cur_msg) {
> -		handle_bad_msg(drv_data);
> -		/* Never fail */
> -		return IRQ_HANDLED;
> -	}
> +		if (!(status & mask))
> +			return ret;
> +
> +		if (!drv_data->master->cur_msg) {
> +			handle_bad_msg(drv_data);
> +			/* Never fail */
> +			return IRQ_HANDLED;
> +		}
> +
> +		ret |= drv_data->transfer_handler(drv_data);
Mmm that looks weird to me, oring a irqreturn.

Imagine that on first iteration the handler returns IRQ_NONE, and on second
IRQ_HANDLED. This makes ret IRQ_HANDLED. Yet after the first iteration the
handler should have exited, especially if the interrupt is shared with another
driver.

Another thing which is along what Andy already said : it would be better
practice to have this loop in the form :
do {
...
} while (exit_condition_not_met);

Just for maintainability, it's better, and it concentrates the test on the
"exit_condition_not_met" in one place, which will enable us to review better the
algorithm.

Cheers.

-- 
Robert

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

#1560324

On 2017-01-17 08:54, Robert Jarzmik wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> When using the a device with edge-triggered interrupts, such as MSIs,
>> the interrupt handler has to ensure that there is a point in time during
>> its execution where all interrupts sources are silent so that a new
>> event can trigger a new interrupt again.
>>
>> This is achieved here by looping over SSSR evaluation. We need to take
>> into account that SSCR1 may be changed by the transfer handler, thus we
>> need to redo the mask calculation, at least regarding the volatile
>> interrupt enable bit (TIE).
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Hi Jan,
> 
>> +	while (1) {
> This bit worries me a bit, as this can be either :
>  - hogging the SoC's CPU, endlessly running
>  - or even worse, blocking the CPU for ever
> 
> The question behind is, should this be done in a top-half, or moved to a irq
> thread ?

Every device with a broken interrupt source can hog CPUs, nothing
special with this one. If you don't close the loop in the handler
itself, you close it over the hardware retriggering the interrupt over
and over again.

So, I don't see a point in offloading to a thread. The normal case is
some TX done (FIFO available) event followed by an RX event, then the
transfer is complete, isn't it?

> 
>> +		/* Ignore possible writes if we don't need to write */
>> +		if (!(sccr1_reg & SSCR1_TIE))
>> +			mask &= ~SSSR_TFS;
>>  
>> -	if (!drv_data->master->cur_msg) {
>> -		handle_bad_msg(drv_data);
>> -		/* Never fail */
>> -		return IRQ_HANDLED;
>> -	}
>> +		if (!(status & mask))
>> +			return ret;
>> +
>> +		if (!drv_data->master->cur_msg) {
>> +			handle_bad_msg(drv_data);
>> +			/* Never fail */
>> +			return IRQ_HANDLED;
>> +		}
>> +
>> +		ret |= drv_data->transfer_handler(drv_data);
> Mmm that looks weird to me, oring a irqreturn.

Not really an uncommon pattern, though.

> 
> Imagine that on first iteration the handler returns IRQ_NONE, and on second
> IRQ_HANDLED. This makes ret IRQ_HANDLED. Yet after the first iteration the
> handler should have exited, especially if the interrupt is shared with another
> driver.

That would be a bug in transfer_handler, because we don't enter it
without a reason (status != 0).

> 
> Another thing which is along what Andy already said : it would be better
> practice to have this loop in the form :
> do {
> ...
> } while (exit_condition_not_met);

This implies code duplication in order to calculate the condition
(mask...). I can do this if desired, I wouldn't do this to my own code,
though.

Jan

> 
> Just for maintainability, it's better, and it concentrates the test on the
> "exit_condition_not_met" in one place, which will enable us to review better the
> algorithm.
> 
> Cheers.
> 


-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1561352

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 2017-01-17 08:54, Robert Jarzmik wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> When using the a device with edge-triggered interrupts, such as MSIs,
>>> the interrupt handler has to ensure that there is a point in time during
>>> its execution where all interrupts sources are silent so that a new
>>> event can trigger a new interrupt again.
>>>
>>> This is achieved here by looping over SSSR evaluation. We need to take
>>> into account that SSCR1 may be changed by the transfer handler, thus we
>>> need to redo the mask calculation, at least regarding the volatile
>>> interrupt enable bit (TIE).
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Hi Jan,
>> 
>>> +	while (1) {
>> This bit worries me a bit, as this can be either :
>>  - hogging the SoC's CPU, endlessly running
>>  - or even worse, blocking the CPU for ever
>> 
>> The question behind is, should this be done in a top-half, or moved to a irq
>> thread ?
>
> Every device with a broken interrupt source can hog CPUs, nothing
> special with this one. If you don't close the loop in the handler
> itself, you close it over the hardware retriggering the interrupt over
> and over again.
I'm not speaking of a broken interrupt source, I'm speaking of a broken code,
such as in the handler, or broken status readback, or lack of understanding on
the status register which may imply the while(1) to loop forever.

> So, I don't see a point in offloading to a thread. The normal case is
> some TX done (FIFO available) event followed by an RX event, then the
> transfer is complete, isn't it?
The point is if you stay forever in the while(1) loop, you can at least have a
print a backtrace (LOCKUP_DETECTOR).

>> Imagine that on first iteration the handler returns IRQ_NONE, and on second
>> IRQ_HANDLED. This makes ret IRQ_HANDLED. Yet after the first iteration the
>> handler should have exited, especially if the interrupt is shared with another
>> driver.
>
> That would be a bug in transfer_handler, because we don't enter it
> without a reason (status != 0).
Sure, but can you be sure that all the people modifying the code after you will
see that also ? The other way will _force_ them to see it.

>> Another thing which is along what Andy already said : it would be better
>> practice to have this loop in the form :
>> do {
>> ...
>> } while (exit_condition_not_met);
>
> This implies code duplication in order to calculate the condition
> (mask...). I can do this if desired, I wouldn't do this to my own code,
> though.
Okay, that's acceptable.
Why not have something like this :

sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
if (!(sccr1_reg & SSCR1_TIE))
	mask &= ~SSSR_TFS;

/* Ignore RX timeout interrupt if it is disabled */
if (!(sccr1_reg & SSCR1_TINTE))
	mask &= ~SSSR_TINT;

status = pxa2xx_spi_read(drv_data, SSR);
while (status & mask) {
   ... handlers etc ...
   status = pxa2xx_spi_read(drv_data, SSR);
};

There is a duplication of the status read, but that looks acceptable, and the
mask calculation is moved out of the loop (this should be checked more
thoroughly as it looked to me only probe() would change these values, yet I
might be wrong).

Cheers.

-- 
Robert

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

#1561395

On 2017-01-18 09:21, Robert Jarzmik wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> On 2017-01-17 08:54, Robert Jarzmik wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> When using the a device with edge-triggered interrupts, such as MSIs,
>>>> the interrupt handler has to ensure that there is a point in time during
>>>> its execution where all interrupts sources are silent so that a new
>>>> event can trigger a new interrupt again.
>>>>
>>>> This is achieved here by looping over SSSR evaluation. We need to take
>>>> into account that SSCR1 may be changed by the transfer handler, thus we
>>>> need to redo the mask calculation, at least regarding the volatile
>>>> interrupt enable bit (TIE).
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> Hi Jan,
>>>
>>>> +	while (1) {
>>> This bit worries me a bit, as this can be either :
>>>  - hogging the SoC's CPU, endlessly running
>>>  - or even worse, blocking the CPU for ever
>>>
>>> The question behind is, should this be done in a top-half, or moved to a irq
>>> thread ?
>>
>> Every device with a broken interrupt source can hog CPUs, nothing
>> special with this one. If you don't close the loop in the handler
>> itself, you close it over the hardware retriggering the interrupt over
>> and over again.
> I'm not speaking of a broken interrupt source, I'm speaking of a broken code,
> such as in the handler, or broken status readback, or lack of understanding on
> the status register which may imply the while(1) to loop forever.
> 
>> So, I don't see a point in offloading to a thread. The normal case is
>> some TX done (FIFO available) event followed by an RX event, then the
>> transfer is complete, isn't it?
> The point is if you stay forever in the while(1) loop, you can at least have a
> print a backtrace (LOCKUP_DETECTOR).

I won't consider "debugability" as a good reason to move interrupt
handlers into threads. There should be real workload that requires
offloading or specific prioritization.

> 
>>> Imagine that on first iteration the handler returns IRQ_NONE, and on second
>>> IRQ_HANDLED. This makes ret IRQ_HANDLED. Yet after the first iteration the
>>> handler should have exited, especially if the interrupt is shared with another
>>> driver.
>>
>> That would be a bug in transfer_handler, because we don't enter it
>> without a reason (status != 0).
> Sure, but can you be sure that all the people modifying the code after you will
> see that also ? The other way will _force_ them to see it.
> 
>>> Another thing which is along what Andy already said : it would be better
>>> practice to have this loop in the form :
>>> do {
>>> ...
>>> } while (exit_condition_not_met);
>>
>> This implies code duplication in order to calculate the condition
>> (mask...). I can do this if desired, I wouldn't do this to my own code,
>> though.
> Okay, that's acceptable.
> Why not have something like this :
> 
> sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
> if (!(sccr1_reg & SSCR1_TIE))
> 	mask &= ~SSSR_TFS;
> 
> /* Ignore RX timeout interrupt if it is disabled */
> if (!(sccr1_reg & SSCR1_TINTE))
> 	mask &= ~SSSR_TINT;
> 
> status = pxa2xx_spi_read(drv_data, SSR);
> while (status & mask) {
>    ... handlers etc ...
>    status = pxa2xx_spi_read(drv_data, SSR);
> };
> 
> There is a duplication of the status read, but that looks acceptable, and the
> mask calculation is moved out of the loop (this should be checked more
> thoroughly as it looked to me only probe() would change these values, yet I
> might be wrong).

Unfortunately, mask can change if SSCR1_TIE is cleared. So this is not
correct.

What would be an alternative to looping is masking (would be required
for threaded irq anyway - but then we won't need to loop in the first
place): disable all irq sources, check the status bits once, re-enable
according to a potentially updated set, leave the handler and let the
hardware call us again.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1561726

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jan 18, 2017 at 10:33:07AM +0100, Jan Kiszka wrote:
> On 2017-01-18 09:21, Robert Jarzmik wrote:

> >>>> +	while (1) {

> >>> This bit worries me a bit, as this can be either :
> >>>  - hogging the SoC's CPU, endlessly running
> >>>  - or even worse, blocking the CPU for ever

> >>> The question behind is, should this be done in a top-half, or moved to a irq
> >>> thread ?

> >> Every device with a broken interrupt source can hog CPUs, nothing
> >> special with this one. If you don't close the loop in the handler
> >> itself, you close it over the hardware retriggering the interrupt over
> >> and over again.

> > I'm not speaking of a broken interrupt source, I'm speaking of a broken code,
> > such as in the handler, or broken status readback, or lack of understanding on
> > the status register which may imply the while(1) to loop forever.

> >> So, I don't see a point in offloading to a thread. The normal case is
> >> some TX done (FIFO available) event followed by an RX event, then the
> >> transfer is complete, isn't it?

> > The point is if you stay forever in the while(1) loop, you can at least have a
> > print a backtrace (LOCKUP_DETECTOR).

> I won't consider "debugability" as a good reason to move interrupt
> handlers into threads. There should be real workload that requires
> offloading or specific prioritization.

It's failure mitigation - you're translating a hard lockup into
something that will potentially allow the system to soldier on which is
likely to be less severe for the user as well as making things easier to
figure out.  If we're doing something like this I'd at least have a
limit on how long we allow the interrupt to scream.

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

#1562862

[Multipart message — attachments visible in raw view] — view raw

On 2017-01-18 13:46, Mark Brown wrote:
> On Wed, Jan 18, 2017 at 10:33:07AM +0100, Jan Kiszka wrote:
>> On 2017-01-18 09:21, Robert Jarzmik wrote:
> 
>>>>>> +	while (1) {
> 
>>>>> This bit worries me a bit, as this can be either :
>>>>>  - hogging the SoC's CPU, endlessly running
>>>>>  - or even worse, blocking the CPU for ever
> 
>>>>> The question behind is, should this be done in a top-half, or moved to a irq
>>>>> thread ?
> 
>>>> Every device with a broken interrupt source can hog CPUs, nothing
>>>> special with this one. If you don't close the loop in the handler
>>>> itself, you close it over the hardware retriggering the interrupt over
>>>> and over again.
> 
>>> I'm not speaking of a broken interrupt source, I'm speaking of a broken code,
>>> such as in the handler, or broken status readback, or lack of understanding on
>>> the status register which may imply the while(1) to loop forever.
> 
>>>> So, I don't see a point in offloading to a thread. The normal case is
>>>> some TX done (FIFO available) event followed by an RX event, then the
>>>> transfer is complete, isn't it?
> 
>>> The point is if you stay forever in the while(1) loop, you can at least have a
>>> print a backtrace (LOCKUP_DETECTOR).
> 
>> I won't consider "debugability" as a good reason to move interrupt
>> handlers into threads. There should be real workload that requires
>> offloading or specific prioritization.
> 
> It's failure mitigation - you're translating a hard lockup into
> something that will potentially allow the system to soldier on which is
> likely to be less severe for the user as well as making things easier to
> figure out.  If we're doing something like this I'd at least have a
> limit on how long we allow the interrupt to scream.
> 

OK, OK, if that is the biggest worry, I can change the pattern from
loop-based to SCCR1-based, i.e. mask all interrupt sources once per
interrupt so that we enforce a falling edge. Fine.

But now I'm looking at the driver, wondering who all is fiddling under
which conditions with SCCR1. There are a lot of RMW patterns, but I do
not see the locking pattern behind that. Are all RMW accesses run only
in the interrupt handler context? Unlikely, at least with the dmaengine
in the loop.

Closing my eyes regarding this potential issue for now, the patch could
become as simple as

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 0d10090..f9c2329 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -785,6 +785,9 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 	if (!(status & mask))
 		return IRQ_NONE;
 
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg & ~drv_data->int_cr1);
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg);
+
 	if (!drv_data->master->cur_msg) {
 		handle_bad_msg(drv_data);
 		/* Never fail */

Not efficient /wrt register accesses, but that's apparently not yet
a design goal anyway (I stumbled over the SSCR1 locking while
considering to introduce a cache for that reg).

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1563037 — [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Multipart message — attachments visible in raw view] — view raw

When using the a device with edge-triggered interrupts, such as MSIs,
the interrupt handler has to ensure that there is a point in time during
its execution where all interrupts sources are silent so that a new
event can trigger a new interrupt again.

This is achieved here by disabling all interrupt sources for a moment
before processing them according to the status register. If a new
interrupt should have arrived after we read the status, it will now
re-trigger the interrupt, even in edge mode.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Successfully tested by now.

Changes to v2:
- avoid inner looping by making the hardware retrigger on "forgotten"
  IRQ sources

 drivers/spi/spi-pxa2xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 0d10090..f9c2329 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -785,6 +785,9 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 	if (!(status & mask))
 		return IRQ_NONE;
 
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg & ~drv_data->int_cr1);
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg);
+
 	if (!drv_data->master->cur_msg) {
 		handle_bad_msg(drv_data);
 		/* Never fail */
-- 
2.1.4

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

#1563054 — Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

On Thu, 2017-01-19 at 19:57 +0000, Mark Brown wrote:
> On Thu, Jan 19, 2017 at 08:37:40PM +0100, Jan Kiszka wrote:
> > When using the a device with edge-triggered interrupts, such as
> > MSIs,
> > the interrupt handler has to ensure that there is a point in time
> > during
> 
> I'm missing patches 1 and 3, what's going on here?

Patch 1 had been applied by you. I think Jan just needs to resend with
proper version and set of patches.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

#1563533 — Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jan 19, 2017 at 10:04:32PM +0200, Andy Shevchenko wrote:
> On Thu, 2017-01-19 at 19:57 +0000, Mark Brown wrote:

> > I'm missing patches 1 and 3, what's going on here?

> Patch 1 had been applied by you. I think Jan just needs to resend with
> proper version and set of patches.

OK - Jan, the purpose of numbering patches in a series is so we can tell
what order they come in.  That's it.  If the set of patches gets changed
due to things being added or removed the numbers will be different but
that's totally OK as their only relevance is in ordering things within a
given posting.

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

#1563719 — Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Multipart message — attachments visible in raw view] — view raw

On 2017-01-20 13:21, Mark Brown wrote:
> On Thu, Jan 19, 2017 at 10:04:32PM +0200, Andy Shevchenko wrote:
>> On Thu, 2017-01-19 at 19:57 +0000, Mark Brown wrote:
> 
>>> I'm missing patches 1 and 3, what's going on here?
> 
>> Patch 1 had been applied by you. I think Jan just needs to resend with
>> proper version and set of patches.
> 
> OK - Jan, the purpose of numbering patches in a series is so we can tell
> what order they come in.  That's it.  If the set of patches gets changed
> due to things being added or removed the numbers will be different but
> that's totally OK as their only relevance is in ordering things within a
> given posting.

Sorry if the numbering was confusing for you, it was, of course, a
selective update. Just tell me, if you need a new round with of patches
2 and 3 (as 1 and 2).

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1563814 — Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jan 20, 2017 at 04:29:28PM +0100, Jan Kiszka wrote:

> Sorry if the numbering was confusing for you, it was, of course, a
> selective update. Just tell me, if you need a new round with of patches
> 2 and 3 (as 1 and 2).

Please resend.

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

#1563066 — Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jan 19, 2017 at 08:37:40PM +0100, Jan Kiszka wrote:
> When using the a device with edge-triggered interrupts, such as MSIs,
> the interrupt handler has to ensure that there is a point in time during

I'm missing patches 1 and 3, what's going on here?

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web