[PATCH net-next] smsc911x: propagate reset GPIO errors

[PATCH net-next] smsc911x: propagate reset GPIO errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

devm_gpiod_get_optional() may return -EPROBE_DEFER or other errors,
but smsc911x_request_resources() currently does not handle them.

Check the return value and propagate errors from the reset GPIO lookup.
Release the regulators if they were successfully acquired before the
GPIO lookup failed.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 3ebd0664c697..41423baaf0af 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -439,6 +439,13 @@ static int smsc911x_request_resources(struct platform_device *pdev)
 	pdata->reset_gpiod = devm_gpiod_get_optional(&pdev->dev,
 						     "reset",
 						     GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->reset_gpiod)) {
+		if (!ret)
+			regulator_bulk_free(ARRAY_SIZE(pdata->supplies),
+					    pdata->supplies);
+		return dev_err_probe(&pdev->dev, PTR_ERR(pdata->reset_gpiod),
+				     "failed to get reset gpio\n");
+	}
 
 	/* Request clock */
 	pdata->clk = clk_get(&pdev->dev, NULL);
-- 
2.43.0

Re: [PATCH net-next] smsc911x: propagate reset GPIO errors

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] net: smsc911x: clock reference leak when regulator_bulk_get()
  fails
- [Low] The commit fixes a real, user-visible probe failure, but it has
  no Fixes: tag and its message does not say what goes wrong for users.

Pre-existing issues:
- [Low] The header comment on smsc911x_request_resources() (line 409)
  says "Request resources, currently just regulators." The function also…

--

smsc911x: propagate reset GPIO errors

This patch checks the return value of devm_gpiod_get_optional() for the
reset GPIO in smsc911x_request_resources() and propagates errors,
including -EPROBE_DEFER, via dev_err_probe(). If the regulators were
acquired before the GPIO lookup failed, it releases them first.

> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> index 3ebd0664c697..41423baaf0af 100644
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
> @@ -439,6 +439,13 @@ static int smsc911x_request_resources(struct platform_device *pdev)
>  	pdata->reset_gpiod = devm_gpiod_get_optional(&pdev->dev,
>  						     "reset",
>  						     GPIOD_OUT_LOW);
> +	if (IS_ERR(pdata->reset_gpiod)) {
> +		if (!ret)
> +			regulator_bulk_free(ARRAY_SIZE(pdata->supplies),
> +					    pdata->supplies);
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pdata->reset_gpiod),
> +				     "failed to get reset gpio\n");
> +	}

[Severity: Medium]
This problem was not introduced by this patch. The new "if (!ret)"
check depends on the same behaviour, though, so it seems worth raising
here.

If regulator_bulk_get() fails with an error other than -EPROBE_DEFER,
the error is logged and kept in ret, and the function carries on. The
reset GPIO lookup and clk_get() still run. If clk_get() succeeds,
pdata->clk holds a reference. The function then returns the saved
regulator error anyway.

In smsc911x_drv_probe(), a non-zero return from
smsc911x_request_resources() jumps to out_request_resources_fail.
That label only calls iounmap() and skips smsc911x_free_resources(),
so clk_put() is never called and the clock reference leaks:

smsc911x_drv_probe()
  smsc911x_request_resources()
    regulator_bulk_get() returns -EINVAL, saved in ret
    devm_gpiod_get_optional() succeeds
    clk_get() succeeds, pdata->clk holds a reference
    returns -EINVAL
  goto out_request_resources_fail
    iounmap(), but no clk_put()

The "try to continue" comment also does not match what happens. Probe
fails whenever ret is set, so the driver never continues without its
regulators.

Is probe meant to continue without regulators, as the comment says, or
should the regulator failure path just return ret right away? If it
returns early, the new "if (!ret)" test before regulator_bulk_free()
would no longer be needed. Either way, could this be fixed together
with the GPIO error handling, for example by returning early or by
calling clk_put() before returning an error?

[Severity: Low]
This looks like a fix for a probe failure that users can see. Should the
commit message have a Fixes: tag and say what goes wrong?

The unchecked lookup was added by:

commit dd0cb7dbb065 ("net: smsc911x: request and deassert optional RESET GPIO")

Before this patch, devm_gpiod_get_optional() returned
ERR_PTR(-EPROBE_DEFER) if the GPIO provider had not probed yet. That
error was ignored, so the reset line was never requested or deasserted.

Some boards do not pull RESET up, for example the APQ8060 Dragonboard
named in dd0cb7dbb065. On those boards the chip stays in reset, and
smsc911x_init() reads a bad BYTE_TEST or ID_REV value:

smsc911x_drv_probe()
  smsc911x_request_resources()
    devm_gpiod_get_optional() returns ERR_PTR(-EPROBE_DEFER), ignored
  smsc911x_enable_resources()
  smsc911x_init()
    returns -ENODEV

-ENODEV is not a deferral, so probe is never retried and the Ethernet port
is lost.

Could the commit message add the following tag and a sentence about this
failure, so the fix can be picked up for stable?

Fixes: dd0cb7dbb065 ("net: smsc911x: request and deassert optional RESET GPIO")

[Severity: Low]
This problem was not introduced by this patch, but the header comments
on these functions look stale.

The header on smsc911x_request_resources() says:

/*
 * Request resources, currently just regulators.
 ...

The function also requests the optional reset GPIO and the clock. With
this patch, a failed GPIO lookup can now make it return an error.

The header on smsc911x_free_resources() also says "currently just
regulators", but that function also calls clk_put().

Would it be worth updating both comments while touching this code?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007104731.507647-1-phucduc.bui%40gmail.com