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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 12 Jun 2014 08:44:38 -0700
From:	Sören Brinkmann <soren.brinkmann@...inx.com>
To:	Jongsung Kim <neidhard.kim@....com>
CC:	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	"David S. Miller" <davem@...emloft.net>,
	Hayun Hwang <hwang.hayun@....com>,
	Youngkyu Choi <youngkyu7.choi@....com>
Subject: Re: [PATCH] net/cadence/macb: clear interrupts simply and correctly

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@....com>
> Signed-off-by: Youngkyu Choi <youngkyu7.choi@....com>
> Signed-off-by: Jongsung Kim <neidhard.kim@....com>
> Tested-by: Hayun Hwang <hwang.hayun@....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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html