| Message ID | 20200224144048.6587-1-jonathanh@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | regulator: pwm: Don't warn on probe deferral | expand |
On Mon, Feb 24, 2020 at 02:40:48PM +0000, Jon Hunter wrote: > Deferred probe is an expected return value for devm_pwm_get(). Given > that the driver deals with it properly, there's no need to output a > warning that may potentially confuse users. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/regulator/pwm-regulator.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Thierry Reding <treding@nvidia.com>
On Mon, Feb 24, 2020 at 02:40:48PM +0000, Jon Hunter wrote: > Deferred probe is an expected return value for devm_pwm_get(). Given > that the driver deals with it properly, there's no need to output a > warning that may potentially confuse users. > ret = PTR_ERR(drvdata->pwm); > - dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret); > + if (ret != -EPROBE_DEFER) > + dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret); This then means that there's no way for users to determine why the driver has failed to instantiate which can be frustrating. It'd be better to at least have some dev_dbg() output when deferring so that there's something for people to go on without having to instrument the code.
On Mon, Feb 24, 2020 at 04:58:59PM +0000, Mark Brown wrote: > On Mon, Feb 24, 2020 at 02:40:48PM +0000, Jon Hunter wrote: > > Deferred probe is an expected return value for devm_pwm_get(). Given > > that the driver deals with it properly, there's no need to output a > > warning that may potentially confuse users. > > > ret = PTR_ERR(drvdata->pwm); > > - dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret); > > + if (ret != -EPROBE_DEFER) > > + dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret); > > This then means that there's no way for users to determine why the > driver has failed to instantiate which can be frustrating. It'd be > better to at least have some dev_dbg() output when deferring so that > there's something for people to go on without having to instrument the > code. Not printing an error message is quite usual however. I think a generic approach that for example makes the list of devices that should be retried to probe on the next opportunity inspectable would be nice. Best regards Uwe
On Wed, Feb 26, 2020 at 05:17:57PM +0100, Uwe Kleine-König wrote: > On Mon, Feb 24, 2020 at 04:58:59PM +0000, Mark Brown wrote: > > This then means that there's no way for users to determine why the > > driver has failed to instantiate which can be frustrating. It'd be > > better to at least have some dev_dbg() output when deferring so that > > there's something for people to go on without having to instrument the > > code. > Not printing an error message is quite usual however. I think a generic Usual yet also frustraing. > approach that for example makes the list of devices that should be > retried to probe on the next opportunity inspectable would be nice. That's not really the issue, the bigger issue is trying to figure out why things are stuck - what exactly caused things to fail to instantiate.
On Wed, Feb 26, 2020 at 04:39:05PM +0000, Mark Brown wrote: > On Wed, Feb 26, 2020 at 05:17:57PM +0100, Uwe Kleine-König wrote: > > On Mon, Feb 24, 2020 at 04:58:59PM +0000, Mark Brown wrote: > > > > This then means that there's no way for users to determine why the > > > driver has failed to instantiate which can be frustrating. It'd be > > > better to at least have some dev_dbg() output when deferring so that > > > there's something for people to go on without having to instrument the > > > code. > > > Not printing an error message is quite usual however. I think a generic > > Usual yet also frustraing. > > > approach that for example makes the list of devices that should be > > retried to probe on the next opportunity inspectable would be nice. > > That's not really the issue, the bigger issue is trying to figure out > why things are stuck - what exactly caused things to fail to > instantiate. I wonder if we should do something like: ret = some_call(some, args); if (ret) { if (emit_errmsg_for_err(ret)) dev_err(dev, "some_call failed: %pE\n", ERR_PTR(ret)); return ret; } and have emit_errmsg_for_err return true if ret != -EPROBE_DEFER or some kernel parameter is given. Best regards Uwe
On Fri, Mar 06, 2020 at 08:51:29AM +0100, Uwe Kleine-König wrote: > I wonder if we should do something like: > ret = some_call(some, args); > if (ret) { > if (emit_errmsg_for_err(ret)) > dev_err(dev, "some_call failed: %pE\n", ERR_PTR(ret)); > return ret; > } > and have emit_errmsg_for_err return true if ret != -EPROBE_DEFER or some > kernel parameter is given. There was some effort in the past to have a dev_probe_err() or something which could have a similar implementation but that didn't end up going anywhere I think. I do prefer the debug level log since it's much easier to have the information there without having to ask for it, that design would've supported that.
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c index e74e11101fc1..fb25777a7d47 100644 --- a/drivers/regulator/pwm-regulator.c +++ b/drivers/regulator/pwm-regulator.c @@ -354,7 +354,8 @@ static int pwm_regulator_probe(struct platform_device *pdev) drvdata->pwm = devm_pwm_get(&pdev->dev, NULL); if (IS_ERR(drvdata->pwm)) { ret = PTR_ERR(drvdata->pwm); - dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret); + if (ret != -EPROBE_DEFER) + dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret); return ret; }
Deferred probe is an expected return value for devm_pwm_get(). Given that the driver deals with it properly, there's no need to output a warning that may potentially confuse users. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/regulator/pwm-regulator.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)