| Message ID | E1pcSOp-00DiAo-Su@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ef63461caf427a77a04620d74ba90035a712af9c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Minor fixes for pcs_get_state() implementations | expand |
Hi Russell, On Wed, 2023-03-15 at 14:46 +0000, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Phylink does not want the current state of the link when reading the > PCS link state - it wants the latched state. Don't double-read the > MII status register. Phylink will re-read as necessary to capture > transient link-down events as of dbae3388ea9c ("net: phylink: Force > retrigger in case of latched link-fail indicator"). > > The above referenced commit is a dependency for this change, and thus > this change should not be backported to any kernel that does not > contain the above referenced commit. > > Fixes: fcb26bd2b6ca ("net: phy: Add Synopsys DesignWare XPCS MDIO module") > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/pcs/pcs-xpcs.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > index bc428a816719..04a685353041 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -321,7 +321,7 @@ static int xpcs_read_fault_c73(struct dw_xpcs *xpcs, > return 0; > } > > -static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an) > +static int xpcs_read_link_c73(struct dw_xpcs *xpcs) > { > bool link = true; > int ret; > @@ -333,15 +333,6 @@ static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an) > if (!(ret & MDIO_STAT1_LSTATUS)) > link = false; > > - if (an) { > - ret = xpcs_read(xpcs, MDIO_MMD_AN, MDIO_STAT1); > - if (ret < 0) > - return ret; > - > - if (!(ret & MDIO_STAT1_LSTATUS)) > - link = false; > - } > - > return link; > } > > @@ -935,7 +926,7 @@ static int xpcs_get_state_c73(struct dw_xpcs *xpcs, > int ret; > > /* Link needs to be read first ... */ > - state->link = xpcs_read_link_c73(xpcs, state->an_enabled) > 0 ? 1 : 0; > + state->link = xpcs_read_link_c73(xpcs) > 0 ? 1 : 0; Couldn't you just say: state->link = xpcs_read_link_c73(xpcs) > 0; That should be a boolean, right? > > /* ... and then we check the faults. */ > ret = xpcs_read_fault_c73(xpcs, state); > -- > 2.30.2 > BR Steen
On Thu, Mar 16, 2023 at 10:44:48AM +0100, Steen Hegelund wrote: > Hi Russell, > > > On Wed, 2023-03-15 at 14:46 +0000, Russell King (Oracle) wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Phylink does not want the current state of the link when reading the > > PCS link state - it wants the latched state. Don't double-read the > > MII status register. Phylink will re-read as necessary to capture > > transient link-down events as of dbae3388ea9c ("net: phylink: Force > > retrigger in case of latched link-fail indicator"). > > > > The above referenced commit is a dependency for this change, and thus > > this change should not be backported to any kernel that does not > > contain the above referenced commit. > > > > Fixes: fcb26bd2b6ca ("net: phy: Add Synopsys DesignWare XPCS MDIO module") > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/pcs/pcs-xpcs.c | 13 ++----------- > > 1 file changed, 2 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > index bc428a816719..04a685353041 100644 > > --- a/drivers/net/pcs/pcs-xpcs.c > > +++ b/drivers/net/pcs/pcs-xpcs.c > > @@ -321,7 +321,7 @@ static int xpcs_read_fault_c73(struct dw_xpcs *xpcs, > > return 0; > > } > > > > -static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an) > > +static int xpcs_read_link_c73(struct dw_xpcs *xpcs) > > { > > bool link = true; > > int ret; > > @@ -333,15 +333,6 @@ static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an) > > if (!(ret & MDIO_STAT1_LSTATUS)) > > link = false; > > > > - if (an) { > > - ret = xpcs_read(xpcs, MDIO_MMD_AN, MDIO_STAT1); > > - if (ret < 0) > > - return ret; > > - > > - if (!(ret & MDIO_STAT1_LSTATUS)) > > - link = false; > > - } > > - > > return link; > > } > > > > @@ -935,7 +926,7 @@ static int xpcs_get_state_c73(struct dw_xpcs *xpcs, > > int ret; > > > > /* Link needs to be read first ... */ > > - state->link = xpcs_read_link_c73(xpcs, state->an_enabled) > 0 ? 1 : 0; > > + state->link = xpcs_read_link_c73(xpcs) > 0 ? 1 : 0; > > Couldn't you just say: > > state->link = xpcs_read_link_c73(xpcs) > 0; > > That should be a boolean, right? That would be another change to the code - and consider how such a change would fit in this series given its description and also this patch. IMHO such a change should be a separate patch - and there's plenty of scope for cleanups like the one you suggest in this driver. Thanks.
Hi Russell, On Thu, 2023-03-16 at 16:12 +0000, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Thu, Mar 16, 2023 at 10:44:48AM +0100, Steen Hegelund wrote: > > Hi Russell, > > > > > > On Wed, 2023-03-15 at 14:46 +0000, Russell King (Oracle) wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > > content is safe > > > > > > Phylink does not want the current state of the link when reading the > > > PCS link state - it wants the latched state. Don't double-read the > > > MII status register. Phylink will re-read as necessary to capture > > > transient link-down events as of dbae3388ea9c ("net: phylink: Force > > > retrigger in case of latched link-fail indicator"). > > > > > > The above referenced commit is a dependency for this change, and thus > > > this change should not be backported to any kernel that does not > > > contain the above referenced commit. > > > > > > Fixes: fcb26bd2b6ca ("net: phy: Add Synopsys DesignWare XPCS MDIO module") > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > --- > > > drivers/net/pcs/pcs-xpcs.c | 13 ++----------- > > > 1 file changed, 2 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > > index bc428a816719..04a685353041 100644 > > > --- a/drivers/net/pcs/pcs-xpcs.c > > > +++ b/drivers/net/pcs/pcs-xpcs.c > > > @@ -321,7 +321,7 @@ static int xpcs_read_fault_c73(struct dw_xpcs *xpcs, > > > return 0; > > > } > > > > > > -static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an) > > > +static int xpcs_read_link_c73(struct dw_xpcs *xpcs) > > > { > > > bool link = true; > > > int ret; > > > @@ -333,15 +333,6 @@ static int xpcs_read_link_c73(struct dw_xpcs *xpcs, > > > bool an) > > > if (!(ret & MDIO_STAT1_LSTATUS)) > > > link = false; > > > > > > - if (an) { > > > - ret = xpcs_read(xpcs, MDIO_MMD_AN, MDIO_STAT1); > > > - if (ret < 0) > > > - return ret; > > > - > > > - if (!(ret & MDIO_STAT1_LSTATUS)) > > > - link = false; > > > - } > > > - > > > return link; > > > } > > > > > > @@ -935,7 +926,7 @@ static int xpcs_get_state_c73(struct dw_xpcs *xpcs, > > > int ret; > > > > > > /* Link needs to be read first ... */ > > > - state->link = xpcs_read_link_c73(xpcs, state->an_enabled) > 0 ? 1 > > > : 0; > > > + state->link = xpcs_read_link_c73(xpcs) > 0 ? 1 : 0; > > > > Couldn't you just say: > > > > state->link = xpcs_read_link_c73(xpcs) > 0; > > > > That should be a boolean, right? > > That would be another change to the code - and consider how such a > change would fit in this series given its description and also this > patch. > > IMHO such a change should be a separate patch - and there's plenty > of scope for cleanups like the one you suggest in this driver. > > Thanks. OK - got that! > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com> BR Steen
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index bc428a816719..04a685353041 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -321,7 +321,7 @@ static int xpcs_read_fault_c73(struct dw_xpcs *xpcs, return 0; } -static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an) +static int xpcs_read_link_c73(struct dw_xpcs *xpcs) { bool link = true; int ret; @@ -333,15 +333,6 @@ static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an) if (!(ret & MDIO_STAT1_LSTATUS)) link = false; - if (an) { - ret = xpcs_read(xpcs, MDIO_MMD_AN, MDIO_STAT1); - if (ret < 0) - return ret; - - if (!(ret & MDIO_STAT1_LSTATUS)) - link = false; - } - return link; } @@ -935,7 +926,7 @@ static int xpcs_get_state_c73(struct dw_xpcs *xpcs, int ret; /* Link needs to be read first ... */ - state->link = xpcs_read_link_c73(xpcs, state->an_enabled) > 0 ? 1 : 0; + state->link = xpcs_read_link_c73(xpcs) > 0 ? 1 : 0; /* ... and then we check the faults. */ ret = xpcs_read_fault_c73(xpcs, state);
Phylink does not want the current state of the link when reading the PCS link state - it wants the latched state. Don't double-read the MII status register. Phylink will re-read as necessary to capture transient link-down events as of dbae3388ea9c ("net: phylink: Force retrigger in case of latched link-fail indicator"). The above referenced commit is a dependency for this change, and thus this change should not be backported to any kernel that does not contain the above referenced commit. Fixes: fcb26bd2b6ca ("net: phy: Add Synopsys DesignWare XPCS MDIO module") Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/pcs/pcs-xpcs.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)