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

[Thomas Bogendoerfer]

On Fri, 25 Jan 2019 09:37:20 +0100
Antoine Tenart <antoine.tenart@xxxxxxxxxxx> 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@xxxxxxx>
> > > > ---
> > > >  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)