[PATCH] net/cadence/macb: clear interrupts simply and correctly

[PATCH] net/cadence/macb: clear interrupts simply and correctly

[Jongsung Kim]

The "Rx used bit read" interrupt is enabled but not cleared for some
systems with the ISR (Interrupt Status Register) configured as clear-
on-write. This interrupt may be asserted when the CPU does not handle
Rx-complete interrupts for a long time. (e.g., if the CPU is stopped
by debugger) Once asserted, it'll not be cleared, and the CPU will
loop infinitly in the interrupt handler.

This patch forces to use a dedicated function for reading the ISR,
and the function clears it if clear-on-write. So the ISR is always
cleared after read, regardless of clear-on-write configuration.

Reported-by: Hayun Hwang <hwang.hayun@lge.com>
Signed-off-by: Youngkyu Choi <youngkyu7.choi@lge.com>
Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
Tested-by: Hayun Hwang <hwang.hayun@lge.com>
---
 drivers/net/ethernet/cadence/macb.c |   37 ++++++++++++++--------------------
 1 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index e9daa07..21cc022 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -98,6 +98,16 @@ static void *macb_rx_buffer(struct macb *bp, unsigned int index)
 	return bp->rx_buffers + bp->rx_buffer_size * macb_rx_ring_wrap(index);
 }
 
+static u32 macb_read_isr(struct macb *bp)
+{
+	u32 status = macb_readl(bp, ISR);
+
+	if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+		macb_writel(bp, ISR, status);
+
+	return status;
+}
+
 void macb_set_hwaddr(struct macb *bp)
 {
 	u32 bottom;
@@ -552,9 +562,6 @@ static void macb_tx_interrupt(struct macb *bp)
 	status = macb_readl(bp, TSR);
 	macb_writel(bp, TSR, status);
 
-	if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
-		macb_writel(bp, ISR, MACB_BIT(TCOMP));
-
 	netdev_vdbg(bp->dev, "macb_tx_interrupt status = 0x%03lx\n",
 		(unsigned long)status);
 
@@ -883,13 +890,10 @@ static int macb_poll(struct napi_struct *napi, int budget)
 
 		/* Packets received while interrupts were disabled */
 		status = macb_readl(bp, RSR);
-		if (status) {
-			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
-				macb_writel(bp, ISR, MACB_BIT(RCOMP));
+		if (status)
 			napi_reschedule(napi);
-		} else {
+		else
 			macb_writel(bp, IER, MACB_RX_INT_FLAGS);
-		}
 	}
 
 	/* TODO: Handle errors */
@@ -903,7 +907,7 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 	struct macb *bp = netdev_priv(dev);
 	u32 status;
 
-	status = macb_readl(bp, ISR);
+	status = macb_read_isr(bp);
 
 	if (unlikely(!status))
 		return IRQ_NONE;
@@ -928,8 +932,6 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 			 * now.
 			 */
 			macb_writel(bp, IDR, MACB_RX_INT_FLAGS);
-			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
-				macb_writel(bp, ISR, MACB_BIT(RCOMP));
 
 			if (napi_schedule_prep(&bp->napi)) {
 				netdev_vdbg(bp->dev, "scheduling RX softirq\n");
@@ -941,9 +943,6 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 			macb_writel(bp, IDR, MACB_TX_INT_FLAGS);
 			schedule_work(&bp->tx_error_task);
 
-			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
-				macb_writel(bp, ISR, MACB_TX_ERR_FLAGS);
-
 			break;
 		}
 
@@ -961,9 +960,6 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 				bp->hw_stats.gem.rx_overruns++;
 			else
 				bp->hw_stats.macb.rx_overruns++;
-
-			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
-				macb_writel(bp, ISR, MACB_BIT(ISR_ROVR));
 		}
 
 		if (status & MACB_BIT(HRESP)) {
@@ -973,12 +969,9 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 			 * (work queue?)
 			 */
 			netdev_err(dev, "DMA bus error: HRESP not OK\n");
-
-			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
-				macb_writel(bp, ISR, MACB_BIT(HRESP));
 		}
 
