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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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)

Powered by blists - more mailing lists