Re: [PATCH] net: dsa: mv88e6xxx: Fix port_hidden_wait to account for port_base_addr

Re: [PATCH] net: dsa: mv88e6xxx: Fix port_hidden_wait to account for port_base_addr

[Marek Behún]

On Sun, 24 Apr 2022 14:17:59 +0000
Nathan Rossi <nathan@nathanrossi.com> wrote:

> The other port_hidden functions rely on the port_read/port_write
> functions to access the hidden control port. These functions apply the
> offset for port_base_addr where applicable. Update port_hidden_wait to
> use the port_wait_bit so that port_base_addr offsets are accounted for
> when waiting for the busy bit to change.
> 
> Without the offset the port_hidden_wait function would timeout on
> devices that have a non-zero port_base_addr (e.g. MV88E6141), however
> devices that have a zero port_base_addr would operate correctly (e.g.
> MV88E6390).

So basically the code is accessing the wrong register for devices with
non-zero port_base_addr. This means that the patch should have a Fixes
tag with the commit that introduced this bug, so that it gets
backported to relevant stable versions.

Could you resend with Fixes tag?

Marek

Re: [PATCH] net: dsa: mv88e6xxx: Fix port_hidden_wait to account for port_base_addr

[Andrew Lunn]

On Sun, Apr 24, 2022 at 04:52:28PM +0200, Marek Behún wrote:
> On Sun, 24 Apr 2022 14:17:59 +0000
> Nathan Rossi <nathan@nathanrossi.com> wrote:
> 
> > The other port_hidden functions rely on the port_read/port_write
> > functions to access the hidden control port. These functions apply the
> > offset for port_base_addr where applicable. Update port_hidden_wait to
> > use the port_wait_bit so that port_base_addr offsets are accounted for
> > when waiting for the busy bit to change.
> > 
> > Without the offset the port_hidden_wait function would timeout on
> > devices that have a non-zero port_base_addr (e.g. MV88E6141), however
> > devices that have a zero port_base_addr would operate correctly (e.g.
> > MV88E6390).
> 
> So basically the code is accessing the wrong register for devices with
> non-zero port_base_addr. This means that the patch should have a Fixes
> tag with the commit that introduced this bug, so that it gets
> backported to relevant stable versions.
> 
> Could you resend with Fixes tag?

I think that would be:

Fixes: 609070133aff ("net: dsa: mv88e6xxx: update code operating on hidden registers")

That patch moved the code out of chip.c into port_hidden. At the same
time, the functions got renamed from mv88e6390_hidden_read() to
mv886xxx_hidden_read(). 6390 does have zero-based ports, so was
correct before the rename. But the more generic name suggest it should
take into account devices which do not have zero based ports.

With the fixes tag added and based on met:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew