| Message ID | 20240427-mv88e6xx-list_empty-v2-1-b7ce47c77bc7@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,RFC,v2] net: dsa: mv88e6xxx: Correct check for empty list | expand |
On Sat, Apr 27, 2024 at 09:52:03AM +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, use list_first_entry() which does return NULL if the list is > empty. > > Flagged by Smatch. > Compile tested only. > > Signed-off-by: Simon Horman <horms@kernel.org> Sorry, I forgot to drop the RFC tag. I'll send a v3 after waiting for review. > --- > Changes in v2: > - Use list_first_entry_or_null() instead of open-coding > a condition on list_empty(). > Suggested by Dan Carpenter. > - Update commit message. > - Link to v1: https://lore.kernel.org/r/20240419-mv88e6xx-list_empty-v1-1-64fd6d1059a8@kernel.org > --- > As discussed in v1, this is not being considered a fix > as it has been like this for a long time without any > reported problems. > --- > drivers/net/dsa/mv88e6xxx/chip.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index f29ef72a2f1d..fc6e2e3ab0f0 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -131,8 +131,8 @@ struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip) > { > struct mv88e6xxx_mdio_bus *mdio_bus; > > - mdio_bus = list_first_entry(&chip->mdios, struct mv88e6xxx_mdio_bus, > - list); > + mdio_bus = list_first_entry_or_null(&chip->mdios, > + struct mv88e6xxx_mdio_bus, list); > if (!mdio_bus) > return NULL; > > >
On Sat, Apr 27, 2024 at 09:53:06AM +0100, Simon Horman wrote: > On Sat, Apr 27, 2024 at 09:52:03AM +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, use list_first_entry() which does return NULL if the list is ^^^^^^^^^^^^^^^^^^ list_first_entry_or_null() regards, dan carpenter
On Sat, Apr 27, 2024 at 09:52:03AM +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, use list_first_entry() which does return NULL if the list is > empty. > > Flagged by Smatch. > Compile tested only. > > Signed-off-by: Simon Horman <horms@kernel.org> For the code itself: Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index f29ef72a2f1d..fc6e2e3ab0f0 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -131,8 +131,8 @@ struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip) { struct mv88e6xxx_mdio_bus *mdio_bus; - mdio_bus = list_first_entry(&chip->mdios, struct mv88e6xxx_mdio_bus, - list); + mdio_bus = list_first_entry_or_null(&chip->mdios, + struct mv88e6xxx_mdio_bus, list); if (!mdio_bus) return NULL;
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, use list_first_entry() which does return NULL if the list is empty. Flagged by Smatch. Compile tested only. Signed-off-by: Simon Horman <horms@kernel.org> --- Changes in v2: - Use list_first_entry_or_null() instead of open-coding a condition on list_empty(). Suggested by Dan Carpenter. - Update commit message. - Link to v1: https://lore.kernel.org/r/20240419-mv88e6xx-list_empty-v1-1-64fd6d1059a8@kernel.org --- As discussed in v1, this is not being considered a fix as it has been like this for a long time without any reported problems. --- drivers/net/dsa/mv88e6xxx/chip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)