-		status = macb_readl(bp, ISR);
+		status = macb_read_isr(bp);
 	}
 
 	spin_unlock(&bp->lock);
@@ -1273,7 +1266,7 @@ static void macb_reset_hw(struct macb *bp)
 
 	/* Disable all interrupts */
 	macb_writel(bp, IDR, -1);
-	macb_readl(bp, ISR);
+	macb_read_isr(bp);
 }
 
 static u32 gem_mdc_clk_div(struct macb *bp)
-- 
1.7.1

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Sören Brinkmann]

Hi Jongsung,

On Thu, 2014-06-12 at 05:50PM +0900, Jongsung Kim wrote:
> The "Rx used bit read" interrupt is enabled but not cleared for some
> systems with the ISR (Interrupt Status Register) configured as clear-
> on-write.
Does this interrupt need to be enabled? There is nothing checking
that bit and handling this IRQ in the handler, AFAICT. And you solve
this by simply clearing the bit. So, I wonder whether not enabling this
IRQ in the first place would solve things too.

> This interrupt may be asserted when the CPU does not handle
> Rx-complete interrupts for a long time. (e.g., if the CPU is stopped
> by debugger) Once asserted, it'll not be cleared, and the CPU will
> loop infinitly in the interrupt handler.
> 
> This patch forces to use a dedicated function for reading the ISR,
> and the function clears it if clear-on-write. So the ISR is always
> cleared after read, regardless of clear-on-write configuration.
> 
> Reported-by: Hayun Hwang <hwang.hayun@lge.com>
> Signed-off-by: Youngkyu Choi <youngkyu7.choi@lge.com>
> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> Tested-by: Hayun Hwang <hwang.hayun@lge.com>
> ---
>  drivers/net/ethernet/cadence/macb.c |   37 ++++++++++++++--------------------
>  1 files changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index e9daa07..21cc022 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -98,6 +98,16 @@ static void *macb_rx_buffer(struct macb *bp, unsigned int index)
>  	return bp->rx_buffers + bp->rx_buffer_size * macb_rx_ring_wrap(index);
>  }
>  
> +static u32 macb_read_isr(struct macb *bp)
> +{
> +	u32 status = macb_readl(bp, ISR);
> +
> +	if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> +		macb_writel(bp, ISR, status);
> +
> +	return status;
> +}
> +
>  void macb_set_hwaddr(struct macb *bp)
>  {
>  	u32 bottom;
> @@ -552,9 +562,6 @@ static void macb_tx_interrupt(struct macb *bp)
>  	status = macb_readl(bp, TSR);
>  	macb_writel(bp, TSR, status);
>  
> -	if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> -		macb_writel(bp, ISR, MACB_BIT(TCOMP));
> -
>  	netdev_vdbg(bp->dev, "macb_tx_interrupt status = 0x%03lx\n",
>  		(unsigned long)status);
>  
> @@ -883,13 +890,10 @@ static int macb_poll(struct napi_struct *napi, int budget)
>  
>  		/* Packets received while interrupts were disabled */
>  		status = macb_readl(bp, RSR);
This is now clearing all IRQ flags which is probably not what we want
here. This is handling RX only. We still want the non-RX interrupts to go to
the actual interrupt service routing.

	Sören

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Jongsung Kim]

On 06/13/2014 12:44 AM, Sören Brinkmann wrote:
> Hi Jongsung,

Hi Sören,

> Does this interrupt need to be enabled? There is nothing checking
> that bit and handling this IRQ in the handler, AFAICT. And you solve
> this by simply clearing the bit. So, I wonder whether not enabling this
> IRQ in the first place would solve things too.

The driver actually checks the bit, but does not clear it. Disabling the
"Rx used bit read" interrupt you said may be a solution. However, I think
it is the better way to clear the exceptional HW-state rather than just to
mask it.

> This is now clearing all IRQ flags which is probably not what we want
> here. This is handling RX only. We still want the non-RX interrupts to go to
> the actual interrupt service routing.

