| 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 |
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>
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
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
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
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 --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,
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