netdev - Re: [PATCH net] net: phy: fix interrupt handling in non-started states

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]

Message-ID: <20190212193716.GD7527@lunn.ch>
Date:   Tue, 12 Feb 2019 20:37:16 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>
Subject: Re: [PATCH net] net: phy: fix interrupt handling in non-started
 states

On Tue, Feb 12, 2019 at 07:56:15PM +0100, Heiner Kallweit wrote:
> phylib enables interrupts before phy_start() has been called, and if
> we receive an interrupt in a non-started state, the interrupt handler
> returns IRQ_NONE. This causes problems with at least one Marvell chip
> as reported by Andrew.
> Fix this by handling interrupts the same as in phy_mac_interrupt(),
> basically always running the phylib state machine. It knows when it
> has to do something and when not.
> This change allows to handle interrupts gracefully even if they
> occur in a non-started state.
> 
> Fixes: 2b3e88ea6528 ("net: phy: improve phy state checking")
> Reported-by: Andrew Lunn <andrew@...n.ch>
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> ---
>  drivers/net/phy/phy.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 189cd2048c3a..ca5e0c0f018c 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -762,9 +762,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>  {
>  	struct phy_device *phydev = phy_dat;
>  
> -	if (!phy_is_started(phydev))
> -		return IRQ_NONE;		/* It can't be ours.  */
> -
>  	if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
>  		return IRQ_NONE;

Hi Heiner

Did you look at

ommit 3c3070d713d798f7f9e7ee3614e49b47655d14d8
Author: Maciej W. Rozycki <macro@...ux-mips.org>
Date:   Tue Oct 3 16:18:35 2006 +0100

    [PATCH] 2.6.18: sb1250-mac: Phylib IRQ handling fixes
    
     This patch fixes a couple of problems discovered with interrupt handling
    in the phylib core, namely:
    
    1. The driver uses timer and workqueue calls, but does not include
       <linux/timer.h> nor <linux/workqueue.h>.
    
    2. The driver uses schedule_work() for handling interrupts, but does not
       make sure any pending work scheduled thus has been completed before
       driver's structures get freed from memory.  This is especially
       important as interrupts may keep arriving if the line is shared with
       another PHY.
    
       The solution is to ignore phy_interrupt() calls if the reported device
       has already been halted and calling flush_scheduled_work() from
       phy_stop_interrupts() (but guarded with current_is_keventd() in case
       the function has been called through keventd from the MAC device's
       close call to avoid a deadlock on the netlink lock).
    
    Signed-off-by: Maciej W. Rozycki <macro@...ux-mips.org>
    
    patch-mips-2.6.18-20060920-phy-irq-16
    Signed-off-by: Jeff Garzik <jeff@...zik.org>

There has been a lot of change since then, so maybe this is no longer
an issue?

   Thanks
	Andrew