The ISR(Interrupt Status Register) is read only in the interrupt service
routine, macb_interrupt. But is partially cleared here and there. Further
handler-functions decide jobs to be done by reading/checking other status
registers. (e.g., TSR, RSR) So, clearing the ISR after reading looks not
a bad idea.

Jongsung

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Sören Brinkmann]

On Mon, 2014-06-16 at 02:00PM +0900, Jongsung Kim wrote:
> On 06/13/2014 12:44 AM, Sören Brinkmann wrote:
> > Hi Jongsung,
> 
> Hi Sören,
> 
> > Does this interrupt need to be enabled? There is nothing checking
> > that bit and handling this IRQ in the handler, AFAICT. And you solve
> > this by simply clearing the bit. So, I wonder whether not enabling this
> > IRQ in the first place would solve things too.
> 
> The driver actually checks the bit, but does not clear it. Disabling the
> "Rx used bit read" interrupt you said may be a solution. However, I think
> it is the better way to clear the exceptional HW-state rather than just to
> mask it.
Hmm, I must have missed that.

> 
> > This is now clearing all IRQ flags which is probably not what we want
> > here. This is handling RX only. We still want the non-RX interrupts to go to
> > the actual interrupt service routing.
> 
> The ISR(Interrupt Status Register) is read only in the interrupt service
> routine, macb_interrupt. But is partially cleared here and there. Further
> handler-functions decide jobs to be done by reading/checking other status
> registers. (e.g., TSR, RSR) So, clearing the ISR after reading looks not
> a bad idea.
But you are clearing _all_ interrupt flags in the RX NAPI handler.
Doesn't that mean we might miss certain events?

	Sören

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Sören Brinkmann]

On Thu, 2014-06-12 at 05:50PM +0900, Jongsung Kim wrote:
> The "Rx used bit read" interrupt is enabled but not cleared for some
> systems with the ISR (Interrupt Status Register) configured as clear-
> on-write. This interrupt may be asserted when the CPU does not handle
> Rx-complete interrupts for a long time. (e.g., if the CPU is stopped
> by debugger) Once asserted, it'll not be cleared, and the CPU will
> loop infinitly in the interrupt handler.
> 
> This patch forces to use a dedicated function for reading the ISR,
> and the function clears it if clear-on-write. So the ISR is always
> cleared after read, regardless of clear-on-write configuration.
> 
> Reported-by: Hayun Hwang <hwang.hayun@lge.com>
> Signed-off-by: Youngkyu Choi <youngkyu7.choi@lge.com>
> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> Tested-by: Hayun Hwang <hwang.hayun@lge.com>
> ---
>  drivers/net/ethernet/cadence/macb.c |   37 ++++++++++++++--------------------
>  1 files changed, 15 insertions(+), 22 deletions(-)
> 
[...]
> @@ -552,9 +562,6 @@ static void macb_tx_interrupt(struct macb *bp)
>  	status = macb_readl(bp, TSR);
>  	macb_writel(bp, TSR, status);
>  
> -	if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> -		macb_writel(bp, ISR, MACB_BIT(TCOMP));
> -
>  	netdev_vdbg(bp->dev, "macb_tx_interrupt status = 0x%03lx\n",
>  		(unsigned long)status);
>  
> @@ -883,13 +890,10 @@ static int macb_poll(struct napi_struct *napi, int budget)
>  
>  		/* Packets received while interrupts were disabled */
>  		status = macb_readl(bp, RSR);
> -		if (status) {
> -			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> -				macb_writel(bp, ISR, MACB_BIT(RCOMP));
> +		if (status)
>  			napi_reschedule(napi);
> -		} else {
> +		else
>  			macb_writel(bp, IER, MACB_RX_INT_FLAGS);
> -		}
>  	}
>  
>  	/* TODO: Handle errors */
> @@ -903,7 +907,7 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>  	struct macb *bp = netdev_priv(dev);
>  	u32 status;
>  
> -	status = macb_readl(bp, ISR);
> +	status = macb_read_isr(bp);
>  
>  	if (unlikely(!status))
>  		return IRQ_NONE;
> @@ -928,8 +932,6 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>  			 * now.
>  			 */
>  			macb_writel(bp, IDR, MACB_RX_INT_FLAGS);
> -			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> -				macb_writel(bp, ISR, MACB_BIT(RCOMP));
Shouldn't it be sufficient to replace 'MACB_BIT(RCOMP) with 'MACB_RX_INT_FLAGS'
to clear all the RX IRQ flags.

	Sören

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Jongsung Kim]

