| Message ID | 20230821025020.1971520-4-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Return PTR_ERR() for fixed_phy_register() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | success | Patch already applied to net-next |
On Mon, 21 Aug 2023 10:50:20 +0800 Jinjie Ruan wrote: > fixed_phy_register() returns -EPROBE_DEFER, -EINVAL and -EBUSY, > etc, in addition to -EIO. The Best practice is to return these > error codes with PTR_ERR(). EPROBE_DEFER is not a unix error code. We can't return it to user space, so propagating it from ndo_open is not correct. > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c > index a36f6369f132..c81cdeb4d4e7 100644 > --- a/drivers/net/ethernet/microchip/lan743x_main.c > +++ b/drivers/net/ethernet/microchip/lan743x_main.c > @@ -1515,7 +1515,7 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter) > &fphy_status, NULL); > if (IS_ERR(phydev)) { > netdev_err(netdev, "No PHY/fixed_PHY found\n"); > - return -EIO; > + return PTR_ERR(phydev); > } > } else { > goto return_error;
On 2023/8/23 8:03, Jakub Kicinski wrote: > On Mon, 21 Aug 2023 10:50:20 +0800 Jinjie Ruan wrote: >> fixed_phy_register() returns -c, -EINVAL and -EBUSY, >> etc, in addition to -EIO. The Best practice is to return these >> error codes with PTR_ERR(). > > EPROBE_DEFER is not a unix error code. We can't return it to user > space, so propagating it from ndo_open is not correct. When the error is EPROBE_DEFER, Whether print the netdev_err is ok? And what should it return? How about this? if (IS_ERR(phydev)) { ...... if (PTR_ERR(phydev) != -EPROBE_DEFER) return PTR_ERR(phydev); else return -EIO; } > >> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c >> index a36f6369f132..c81cdeb4d4e7 100644 >> --- a/drivers/net/ethernet/microchip/lan743x_main.c >> +++ b/drivers/net/ethernet/microchip/lan743x_main.c >> @@ -1515,7 +1515,7 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter) >> &fphy_status, NULL); >> if (IS_ERR(phydev)) { >> netdev_err(netdev, "No PHY/fixed_PHY found\n"); >> - return -EIO; >> + return PTR_ERR(phydev); >> } >> } else { >> goto return_error;
On Wed, 23 Aug 2023 15:43:58 +0800 Ruan Jinjie wrote: > > EPROBE_DEFER is not a unix error code. We can't return it to user > > space, so propagating it from ndo_open is not correct. > > When the error is EPROBE_DEFER, Whether print the netdev_err is ok? And > what should it return? > > How about this? > > if (IS_ERR(phydev)) { > ...... > if (PTR_ERR(phydev) != -EPROBE_DEFER) > return PTR_ERR(phydev); > else > return -EIO; > } That's too much code to copy & paste into every driver. Someone who understands the code flow very well should tackle this. Please leave the cases where fixed_phy_register() is called outside of probe alone.
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c index a36f6369f132..c81cdeb4d4e7 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.c +++ b/drivers/net/ethernet/microchip/lan743x_main.c @@ -1515,7 +1515,7 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter) &fphy_status, NULL); if (IS_ERR(phydev)) { netdev_err(netdev, "No PHY/fixed_PHY found\n"); - return -EIO; + return PTR_ERR(phydev); } } else { goto return_error;
fixed_phy_register() returns -EPROBE_DEFER, -EINVAL and -EBUSY, etc, in addition to -EIO. The Best practice is to return these error codes with PTR_ERR(). Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- v4: - Update to bring the author's name before. v3: - Update the commit title and message. --- drivers/net/ethernet/microchip/lan743x_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)