| Message ID | 95edb9b6-d779-a1b9-77cd-7e86cb41f73d@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: dsa: mv88e6xxx: fix few issues in mv88e6390x_port_set_cmode | expand |
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Sat, 23 Mar 2019 19:41:32 +0100 > This patches fixes few issues in mv88e6390x_port_set_cmode(). > > 1. When entering the function the old cmode may be 0, in this case > mv88e6390x_serdes_get_lane() returns -ENODEV. As result we bail > out and have no chance to set a new mode. Therefore deal properly > with -ENODEV. > > 2. Once we have disabled power and irq, let's set the cached cmode to 0. > This reflects the actual status and is cleaner if we bail out with an > error in the following function calls. > > 3. The cached cmode is used by mv88e6390x_serdes_get_lane(), > mv88e6390_serdes_power_lane() and mv88e6390_serdes_irq_enable(). > Currently we set the cached mode to the new one at the very end of > the function only, means until then we use the old one what may be > wrong. > > 4. When calling mv88e6390_serdes_irq_enable() we use the lane value > belonging to the old cmode. Get the lane belonging to the new cmode > before calling this function. > > It's hard to provide a good "Fixes" tag because quite a few smaller > changes have been done to the code in question recently. > > Fixes: d235c48b40d3 ("net: dsa: mv88e6xxx: power serdes on/off for 10G interfaces on 6390X") > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Andrew et al., please review. Thank you.
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Sat, 23 Mar 2019 19:41:32 +0100 > This patches fixes few issues in mv88e6390x_port_set_cmode(). > > 1. When entering the function the old cmode may be 0, in this case > mv88e6390x_serdes_get_lane() returns -ENODEV. As result we bail > out and have no chance to set a new mode. Therefore deal properly > with -ENODEV. > > 2. Once we have disabled power and irq, let's set the cached cmode to 0. > This reflects the actual status and is cleaner if we bail out with an > error in the following function calls. > > 3. The cached cmode is used by mv88e6390x_serdes_get_lane(), > mv88e6390_serdes_power_lane() and mv88e6390_serdes_irq_enable(). > Currently we set the cached mode to the new one at the very end of > the function only, means until then we use the old one what may be > wrong. > > 4. When calling mv88e6390_serdes_irq_enable() we use the lane value > belonging to the old cmode. Get the lane belonging to the new cmode > before calling this function. > > It's hard to provide a good "Fixes" tag because quite a few smaller > changes have been done to the code in question recently. > > Fixes: d235c48b40d3 ("net: dsa: mv88e6xxx: power serdes on/off for 10G interfaces on 6390X") > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Once again, Andrew, Florian, Vivien, et al., please review.
On 3/23/19 11:41 AM, Heiner Kallweit wrote: > This patches fixes few issues in mv88e6390x_port_set_cmode(). > > 1. When entering the function the old cmode may be 0, in this case > mv88e6390x_serdes_get_lane() returns -ENODEV. As result we bail > out and have no chance to set a new mode. Therefore deal properly > with -ENODEV. > > 2. Once we have disabled power and irq, let's set the cached cmode to 0. > This reflects the actual status and is cleaner if we bail out with an > error in the following function calls. > > 3. The cached cmode is used by mv88e6390x_serdes_get_lane(), > mv88e6390_serdes_power_lane() and mv88e6390_serdes_irq_enable(). > Currently we set the cached mode to the new one at the very end of > the function only, means until then we use the old one what may be > wrong. > > 4. When calling mv88e6390_serdes_irq_enable() we use the lane value > belonging to the old cmode. Get the lane belonging to the new cmode > before calling this function. > > It's hard to provide a good "Fixes" tag because quite a few smaller > changes have been done to the code in question recently. > > Fixes: d235c48b40d3 ("net: dsa: mv88e6xxx: power serdes on/off for 10G interfaces on 6390X") > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/dsa/mv88e6xxx/port.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c > index dce84a2a6..c44b2822e 100644 > --- a/drivers/net/dsa/mv88e6xxx/port.c > +++ b/drivers/net/dsa/mv88e6xxx/port.c > @@ -427,18 +427,22 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, > return 0; > > lane = mv88e6390x_serdes_get_lane(chip, port); > - if (lane < 0) > + if (lane < 0 && lane != -ENODEV) > return lane; It does not look like you can return a negative error code from mv88e6390x_serdes_get_lane() that is not -ENODEV? Other than that: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On 27.03.2019 22:22, Florian Fainelli wrote: > On 3/23/19 11:41 AM, Heiner Kallweit wrote: >> This patches fixes few issues in mv88e6390x_port_set_cmode(). >> >> 1. When entering the function the old cmode may be 0, in this case >> mv88e6390x_serdes_get_lane() returns -ENODEV. As result we bail >> out and have no chance to set a new mode. Therefore deal properly >> with -ENODEV. >> >> 2. Once we have disabled power and irq, let's set the cached cmode to 0. >> This reflects the actual status and is cleaner if we bail out with an >> error in the following function calls. >> >> 3. The cached cmode is used by mv88e6390x_serdes_get_lane(), >> mv88e6390_serdes_power_lane() and mv88e6390_serdes_irq_enable(). >> Currently we set the cached mode to the new one at the very end of >> the function only, means until then we use the old one what may be >> wrong. >> >> 4. When calling mv88e6390_serdes_irq_enable() we use the lane value >> belonging to the old cmode. Get the lane belonging to the new cmode >> before calling this function. >> >> It's hard to provide a good "Fixes" tag because quite a few smaller >> changes have been done to the code in question recently. >> >> Fixes: d235c48b40d3 ("net: dsa: mv88e6xxx: power serdes on/off for 10G interfaces on 6390X") >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/dsa/mv88e6xxx/port.c | 24 ++++++++++++++++-------- >> 1 file changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c >> index dce84a2a6..c44b2822e 100644 >> --- a/drivers/net/dsa/mv88e6xxx/port.c >> +++ b/drivers/net/dsa/mv88e6xxx/port.c >> @@ -427,18 +427,22 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, >> return 0; >> >> lane = mv88e6390x_serdes_get_lane(chip, port); >> - if (lane < 0) >> + if (lane < 0 && lane != -ENODEV) >> return lane; > > It does not look like you can return a negative error code from > mv88e6390x_serdes_get_lane() that is not -ENODEV? > Thanks for reviewing, Florian. Right, at least for now mv88e6390x_serdes_get_lane() doesn't return any other errno. But that's not carved in stone, and if somebody comes and adds a check that returns -EINVAL to this function, then this person most likely won't update mv88e6390x_port_set_cmode(). > Other than that: > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Sat, 23 Mar 2019 19:41:32 +0100 > This patches fixes few issues in mv88e6390x_port_set_cmode(). > > 1. When entering the function the old cmode may be 0, in this case > mv88e6390x_serdes_get_lane() returns -ENODEV. As result we bail > out and have no chance to set a new mode. Therefore deal properly > with -ENODEV. > > 2. Once we have disabled power and irq, let's set the cached cmode to 0. > This reflects the actual status and is cleaner if we bail out with an > error in the following function calls. > > 3. The cached cmode is used by mv88e6390x_serdes_get_lane(), > mv88e6390_serdes_power_lane() and mv88e6390_serdes_irq_enable(). > Currently we set the cached mode to the new one at the very end of > the function only, means until then we use the old one what may be > wrong. > > 4. When calling mv88e6390_serdes_irq_enable() we use the lane value > belonging to the old cmode. Get the lane belonging to the new cmode > before calling this function. > > It's hard to provide a good "Fixes" tag because quite a few smaller > changes have been done to the code in question recently. > > Fixes: d235c48b40d3 ("net: dsa: mv88e6xxx: power serdes on/off for 10G interfaces on 6390X") > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied, thanks.
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c index dce84a2a6..c44b2822e 100644 --- a/drivers/net/dsa/mv88e6xxx/port.c +++ b/drivers/net/dsa/mv88e6xxx/port.c @@ -427,18 +427,22 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, return 0; lane = mv88e6390x_serdes_get_lane(chip, port); - if (lane < 0) + if (lane < 0 && lane != -ENODEV) return lane; - if (chip->ports[port].serdes_irq) { - err = mv88e6390_serdes_irq_disable(chip, port, lane); + if (lane >= 0) { + if (chip->ports[port].serdes_irq) { + err = mv88e6390_serdes_irq_disable(chip, port, lane); + if (err) + return err; + } + + err = mv88e6390x_serdes_power(chip, port, false); if (err) return err; } - err = mv88e6390x_serdes_power(chip, port, false); - if (err) - return err; + chip->ports[port].cmode = 0; if (cmode) { err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, ®); @@ -452,6 +456,12 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, if (err) return err; + chip->ports[port].cmode = cmode; + + lane = mv88e6390x_serdes_get_lane(chip, port); + if (lane < 0) + return lane; + err = mv88e6390x_serdes_power(chip, port, true); if (err) return err; @@ -463,8 +473,6 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, } } - chip->ports[port].cmode = cmode; - return 0; }
This patches fixes few issues in mv88e6390x_port_set_cmode(). 1. When entering the function the old cmode may be 0, in this case mv88e6390x_serdes_get_lane() returns -ENODEV. As result we bail out and have no chance to set a new mode. Therefore deal properly with -ENODEV. 2. Once we have disabled power and irq, let's set the cached cmode to 0. This reflects the actual status and is cleaner if we bail out with an error in the following function calls. 3. The cached cmode is used by mv88e6390x_serdes_get_lane(), mv88e6390_serdes_power_lane() and mv88e6390_serdes_irq_enable(). Currently we set the cached mode to the new one at the very end of the function only, means until then we use the old one what may be wrong. 4. When calling mv88e6390_serdes_irq_enable() we use the lane value belonging to the old cmode. Get the lane belonging to the new cmode before calling this function. It's hard to provide a good "Fixes" tag because quite a few smaller changes have been done to the code in question recently. Fixes: d235c48b40d3 ("net: dsa: mv88e6xxx: power serdes on/off for 10G interfaces on 6390X") Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/dsa/mv88e6xxx/port.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)