On 06/16/2014 11:56 PM, Sören Brinkmann wrote:
> On Mon, 2014-06-16 at 02:00PM +0900, Jongsung Kim wrote:
>> On 06/13/2014 12:44 AM, Sören Brinkmann wrote:
>>> This is now clearing all IRQ flags which is probably not what we want
>>> here. This is handling RX only. We still want the non-RX interrupts to go to
>>> the actual interrupt service routing.
>>
>> The ISR(Interrupt Status Register) is read only in the interrupt service
>> routine, macb_interrupt. But is partially cleared here and there. Further
>> handler-functions decide jobs to be done by reading/checking other status
>> registers. (e.g., TSR, RSR) So, clearing the ISR after reading looks not
>> a bad idea.
>
> But you are clearing _all_ interrupt flags in the RX NAPI handler.
> Doesn't that mean we might miss certain events?

Please inspect my patch again. What I did in the macb_poll is removing
statements clearing the Rx-complete interrupt, not clearing all the
interrupts.

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Jongsung Kim]

On 06/17/2014 06:28 AM, Sören Brinkmann wrote:
> Shouldn't it be sufficient to replace 'MACB_BIT(RCOMP) with 'MACB_RX_INT_FLAGS'
> to clear all the RX IRQ flags.

I'm afraid not.

You know, this driver initially targeted only GEMs configured with "gem_irq_clear_read."
For this implementation of GEM, the ISR is automatically cleared by reading. The driver
was designed to operate with the value read from ISR, not with the ISR itself.

However, there are other GEMs configured without "gem_irq_clear_read," people like you
and I working with. To support them, they insert similar codes conditionally clearing
the ISR here and there. Now they are found at 6 places. Not enough yet. Do you want to
insert another at the end of macb_reset_hw..? Maybe not.

Jongsung

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Sören Brinkmann]

On Tue, 2014-06-17 at 11:38AM +0900, Jongsung Kim wrote:
> On 06/16/2014 11:56 PM, Sören Brinkmann wrote:
> > On Mon, 2014-06-16 at 02:00PM +0900, Jongsung Kim wrote:
> >> On 06/13/2014 12:44 AM, Sören Brinkmann wrote:
> >>> This is now clearing all IRQ flags which is probably not what we want
> >>> here. This is handling RX only. We still want the non-RX interrupts to go to
> >>> the actual interrupt service routing.
> >>
> >> The ISR(Interrupt Status Register) is read only in the interrupt service
> >> routine, macb_interrupt. But is partially cleared here and there. Further
> >> handler-functions decide jobs to be done by reading/checking other status
> >> registers. (e.g., TSR, RSR) So, clearing the ISR after reading looks not
> >> a bad idea.
> >
> > But you are clearing _all_ interrupt flags in the RX NAPI handler.
> > Doesn't that mean we might miss certain events?
> 
> Please inspect my patch again. What I did in the macb_poll is removing
> statements clearing the Rx-complete interrupt, not clearing all the
> interrupts.

Oh, you're right. I misread the patch, sorry. The call to macb_read_isr was
already a different hunk.
Why is clearing those bits removed? It's probably not a big hit, but it might
result in a pointless interrupt which could be avoided. But it should
probably clear all RX interrupts - MACB_RX_INT_FLAGS - instead of just RCOMP.
For clear-on-read implementations it shouldn't make a difference.

And in the if-condition in that new helper, I'd add '&& status' to
avoid writing back zeros.

	Thanks,
	Sören

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Jongsung Kim]

