diff mbox series

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

Message ID 20220424153143.323338-1-nathan@nathanrossi.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v2] net: dsa: mv88e6xxx: Fix port_hidden_wait to account for port_base_addr | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 11 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Nathan Rossi April 24, 2022, 3:31 p.m. UTC
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(-)

---
2.35.2

Comments

Marek Behún April 24, 2022, 5:20 p.m. UTC | #1
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>
Andrew Lunn April 24, 2022, 7:26 p.m. UTC | #2
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
Marek Behún April 24, 2022, 7:33 p.m. UTC | #3
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
Andrew Lunn April 24, 2022, 10:33 p.m. UTC | #4
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
Marek Behún April 24, 2022, 11:16 p.m. UTC | #5
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
diff mbox series

Patch

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,