* [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time @ 2017-03-13 19:20 Vivien Didelot 2017-03-13 22:39 ` Andrew Lunn 2017-03-28 18:13 ` Vivien Didelot 0 siblings, 2 replies; 10+ messages in thread From: Vivien Didelot @ 2017-03-13 19:20 UTC (permalink / raw) To: netdev Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli, Andrew Lunn, Jason Cobham, Vivien Didelot The ATU ageing time value programmed in the switch is rounded up to the nearest multiple of its coefficient (variable depending on the model.) Add a debug message to inform the user about the exact programmed value. On 6352, "brctl setageing br0 18" gives "AgeTime set to 0x01 (15000 ms)" while on 6390 we get "AgeTime set to 0x05 (18750 ms)". Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- drivers/net/dsa/mv88e6xxx/global1_atu.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c index f6cd3c939da4..bac34737b096 100644 --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c @@ -65,7 +65,14 @@ int mv88e6xxx_g1_atu_set_age_time(struct mv88e6xxx_chip *chip, val &= ~0xff0; val |= age_time << 4; - return mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); + err = mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); + if (err) + return err; + + dev_dbg(chip->dev, "AgeTime set to 0x%02x (%d ms)\n", age_time, + age_time * coeff); + + return 0; } /* Offset 0x0B: ATU Operation Register */ -- 2.12.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time 2017-03-13 19:20 [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time Vivien Didelot @ 2017-03-13 22:39 ` Andrew Lunn 2017-03-13 22:42 ` Florian Fainelli 2017-03-28 18:13 ` Vivien Didelot 1 sibling, 1 reply; 10+ messages in thread From: Andrew Lunn @ 2017-03-13 22:39 UTC (permalink / raw) To: Vivien Didelot Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli, Jason Cobham On Mon, Mar 13, 2017 at 03:20:43PM -0400, Vivien Didelot wrote: > The ATU ageing time value programmed in the switch is rounded up to the > nearest multiple of its coefficient (variable depending on the model.) > > Add a debug message to inform the user about the exact programmed value. > > On 6352, "brctl setageing br0 18" gives "AgeTime set to 0x01 (15000 ms)" > while on 6390 we get "AgeTime set to 0x05 (18750 ms)". > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > --- > drivers/net/dsa/mv88e6xxx/global1_atu.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c > index f6cd3c939da4..bac34737b096 100644 > --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c > +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c > @@ -65,7 +65,14 @@ int mv88e6xxx_g1_atu_set_age_time(struct mv88e6xxx_chip *chip, > val &= ~0xff0; > val |= age_time << 4; > > - return mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); > + err = mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); > + if (err) > + return err; > + > + dev_dbg(chip->dev, "AgeTime set to 0x%02x (%d ms)\n", age_time, > + age_time * coeff); > + Hi Vivien You could put the dev_dbg before the mv88e6xxx_g1_write(), to keep the code simpler. If this write fails, we expect a lot of other things to go horribly wrong, so having one debug message being not quite accurate is not important. Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time 2017-03-13 22:39 ` Andrew Lunn @ 2017-03-13 22:42 ` Florian Fainelli 2017-03-13 22:58 ` Andrew Lunn 0 siblings, 1 reply; 10+ messages in thread From: Florian Fainelli @ 2017-03-13 22:42 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot Cc: netdev, linux-kernel, kernel, David S. Miller, Jason Cobham On 03/13/2017 03:39 PM, Andrew Lunn wrote: > On Mon, Mar 13, 2017 at 03:20:43PM -0400, Vivien Didelot wrote: >> The ATU ageing time value programmed in the switch is rounded up to the >> nearest multiple of its coefficient (variable depending on the model.) >> >> Add a debug message to inform the user about the exact programmed value. >> >> On 6352, "brctl setageing br0 18" gives "AgeTime set to 0x01 (15000 ms)" >> while on 6390 we get "AgeTime set to 0x05 (18750 ms)". >> >> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> >> --- >> drivers/net/dsa/mv88e6xxx/global1_atu.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c >> index f6cd3c939da4..bac34737b096 100644 >> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c >> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c >> @@ -65,7 +65,14 @@ int mv88e6xxx_g1_atu_set_age_time(struct mv88e6xxx_chip *chip, >> val &= ~0xff0; >> val |= age_time << 4; >> >> - return mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); >> + err = mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); >> + if (err) >> + return err; >> + >> + dev_dbg(chip->dev, "AgeTime set to 0x%02x (%d ms)\n", age_time, >> + age_time * coeff); >> + > > Hi Vivien > > You could put the dev_dbg before the mv88e6xxx_g1_write(), to keep the > code simpler. If this write fails, we expect a lot of other things to > go horribly wrong, so having one debug message being not quite accurate > is not important. The debug message would not be printed in case mv88e6xxx_g1_write() fails, also, having the message printed after the write occurred is a good way to make sure the write did make it through. Did I miss something in what you are suggesting here? -- Florian ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time 2017-03-13 22:42 ` Florian Fainelli @ 2017-03-13 22:58 ` Andrew Lunn 2017-03-14 11:12 ` Matthias May 0 siblings, 1 reply; 10+ messages in thread From: Andrew Lunn @ 2017-03-13 22:58 UTC (permalink / raw) To: Florian Fainelli Cc: Vivien Didelot, netdev, linux-kernel, kernel, David S. Miller, Jason Cobham On Mon, Mar 13, 2017 at 03:42:36PM -0700, Florian Fainelli wrote: > On 03/13/2017 03:39 PM, Andrew Lunn wrote: > > On Mon, Mar 13, 2017 at 03:20:43PM -0400, Vivien Didelot wrote: > >> The ATU ageing time value programmed in the switch is rounded up to the > >> nearest multiple of its coefficient (variable depending on the model.) > >> > >> Add a debug message to inform the user about the exact programmed value. > >> > >> On 6352, "brctl setageing br0 18" gives "AgeTime set to 0x01 (15000 ms)" > >> while on 6390 we get "AgeTime set to 0x05 (18750 ms)". > >> > >> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > >> --- > >> drivers/net/dsa/mv88e6xxx/global1_atu.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c > >> index f6cd3c939da4..bac34737b096 100644 > >> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c > >> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c > >> @@ -65,7 +65,14 @@ int mv88e6xxx_g1_atu_set_age_time(struct mv88e6xxx_chip *chip, > >> val &= ~0xff0; > >> val |= age_time << 4; > >> > >> - return mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); > >> + err = mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); > >> + if (err) > >> + return err; > >> + > >> + dev_dbg(chip->dev, "AgeTime set to 0x%02x (%d ms)\n", age_time, > >> + age_time * coeff); > >> + > > > > Hi Vivien > > > > You could put the dev_dbg before the mv88e6xxx_g1_write(), to keep the > > code simpler. If this write fails, we expect a lot of other things to > > go horribly wrong, so having one debug message being not quite accurate > > is not important. > > The debug message would not be printed in case mv88e6xxx_g1_write() > fails, also, having the message printed after the write occurred is a > good way to make sure the write did make it through. Did I miss > something in what you are suggesting here? We never, ever see a read or a write failure on the MDIO bus. If it ever does, i expect the switch is dead, gone, never to be heard from again until the power is reset. We are going to have lots of failures. So it seems simpler to have: dev_dbg(chip->dev, "Setting AgeTime to 0x%02x (%d ms)\n", age_time, age_time * coeff); return mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); and accept that if for some unlikely reason the write does fail, the debug message is probably not accurate. Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time 2017-03-13 22:58 ` Andrew Lunn @ 2017-03-14 11:12 ` Matthias May 2017-03-14 12:05 ` Andrew Lunn 0 siblings, 1 reply; 10+ messages in thread From: Matthias May @ 2017-03-14 11:12 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli Cc: Vivien Didelot, netdev, linux-kernel, kernel, David S. Miller, Jason Cobham On 13/03/17 23:58, Andrew Lunn wrote: > On Mon, Mar 13, 2017 at 03:42:36PM -0700, Florian Fainelli wrote: >> On 03/13/2017 03:39 PM, Andrew Lunn wrote: >>> On Mon, Mar 13, 2017 at 03:20:43PM -0400, Vivien Didelot wrote: >>>> The ATU ageing time value programmed in the switch is rounded up to the >>>> nearest multiple of its coefficient (variable depending on the model.) >>>> >>>> Add a debug message to inform the user about the exact programmed value. >>>> >>>> On 6352, "brctl setageing br0 18" gives "AgeTime set to 0x01 (15000 ms)" >>>> while on 6390 we get "AgeTime set to 0x05 (18750 ms)". >>>> >>>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> >>>> --- >>>> drivers/net/dsa/mv88e6xxx/global1_atu.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c >>>> index f6cd3c939da4..bac34737b096 100644 >>>> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c >>>> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c >>>> @@ -65,7 +65,14 @@ int mv88e6xxx_g1_atu_set_age_time(struct mv88e6xxx_chip *chip, >>>> val &= ~0xff0; >>>> val |= age_time << 4; >>>> >>>> - return mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); >>>> + err = mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); >>>> + if (err) >>>> + return err; >>>> + >>>> + dev_dbg(chip->dev, "AgeTime set to 0x%02x (%d ms)\n", age_time, >>>> + age_time * coeff); >>>> + >>> >>> Hi Vivien >>> >>> You could put the dev_dbg before the mv88e6xxx_g1_write(), to keep the >>> code simpler. If this write fails, we expect a lot of other things to >>> go horribly wrong, so having one debug message being not quite accurate >>> is not important. >> >> The debug message would not be printed in case mv88e6xxx_g1_write() >> fails, also, having the message printed after the write occurred is a >> good way to make sure the write did make it through. Did I miss >> something in what you are suggesting here? > > We never, ever see a read or a write failure on the MDIO bus. If it > ever does, i expect the switch is dead, gone, never to be heard from > again until the power is reset. We are going to have lots of > failures. So it seems simpler to have: > > dev_dbg(chip->dev, "Setting AgeTime to 0x%02x (%d ms)\n", age_time, > age_time * coeff); > > return mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val); > > and accept that if for some unlikely reason the write does fail, the > debug message is probably not accurate. > > Andrew > Hi The never ever seeing R/W failure on MDIO bus is not exactly accurate. We had with art (atheros calibration tool) the problem that interrupts were being disabled which lead to MDIO operations running into timout/failing. For normal phys this usually results in calling phy_error in .../net/phy/phy.c which puts the phy into a defined state (PHY_HALTED). Granted this is a problem produced by art2 but couldn't the same be applied here? Put the device in a defined state? BR Matthias ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time 2017-03-14 11:12 ` Matthias May @ 2017-03-14 12:05 ` Andrew Lunn 2017-03-14 13:56 ` Vivien Didelot 0 siblings, 1 reply; 10+ messages in thread From: Andrew Lunn @ 2017-03-14 12:05 UTC (permalink / raw) To: Matthias May Cc: Florian Fainelli, Vivien Didelot, netdev, linux-kernel, kernel, David S. Miller, Jason Cobham > Hi > The never ever seeing R/W failure on MDIO bus is not exactly accurate. > We had with art (atheros calibration tool) the problem that interrupts > were being disabled which lead to MDIO operations running into > timout/failing. Yes, i've seen similar with power management bugs for the MDIO driver. But you get a cascade of failures, lots of warnings and error prints, it is clear something bad has happened, and the switch is in an inconsistent state. So having one more debug print which is also inconsistent does no really harm. Anyway, this whole conversation has taken more effort than just making this simple change to remove a few lines of code. So lets drop it and move on. Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time 2017-03-14 12:05 ` Andrew Lunn @ 2017-03-14 13:56 ` Vivien Didelot 2017-03-14 14:18 ` Andrew Lunn 0 siblings, 1 reply; 10+ messages in thread From: Vivien Didelot @ 2017-03-14 13:56 UTC (permalink / raw) To: Andrew Lunn, Matthias May Cc: Florian Fainelli, netdev, linux-kernel, kernel, David S. Miller, Jason Cobham Hi Andrew, Andrew Lunn <andrew@lunn.ch> writes: >> The never ever seeing R/W failure on MDIO bus is not exactly accurate. >> We had with art (atheros calibration tool) the problem that interrupts >> were being disabled which lead to MDIO operations running into >> timout/failing. > > Yes, i've seen similar with power management bugs for the MDIO > driver. But you get a cascade of failures, lots of warnings and error > prints, it is clear something bad has happened, and the switch is in > an inconsistent state. So having one more debug print which is also > inconsistent does no really harm. > > Anyway, this whole conversation has taken more effort than just making > this simple change to remove a few lines of code. So lets drop it and > move on. I don't understand nor agree with the fact that sometimes it's OK to not check for errors, based on one developer assumptions. Not checking return code is wrong and very likely error-prone. If you really want to stand for that point, please send a patch series which turns mv88e6xxx_read() and mv88e6xxx_write() into void functions. I'd be glad to review and discuss this further. That would indeed make *all* the driver code simpler. Thanks, Vivien ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time 2017-03-14 13:56 ` Vivien Didelot @ 2017-03-14 14:18 ` Andrew Lunn 0 siblings, 0 replies; 10+ messages in thread From: Andrew Lunn @ 2017-03-14 14:18 UTC (permalink / raw) To: Vivien Didelot Cc: Matthias May, Florian Fainelli, netdev, linux-kernel, kernel, David S. Miller, Jason Cobham On Tue, Mar 14, 2017 at 09:56:41AM -0400, Vivien Didelot wrote: > Hi Andrew, > > Andrew Lunn <andrew@lunn.ch> writes: > > >> The never ever seeing R/W failure on MDIO bus is not exactly accurate. > >> We had with art (atheros calibration tool) the problem that interrupts > >> were being disabled which lead to MDIO operations running into > >> timout/failing. > > > > Yes, i've seen similar with power management bugs for the MDIO > > driver. But you get a cascade of failures, lots of warnings and error > > prints, it is clear something bad has happened, and the switch is in > > an inconsistent state. So having one more debug print which is also > > inconsistent does no really harm. > > > > Anyway, this whole conversation has taken more effort than just making > > this simple change to remove a few lines of code. So lets drop it and > > move on. > > I don't understand nor agree with the fact that sometimes it's OK to not > check for errors, based on one developer assumptions. Not checking > return code is wrong and very likely error-prone. Please go back and look what i said. I did check the error code, in that it gets returned to the caller. I just don't check it before printing the debug. But as i said, lets drop this whole topic. Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time 2017-03-13 19:20 [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time Vivien Didelot 2017-03-13 22:39 ` Andrew Lunn @ 2017-03-28 18:13 ` Vivien Didelot 2017-03-29 4:55 ` David Miller 1 sibling, 1 reply; 10+ messages in thread From: Vivien Didelot @ 2017-03-28 18:13 UTC (permalink / raw) To: netdev Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli, Andrew Lunn, Jason Cobham Hi David, Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes: > The ATU ageing time value programmed in the switch is rounded up to the > nearest multiple of its coefficient (variable depending on the model.) > > Add a debug message to inform the user about the exact programmed value. > > On 6352, "brctl setageing br0 18" gives "AgeTime set to 0x01 (15000 ms)" > while on 6390 we get "AgeTime set to 0x05 (18750 ms)". > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Can you pick this patch? Thanks, Vivien ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time 2017-03-28 18:13 ` Vivien Didelot @ 2017-03-29 4:55 ` David Miller 0 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2017-03-29 4:55 UTC (permalink / raw) To: vivien.didelot; +Cc: netdev, linux-kernel, kernel, f.fainelli, andrew, jcobham From: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Date: Tue, 28 Mar 2017 14:13:53 -0400 > Hi David, > > Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes: > >> The ATU ageing time value programmed in the switch is rounded up to the >> nearest multiple of its coefficient (variable depending on the model.) >> >> Add a debug message to inform the user about the exact programmed value. >> >> On 6352, "brctl setageing br0 18" gives "AgeTime set to 0x01 (15000 ms)" >> while on 6390 we get "AgeTime set to 0x05 (18750 ms)". >> >> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > > Can you pick this patch? If it's not in a pending state in patchwork, there must be a reason. And that reason will tell you why I didn't apply it, and what needs to be resolved in order to change that. In any event, you have to at a minimum resubmit the patch. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-29 4:56 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-13 19:20 [PATCH net-next] net: dsa: mv88e6xxx: debug ATU Age Time Vivien Didelot 2017-03-13 22:39 ` Andrew Lunn 2017-03-13 22:42 ` Florian Fainelli 2017-03-13 22:58 ` Andrew Lunn 2017-03-14 11:12 ` Matthias May 2017-03-14 12:05 ` Andrew Lunn 2017-03-14 13:56 ` Vivien Didelot 2017-03-14 14:18 ` Andrew Lunn 2017-03-28 18:13 ` Vivien Didelot 2017-03-29 4:55 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).