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

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

[Nathan Rossi]

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).

Fixes: ea89098ef9a5 ("net: dsa: mv88x6xxx: mv88e6390 errata")
Signed-off-by: Nathan Rossi <nathan@nathanrossi.com>
---
Changes in v2:
- Add fixes
---
 drivers/net/dsa/mv88e6xxx/port_hidden.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/port_hidden.c b/drivers/net/dsa/mv88e6xxx/port_hidden.c
index b49d05f0e1..7a9f9ff6de 100644
--- a/drivers/net/dsa/mv88e6xxx/port_hidden.c
+++ b/drivers/net/dsa/mv88e6xxx/port_hidden.c
@@ -40,8 +40,9 @@ int mv88e6xxx_port_hidden_wait(struct mv88e6xxx_chip *chip)
 {
 	int bit = __bf_shf(MV88E6XXX_PORT_RESERVED_1A_BUSY);
 
-	return mv88e6xxx_wait_bit(chip, MV88E6XXX_PORT_RESERVED_1A_CTRL_PORT,
-				  MV88E6XXX_PORT_RESERVED_1A, bit, 0);
+	return mv88e6xxx_port_wait_bit(chip,
+				       MV88E6XXX_PORT_RESERVED_1A_CTRL_PORT,
+				       MV88E6XXX_PORT_RESERVED_1A, bit, 0);
 }
 
 int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int block, int port,
---
2.35.2

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

[Marek Behún]

On Sun, 24 Apr 2022 15:31:43 +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).
> 
> Fixes: ea89098ef9a5 ("net: dsa: mv88x6xxx: mv88e6390 errata")
> Signed-off-by: Nathan Rossi <nathan@nathanrossi.com>
> ---
> Changes in v2:
> - Add fixes
> ---
>  drivers/net/dsa/mv88e6xxx/port_hidden.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/port_hidden.c b/drivers/net/dsa/mv88e6xxx/port_hidden.c
> index b49d05f0e1..7a9f9ff6de 100644
> --- a/drivers/net/dsa/mv88e6xxx/port_hidden.c
> +++ b/drivers/net/dsa/mv88e6xxx/port_hidden.c
> @@ -40,8 +40,9 @@ int mv88e6xxx_port_hidden_wait(struct mv88e6xxx_chip *chip)
>  {
>  	int bit = __bf_shf(MV88E6XXX_PORT_RESERVED_1A_BUSY);
>  
> -	return mv88e6xxx_wait_bit(chip, MV88E6XXX_PORT_RESERVED_1A_CTRL_PORT,
> -				  MV88E6XXX_PORT_RESERVED_1A, bit, 0);
> +	return mv88e6xxx_port_wait_bit(chip,
> +				       MV88E6XXX_PORT_RESERVED_1A_CTRL_PORT,
> +				       MV88E6XXX_PORT_RESERVED_1A, bit, 0);
>  }
>  
>  int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int block, int port,

Reviewed-by: Marek Behún <kabel@kernel.org>

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

[Andrew Lunn]

On Sun, Apr 24, 2022 at 03:31:43PM +0000, Nathan Rossi 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).
> 
> Fixes: ea89098ef9a5 ("net: dsa: mv88x6xxx: mv88e6390 errata")

That is further back than needed. And due to the code moving around
and getting renamed, you are added extra burden on those doing the
back port for no actual gain.

Please verify what i suggested, 609070133aff1 is better and then
repost.

	Thanks
		Andrew

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

[Marek Behún]

On Sun, 24 Apr 2022 21:26:58 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Sun, Apr 24, 2022 at 03:31:43PM +0000, Nathan Rossi 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).
> > 
> > Fixes: ea89098ef9a5 ("net: dsa: mv88x6xxx: mv88e6390 errata")  
> 
> That is further back than needed. And due to the code moving around
> and getting renamed, you are added extra burden on those doing the
> back port for no actual gain.
> 
> Please verify what i suggested, 609070133aff1 is better and then
> repost.

The bug was introduced by ea89098ef9a5.
609070133aff1 is only requirement for this fix, but Fixes tag should reference
the commit which introduced the bug, afaik.

So it should be 

Fixes: ea89098ef9a5 ("net: dsa: mv88x6xxx: mv88e6390 errata")
Cc: stable@vger.kernel.org # 609070133aff ("net: dsa: mv88e6xxx: update code operating on hidden registers")

Marek

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

[Andrew Lunn]

On Sun, Apr 24, 2022 at 09:33:59PM +0200, Marek Behún wrote:
> On Sun, 24 Apr 2022 21:26:58 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Sun, Apr 24, 2022 at 03:31:43PM +0000, Nathan Rossi 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).
> > > 
> > > Fixes: ea89098ef9a5 ("net: dsa: mv88x6xxx: mv88e6390 errata")  
> > 
> > That is further back than needed. And due to the code moving around
> > and getting renamed, you are added extra burden on those doing the
> > back port for no actual gain.
> > 
> > Please verify what i suggested, 609070133aff1 is better and then
> > repost.
> 
> The bug was introduced by ea89098ef9a5.

I have to disagree with that. ea89098ef9a5 adds:

mv88e6390_hidden_wait()

The mv88e6390_ means it should be used with the mv88e6390 family. And
all members of that family have port offset 0. There is no bug here.

609070133aff1 renames it to mv88e6xxx_port_hidden_wait(). It now has
the generic mv88e6xxx_ prefix, so we can expect it to work with any
device. But it does not. This is where the bug has introduced.

But what i think is more important, is i doubt git cherry-pick is
clever enough to be able to follow 609070133aff1 and know where to
make the change in revisions before then. So it is going to need a
human to figure out the backport. And that effort is a waist of time,
because there is no bug before then.

	Andrew

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

[Marek Behún]

On Mon, 25 Apr 2022 00:33:15 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Sun, Apr 24, 2022 at 09:33:59PM +0200, Marek Behún wrote:
> > On Sun, 24 Apr 2022 21:26:58 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> >   
> > > On Sun, Apr 24, 2022 at 03:31:43PM +0000, Nathan Rossi 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).
> > > > 
> > > > Fixes: ea89098ef9a5 ("net: dsa: mv88x6xxx: mv88e6390 errata")    
> > > 
> > > That is further back than needed. And due to the code moving around
> > > and getting renamed, you are added extra burden on those doing the
> > > back port for no actual gain.
> > > 
> > > Please verify what i suggested, 609070133aff1 is better and then
> > > repost.  
> > 
> > The bug was introduced by ea89098ef9a5.  
> 
> I have to disagree with that. ea89098ef9a5 adds:
> 
> mv88e6390_hidden_wait()
> 
> The mv88e6390_ means it should be used with the mv88e6390 family. And
> all members of that family have port offset 0. There is no bug here.
> 
> 609070133aff1 renames it to mv88e6xxx_port_hidden_wait(). It now has
> the generic mv88e6xxx_ prefix, so we can expect it to work with any
> device. But it does not. This is where the bug has introduced.

You are right. My bad, sorry.

Marek