On 06/17/2014 12:50 PM, Sören Brinkmann wrote:
> On Tue, 2014-06-17 at 11:38AM +0900, Jongsung Kim wrote:
>> On 06/16/2014 11:56 PM, Sören Brinkmann wrote:
>>> On Mon, 2014-06-16 at 02:00PM +0900, Jongsung Kim wrote:
>>>> On 06/13/2014 12:44 AM, Sören Brinkmann wrote:
>>>>> This is now clearing all IRQ flags which is probably not what we want
>>>>> here. This is handling RX only. We still want the non-RX interrupts to go to
>>>>> the actual interrupt service routing.
>>>>
>>>> The ISR(Interrupt Status Register) is read only in the interrupt service
>>>> routine, macb_interrupt. But is partially cleared here and there. Further
>>>> handler-functions decide jobs to be done by reading/checking other status
>>>> registers. (e.g., TSR, RSR) So, clearing the ISR after reading looks not
>>>> a bad idea.
>>>
>>> But you are clearing _all_ interrupt flags in the RX NAPI handler.
>>> Doesn't that mean we might miss certain events?
>>
>> Please inspect my patch again. What I did in the macb_poll is removing
>> statements clearing the Rx-complete interrupt, not clearing all the
>> interrupts.
> 
> Why is clearing those bits removed? It's probably not a big hit, but it might
> result in a pointless interrupt which could be avoided. But it should
> probably clear all RX interrupts - MACB_RX_INT_FLAGS - instead of just RCOMP.
> For clear-on-read implementations it shouldn't make a difference.

I agree. But I removed it because I think stepping the same procedure
regardless of the "gem_irq_clear_read" implementation is better than
implementation-specific optimization.

> And in the if-condition in that new helper, I'd add '&& status' to
> avoid writing back zeros.

Good point. I'll add it when I resend v2.

Jongsung

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Nicolas Ferre]

On 17/06/2014 05:39, Jongsung Kim :
> On 06/17/2014 06:28 AM, Sören Brinkmann wrote:
>> Shouldn't it be sufficient to replace 'MACB_BIT(RCOMP) with 'MACB_RX_INT_FLAGS'
>> to clear all the RX IRQ flags.
> 
> I'm afraid not.
> 
> You know, this driver initially targeted only GEMs configured with "gem_irq_clear_read."
> For this implementation of GEM, the ISR is automatically cleared by reading. The driver
> was designed to operate with the value read from ISR, not with the ISR itself.
> 
> However, there are other GEMs configured without "gem_irq_clear_read," people like you
> and I working with. To support them, they insert similar codes conditionally clearing
> the ISR here and there. Now they are found at 6 places. Not enough yet. Do you want to
> insert another at the end of macb_reset_hw..? Maybe not.

Can't we separate a bit more the implementations of "clear on read" and
"clear on write" so that we do not spread the tests that you are talking
about all over the place and slower the driver's hot paths?

I am more and more skeptical about the mix of MACB/GEM versions in this
single driver as I realized recently that the old MACB-equipped devices
are behaving strangely and with lower performance figures than in the past.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

[Jongsung Kim]

On 06/17/2014 04:54 PM, Nicolas Ferre wrote:

Hi Nicolas,

> On 17/06/2014 05:39, Jongsung Kim :
>> On 06/17/2014 06:28 AM, Sören Brinkmann wrote:
>>> Shouldn't it be sufficient to replace 'MACB_BIT(RCOMP) with 'MACB_RX_INT_FLAGS'
>>> to clear all the RX IRQ flags.
>>
>> I'm afraid not.
>>
>> You know, this driver initially targeted only GEMs configured with "gem_irq_clear_read."
>> For this implementation of GEM, the ISR is automatically cleared by reading. The driver
>> was designed to operate with the value read from ISR, not with the ISR itself.
>>
>> However, there are other GEMs configured without "gem_irq_clear_read," people like you
>> and I working with. To support them, they insert similar codes conditionally clearing
>> the ISR here and there. Now they are found at 6 places. Not enough yet. Do you want to
>> insert another at the end of macb_reset_hw..? Maybe not.
> 
> Can't we separate a bit more the implementations of "clear on read" and
> "clear on write" so that we do not spread the tests that you are talking
> about all over the place and slower the driver's hot paths?

I see. I'll revise my patch. Thank you for your advice.

Regards,
Jongsung