[PATCH net-next RFC] net: dsa: mv88e6xxx: Correct check for empty list

[PATCH net-next RFC] net: dsa: mv88e6xxx: Correct check for empty list

[Simon Horman]

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;

 	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;
 }

Re: [PATCH net-next RFC] net: dsa: mv88e6xxx: Correct check for empty list

[Andrew Lunn]

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

Re: [PATCH net-next RFC] net: dsa: mv88e6xxx: Correct check for empty list

[Dan Carpenter]

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

Re: [PATCH net-next RFC] net: dsa: mv88e6xxx: Correct check for empty list

[Simon Horman]

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.