[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6c60860b-dd3c-4d1c-945b-edb8ef6a8618@lunn.ch>
Date: Fri, 6 Sep 2024 17:48:08 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jacky Chou <jacky_chou@...eedtech.com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Jacob Keller <jacob.e.keller@...el.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH net-next] net: ftgmac100: Fix potential NULL dereference
in error handling
On Fri, Sep 06, 2024 at 06:06:14AM +0000, Jacky Chou wrote:
> Hello,
>
> >
> > We might not have a phy so we need to check for NULL before calling
> > phy_stop(netdev->phydev) or it could lead to an Oops.
> >
> > Fixes: e24a6c874601 ("net: ftgmac100: Get link speed and duplex for NC-SI")
> > Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> > ---
> > drivers/net/ethernet/faraday/ftgmac100.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > b/drivers/net/ethernet/faraday/ftgmac100.c
> > index f3cc14cc757d..0e873e6f60d6 100644
> > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > @@ -1565,7 +1565,8 @@ static int ftgmac100_open(struct net_device
> > *netdev)
> > return 0;
> >
> > err_ncsi:
> > - phy_stop(netdev->phydev);
> > + if (netdev->phydev)
> > + phy_stop(netdev->phydev);
> When using " use-ncsi" property, the driver will register a fixed-link phy device and
> bind to netdev at probe stage.
>
> if (np && of_get_property(np, "use-ncsi", NULL)) {
>
> ......
>
> phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL);
> err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link,
> PHY_INTERFACE_MODE_MII);
> if (err) {
> dev_err(&pdev->dev, "Connecting PHY failed\n");
> goto err_phy_connect;
> }
> } else if (np && of_phy_is_fixed_link(np)) {
>
> Therefore, it does not need to check if the point is NULL in this error handling.
> Thanks.
Are you actually saying:
if (netdev->phydev) {
/* If we have a PHY, start polling */
phy_start(netdev->phydev);
}
is wrong, it is guaranteed there is always a phydev?
Andrew
Powered by blists - more mailing lists