Messages in this thread | | | Date | Mon, 26 Jun 2023 08:50:51 +0200 | From | Michael Walle <> | Subject | Re: [PATCH net-next v2 06/10] net: phy: print an info if a broken C45 bus is found |
| |
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
| |