netdev - Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt

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: <20190125161815.0527d5174fdfb680eee59372@suse.de>
Date:   Fri, 25 Jan 2019 16:18:15 +0100
From:   Thomas Bogendoerfer <tbogendoerfer@...e.de>
To:     Antoine Tenart <antoine.tenart@...tlin.com>
Cc:     Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>, maxime.chevallier@...tlin.com,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: mvpp2: fix condition for setting up link
 interrupt

On Fri, 25 Jan 2019 09:37:20 +0100
Antoine Tenart <antoine.tenart@...tlin.com> wrote:

> Hi,
> 
> On Thu, Jan 24, 2019 at 04:07:41PM +0000, Russell King - ARM Linux admin wrote:
> > On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
> > > On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> > > > 
> > > > Fixes: 4bb043262878 ("net: mvpp2: phylink support")
> > > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@...e.de>
> > > > ---
> > > >  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > index 16066c2d5b3a..0fbfe1945a69 100644
> > > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > @@ -3405,7 +3405,7 @@ static int mvpp2_open(struct net_device *dev)
> > > >  		valid = true;
> > > >  	}
> > > >  
> > > > -	if (priv->hw_version == MVPP22 && port->link_irq && !port->phylink) {
> > > > +	if (priv->hw_version == MVPP22 && port->link_irq && port->phylink) {
> > > >  		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
> > > >  				  dev->name, port);
> > > >  		if (err) {
> > 
> > This still looks fishy to me.  mvpp2_link_status_isr() has handling in
> > it that is safe to be called for non-phylink cases, so presumably the
> > right fix is to drop the "&& !port->phylink" completely?
> 
> That's right, mvpp2_link_status_isr() is safe to be called with or
> without phylink being used. This IRQ is reporting the link status, and
> is used when using in-band status or when phylink isn't used.
> 
> We do have a similar fix locally, which looks like:
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 2bcbf9caaf0d..6bab1824a1e4 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -3406,7 +3406,8 @@ static int mvpp2_open(struct net_device *dev)
>                 valid = true;
>         }
> 
> -       if (priv->hw_version == MVPP22 && port->link_irq && !port->phylink) {
> +       if (priv->hw_version == MVPP22 && port->link_irq &&
> +           (!port->phylink || !port->has_phy)) {
>                 err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
>                                   dev->name, port);
>                 if (err) {
> 
> We haven't submitted it yet, as we saw several issues when a port has no
> PHY and is using the XLG MAC (so, not on the mcbin). We currently are
> working on them.
> 
> I don't like to ask this, as you submitted the fix first, but I do think
> we should hold back a bit while we figure out proper solutions. If you
> need in-band status to be working properly, we could share out current
> local branch so you can test and validate it do solve your issue.

I also thought about it, and your patch makes more sense. It's more about
having no ethernet phy and therefore looking at the serdes for link status.
The system I'm running on has a few internal nodes behind a switch. The
switch propagates the external link state to the nodes via serdes lines.

I've tested your patch and it works in my setup.

Thomas.

-- 
SUSE Linux GmbH
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)