| Message ID | 20230620-feature-c45-over-c22-v2-6-def0ab9ccee2@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: C45-over-C22 access | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 22 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Jun 23, 2023 at 12:29:15PM +0200, Michael Walle wrote: > If there is an PHY which gets confused by C45 transactions on the MDIO > bus, print an info together with the PHY identifier of the offending > one. > > Signed-off-by: Michael Walle <mwalle@kernel.org> > > --- > I wasn't sure if this should be phydev_dbg() or phydev_info(). I mainly > see this as an info to a user why some PHYs might not be probed (or > c45-over-c22 is used later). The information is useful to the DT writer, not the 'user'. I would assume the DT writer has a bit more kernel knowledge and can debug prints on. So i would suggest phydev_dbg(). > @@ -617,10 +617,10 @@ static int mdiobus_scan_bus_c45(struct mii_bus *bus) > */ > void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus) > { > + struct phy_device *phydev; > int i; > > for (i = 0; i < PHY_MAX_ADDR; i++) { > - struct phy_device *phydev; > u32 oui; It is not clear why you changed the scope of phydev. I guess another version used phydev_info(), where as now you have dev_info()? Andrew > > phydev = mdiobus_get_phy(bus, i); > @@ -633,6 +633,11 @@ void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus) > break; > } > } > + > + if (bus->prevent_c45_access) > + dev_info(&bus->dev, > + "Detected broken PHY (ID %08lx). Disabling C45 bus transactions.\n", > + (unsigned long)phydev->phy_id); > } > > /** > > -- > 2.39.2 >
On Fri, Jun 23, 2023 at 07:42:08PM +0200, Andrew Lunn wrote: > On Fri, Jun 23, 2023 at 12:29:15PM +0200, Michael Walle wrote: > > If there is an PHY which gets confused by C45 transactions on the MDIO > > bus, print an info together with the PHY identifier of the offending > > one. > > > > Signed-off-by: Michael Walle <mwalle@kernel.org> > > > > --- > > I wasn't sure if this should be phydev_dbg() or phydev_info(). I mainly > > see this as an info to a user why some PHYs might not be probed (or > > c45-over-c22 is used later). > > The information is useful to the DT writer, not the 'user'. I would > assume the DT writer has a bit more kernel knowledge and can debug > prints on. So i would suggest phydev_dbg(). > > > @@ -617,10 +617,10 @@ static int mdiobus_scan_bus_c45(struct mii_bus *bus) > > */ > > void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus) > > { > > + struct phy_device *phydev; > > int i; > > > > for (i = 0; i < PHY_MAX_ADDR; i++) { > > - struct phy_device *phydev; > > u32 oui; > > It is not clear why you changed the scope of phydev. I guess another > version used phydev_info(), where as now you have dev_info()? I think it is so it can be used in the dev_info() call below. However Smatch has it's doubts that it is always initialised there. .../mdio_bus.c:638 mdiobus_scan_for_broken_c45_access() error: we previously assumed 'phydev' could be null (see line 627) > > phydev = mdiobus_get_phy(bus, i); Line 627 immediately follows the line above, like this: if (!phydev) continue; > > @@ -633,6 +633,11 @@ void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus) > > break; > > } > > } > > + > > + if (bus->prevent_c45_access) > > + dev_info(&bus->dev, > > + "Detected broken PHY (ID %08lx). Disabling C45 bus transactions.\n", > > + (unsigned long)phydev->phy_id); > > } > > > > /** > > > > -- > > 2.39.2 > > >
Am 2023-06-23 22:35, schrieb Simon Horman: > On Fri, Jun 23, 2023 at 07:42:08PM +0200, Andrew Lunn wrote: >> On Fri, Jun 23, 2023 at 12:29:15PM +0200, Michael Walle wrote: >> > If there is an PHY which gets confused by C45 transactions on the MDIO >> > bus, print an info together with the PHY identifier of the offending >> > one. >> > >> > Signed-off-by: Michael Walle <mwalle@kernel.org> >> > >> > --- >> > I wasn't sure if this should be phydev_dbg() or phydev_info(). I mainly >> > see this as an info to a user why some PHYs might not be probed (or >> > c45-over-c22 is used later). >> >> The information is useful to the DT writer, not the 'user'. I would >> assume the DT writer has a bit more kernel knowledge and can debug >> prints on. So i would suggest phydev_dbg(). Why the DT writer? There could be no DT at all, right? But yeah, fair enough, I thought of our hardware engineers as a user, which might be surprised to find no C45 transactions at all for a C45 PHY. That said, I don't have a strong opinion. I'm fine to switch that to dev_dbg() to make the kernel output less noisy. >> > @@ -617,10 +617,10 @@ static int mdiobus_scan_bus_c45(struct mii_bus *bus) >> > */ >> > void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus) >> > { >> > + struct phy_device *phydev; >> > int i; >> > >> > for (i = 0; i < PHY_MAX_ADDR; i++) { >> > - struct phy_device *phydev; >> > u32 oui; >> >> It is not clear why you changed the scope of phydev. I guess another >> version used phydev_info(), where as now you have dev_info()? > > I think it is so it can be used in the dev_info() call below. Yes, to print the PHY ID of the offending one. > However Smatch has it's doubts that it is always initialised there. > > .../mdio_bus.c:638 mdiobus_scan_for_broken_c45_access() error: we > previously assumed 'phydev' could be null (see line 627) > >> > phydev = mdiobus_get_phy(bus, i); > > Line 627 immediately follows the line above, like this: > > if (!phydev) > continue; Mhh, I see. bus->prevent_c45_access could (theoretically) set before calling this function. I could set it to false at the beginning of this function or I could use a new flag to indicate when to print the warning. Any suggestions? -michael
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 00b25f6803bc..38529add6420 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -617,10 +617,10 @@ static int mdiobus_scan_bus_c45(struct mii_bus *bus) */ void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus) { + struct phy_device *phydev; int i; for (i = 0; i < PHY_MAX_ADDR; i++) { - struct phy_device *phydev; u32 oui; phydev = mdiobus_get_phy(bus, i); @@ -633,6 +633,11 @@ void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus) break; } } + + if (bus->prevent_c45_access) + dev_info(&bus->dev, + "Detected broken PHY (ID %08lx). Disabling C45 bus transactions.\n", + (unsigned long)phydev->phy_id); } /**
If there is an PHY which gets confused by C45 transactions on the MDIO bus, print an info together with the PHY identifier of the offending one. Signed-off-by: Michael Walle <mwalle@kernel.org> --- I wasn't sure if this should be phydev_dbg() or phydev_info(). I mainly see this as an info to a user why some PHYs might not be probed (or c45-over-c22 is used later). --- drivers/net/phy/mdio_bus.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)