Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 7 Sep 2023 09:38:07 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Bo Liu <liubo03@inspur.com>
Cc:     iyappan@os.amperecomputing.com, keyur@os.amperecomputing.com,
        quan@os.amperecomputing.com, andrew@lunn.ch, hkallweit1@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: mdio: Use helper function IS_ERR_OR_NULL()
Message-ID: <ZPmL72ShZz4gq2na@shell.armlinux.org.uk>
References: <20230907071705.3907-1-liubo03@inspur.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230907071705.3907-1-liubo03@inspur.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Sep 07, 2023 at 03:17:05AM -0400, Bo Liu wrote:
> Use IS_ERR_OR_NULL() to detect an error pointer or a null pointer
> open-coding to simplify the code.
> 
> Signed-off-by: Bo Liu <liubo03@inspur.com>

Please do a more thorough review of the code before proposing changes
to discover what the correct solution should be.

> diff --git a/drivers/net/mdio/mdio-xgene.c b/drivers/net/mdio/mdio-xgene.c
> index 1190a793555a..1518abfc3e22 100644
> --- a/drivers/net/mdio/mdio-xgene.c
> +++ b/drivers/net/mdio/mdio-xgene.c
> @@ -263,7 +263,7 @@ struct phy_device *xgene_enet_phy_register(struct mii_bus *bus, int phy_addr)
>  	struct phy_device *phy_dev;
>  
>  	phy_dev = get_phy_device(bus, phy_addr, false);
> -	if (!phy_dev || IS_ERR(phy_dev))
> +	if (IS_ERR_OR_NULL(phy_dev))

Looking at get_phy_device(), the returns from this function are either:

        if (r)
                return ERR_PTR(r);

which can't return NULL, or the return value from phy_device_create().

Looking at phy_device_create(), the returns from this function are:

        if (!dev)
                return ERR_PTR(-ENOMEM);

        if (ret) {
...
                dev = ERR_PTR(ret);
...
        return dev;

so I don't see any of this being able to return NULL either. Therefore,
the code that you are modifying should be:


-	if (!phy_dev || IS_ERR(phy_dev))
+	if (IS_ERR(phy_dev))

and the commit description needs to be updated to state that
get_phy_device() does not return NULL.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!