| Message ID | 20240419-mv88e6xx-list_empty-v1-1-64fd6d1059a8@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,RFC] net: dsa: mv88e6xxx: Correct check for empty list | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-1 |
On Fri, Apr 19, 2024 at 01:17:48PM +0100, Simon Horman wrote: > Since commit a3c53be55c95 ("net: dsa: mv88e6xxx: Support multiple MDIO > busses") mv88e6xxx_default_mdio_bus() has checked that the > return value of list_first_entry() is non-NULL. This appears to be > intended to guard against the list chip->mdios being empty. > However, it is not the correct check as the implementation of > list_first_entry is not designed to return NULL for empty lists. > > Instead check directly if the list is empty. > > Flagged by Smatch > > Signed-off-by: Simon Horman <horms@kernel.org> Hi Simon This looks good to me. I would not consider it a fix. As you say, it has been like this a long time and never bothered anybody, which is one of the stable rules. It might be possible to have an empty list, if there are no nodes in DT. But that is something which a novice would do when writing the DT, and so probably would of reported it. However, list_first_entry() does document: * Note, that list is expected to be not empty. So testing it first is wise. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Thanks Andrew
On Fri, Apr 19, 2024 at 01:17:48PM +0100, Simon Horman wrote: > Since commit a3c53be55c95 ("net: dsa: mv88e6xxx: Support multiple MDIO > busses") mv88e6xxx_default_mdio_bus() has checked that the > return value of list_first_entry() is non-NULL. This appears to be > intended to guard against the list chip->mdios being empty. > However, it is not the correct check as the implementation of > list_first_entry is not designed to return NULL for empty lists. > > Instead check directly if the list is empty. > > Flagged by Smatch > > Signed-off-by: Simon Horman <horms@kernel.org> > --- > I'm unsure if this should be considered a fix: it's been around since > v4.11 and the patch is dated January 2017. Perhaps an empty list simply > cannot occur. If so, the function could be simplified by not checking > for an empty list. And, if mdio_bus->bus, then perhaps caller may be > simplified not to check for an error condition. > > It is because I am unsure that I have marked this as an RFC. > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index e950a634a3c7..a236c9fe6a74 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -131,10 +131,11 @@ struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip) > { > struct mv88e6xxx_mdio_bus *mdio_bus; > > + if (list_empty(&chip->mdios)) > + return NULL; > + > mdio_bus = list_first_entry(&chip->mdios, struct mv88e6xxx_mdio_bus, > list); > - if (!mdio_bus) > - return NULL; The other option here would have been to use list_first_entry_or_null(). regards, dan carpenter
On Mon, Apr 22, 2024 at 01:40:01PM +0300, Dan Carpenter wrote: > On Fri, Apr 19, 2024 at 01:17:48PM +0100, Simon Horman wrote: > > Since commit a3c53be55c95 ("net: dsa: mv88e6xxx: Support multiple MDIO > > busses") mv88e6xxx_default_mdio_bus() has checked that the > > return value of list_first_entry() is non-NULL. This appears to be > > intended to guard against the list chip->mdios being empty. > > However, it is not the correct check as the implementation of > > list_first_entry is not designed to return NULL for empty lists. > > > > Instead check directly if the list is empty. > > > > Flagged by Smatch > > > > Signed-off-by: Simon Horman <horms@kernel.org> > > --- > > I'm unsure if this should be considered a fix: it's been around since > > v4.11 and the patch is dated January 2017. Perhaps an empty list simply > > cannot occur. If so, the function could be simplified by not checking > > for an empty list. And, if mdio_bus->bus, then perhaps caller may be > > simplified not to check for an error condition. > > > > It is because I am unsure that I have marked this as an RFC. > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > > index e950a634a3c7..a236c9fe6a74 100644 > > --- a/drivers/net/dsa/mv88e6xxx/chip.c > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > > @@ -131,10 +131,11 @@ struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip) > > { > > struct mv88e6xxx_mdio_bus *mdio_bus; > > > > + if (list_empty(&chip->mdios)) > > + return NULL; > > + > > mdio_bus = list_first_entry(&chip->mdios, struct mv88e6xxx_mdio_bus, > > list); > > - if (!mdio_bus) > > - return NULL; > > The other option here would have been to use list_first_entry_or_null(). Thanks, I guess that is nicer than the open-coded approach I suggested.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index e950a634a3c7..a236c9fe6a74 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -131,10 +131,11 @@ struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip) { struct mv88e6xxx_mdio_bus *mdio_bus; + if (list_empty(&chip->mdios)) + return NULL; + mdio_bus = list_first_entry(&chip->mdios, struct mv88e6xxx_mdio_bus, list); - if (!mdio_bus) - return NULL; return mdio_bus->bus; } --- drivers/net/dsa/mv88e6xxx/chip.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index e950a634a3c7..a236c9fe6a74 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -131,10 +131,11 @@ struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip) { struct mv88e6xxx_mdio_bus *mdio_bus; + if (list_empty(&chip->mdios)) + return NULL; + mdio_bus = list_first_entry(&chip->mdios, struct mv88e6xxx_mdio_bus, list); - if (!mdio_bus) - return NULL; return mdio_bus->bus; }
Since commit a3c53be55c95 ("net: dsa: mv88e6xxx: Support multiple MDIO busses") mv88e6xxx_default_mdio_bus() has checked that the return value of list_first_entry() is non-NULL. This appears to be intended to guard against the list chip->mdios being empty. However, it is not the correct check as the implementation of list_first_entry is not designed to return NULL for empty lists. Instead check directly if the list is empty. Flagged by Smatch Signed-off-by: Simon Horman <horms@kernel.org> --- I'm unsure if this should be considered a fix: it's been around since v4.11 and the patch is dated January 2017. Perhaps an empty list simply cannot occur. If so, the function could be simplified by not checking for an empty list. And, if mdio_bus->bus, then perhaps caller may be simplified not to check for an error condition. It is because I am unsure that I have marked this as an RFC.