| Message ID | Y1dzCCMCDswQFVvO@dc75zzyyyyyyyyyyyyyby-3.rev.dnainternet.fi (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | drivers: fwnode: fix fwnode_irq_get_byname() kerneldoc | expand |
Moi, On Tue, Oct 25, 2022 at 08:24:24AM +0300, Matti Vaittinen wrote: > The fwnode_irq_get_byname() may return zero on device-tree mapping > error. Fix documentation to reflect this as current documentation > suggests check: > > if (ret < 0) > is enough to detect the errors. This is not the case. > > Add zero as a return value indicating error. > > Fixes: ca0acb511c21 ("device property: Add fwnode_irq_get_byname") > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > --- > drivers/base/property.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 4d6278a84868..df437d10aa08 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -960,7 +960,7 @@ EXPORT_SYMBOL(fwnode_irq_get); > * string. > * > * Return: > - * Linux IRQ number on success, or negative errno otherwise. > + * Linux IRQ number on success, zero or negative errno otherwise. I wonder if it would be possible instead to always return a negative error code on error. Returning zero on error is really unconventional and can be expected to be a source of bugs. We have code already that takes the error code zero into account in e.g. static int smbalert_probe(struct i2c_client *ara, const struct i2c_device_id *id) { ... irq = fwnode_irq_get_byname(dev_fwnode(adapter->dev.parent), "smbus_alert"); if (irq <= 0) return irq; And zero turns into successful probe! > */ > int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name) > {
Hi Sakari, On 10/25/22 09:48, Sakari Ailus wrote: > Moi, > > On Tue, Oct 25, 2022 at 08:24:24AM +0300, Matti Vaittinen wrote: >> The fwnode_irq_get_byname() may return zero on device-tree mapping >> error. Fix documentation to reflect this as current documentation >> suggests check: >> >> if (ret < 0) >> is enough to detect the errors. This is not the case. >> >> Add zero as a return value indicating error. >> >> Fixes: ca0acb511c21 ("device property: Add fwnode_irq_get_byname") >> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >> --- >> drivers/base/property.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/base/property.c b/drivers/base/property.c >> index 4d6278a84868..df437d10aa08 100644 >> --- a/drivers/base/property.c >> +++ b/drivers/base/property.c >> @@ -960,7 +960,7 @@ EXPORT_SYMBOL(fwnode_irq_get); >> * string. >> * >> * Return: >> - * Linux IRQ number on success, or negative errno otherwise. >> + * Linux IRQ number on success, zero or negative errno otherwise. > > I wonder if it would be possible instead to always return a negative error > code on error. Returning zero on error is really unconventional and can be > expected to be a source of bugs. Agree, and I did also consider just adding: if (!ret) return -EINVAL; (or another feasible errno) return ret; at the end of the fwnode_irq_get_byname(). However, such a functional change would require auditing the existing callers which I have no time right now. if (someone is up to the task) be my guest :) else please fix the doc ;) Yours -- Matti
ti 25. lokak. 2022 klo 10.06 Matti Vaittinen (mazziesaccount@gmail.com) kirjoitti: > > Hi Sakari, > > On 10/25/22 09:48, Sakari Ailus wrote: > > Moi, > > > > On Tue, Oct 25, 2022 at 08:24:24AM +0300, Matti Vaittinen wrote: > >> The fwnode_irq_get_byname() may return zero on device-tree mapping > >> error. Fix documentation to reflect this as current documentation > >> suggests check: > >> > >> if (ret < 0) > >> is enough to detect the errors. This is not the case. > >> > >> Add zero as a return value indicating error. > >> > >> Fixes: ca0acb511c21 ("device property: Add fwnode_irq_get_byname") > >> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > >> --- > >> drivers/base/property.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/base/property.c b/drivers/base/property.c > >> index 4d6278a84868..df437d10aa08 100644 > >> --- a/drivers/base/property.c > >> +++ b/drivers/base/property.c > >> @@ -960,7 +960,7 @@ EXPORT_SYMBOL(fwnode_irq_get); > >> * string. > >> * > >> * Return: > >> - * Linux IRQ number on success, or negative errno otherwise. > >> + * Linux IRQ number on success, zero or negative errno otherwise. > > > > I wonder if it would be possible instead to always return a negative error > > code on error. Returning zero on error is really unconventional and can be > > expected to be a source of bugs. > > Agree, and I did also consider just adding: > > if (!ret) > return -EINVAL; (or another feasible errno) > > return ret; > > at the end of the fwnode_irq_get_byname(). > > However, such a functional change would require auditing the existing > callers which I have no time right now. Oh. I just did grep the callers. It seems to me that there are only a handful of callers in 6.1-rc2. Auditing those does not seem like a big task after all. So I guess I can check them if changing the return value is preferred. Yours, --Matti
On Tue, Oct 25, 2022 at 10:17:21AM +0300, Matti Vaittinen wrote: > ti 25. lokak. 2022 klo 10.06 Matti Vaittinen > (mazziesaccount@gmail.com) kirjoitti: > > > > Hi Sakari, > > > > On 10/25/22 09:48, Sakari Ailus wrote: > > > Moi, > > > > > > On Tue, Oct 25, 2022 at 08:24:24AM +0300, Matti Vaittinen wrote: > > >> The fwnode_irq_get_byname() may return zero on device-tree mapping > > >> error. Fix documentation to reflect this as current documentation > > >> suggests check: > > >> > > >> if (ret < 0) > > >> is enough to detect the errors. This is not the case. > > >> > > >> Add zero as a return value indicating error. > > >> > > >> Fixes: ca0acb511c21 ("device property: Add fwnode_irq_get_byname") > > >> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > >> --- > > >> drivers/base/property.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/base/property.c b/drivers/base/property.c > > >> index 4d6278a84868..df437d10aa08 100644 > > >> --- a/drivers/base/property.c > > >> +++ b/drivers/base/property.c > > >> @@ -960,7 +960,7 @@ EXPORT_SYMBOL(fwnode_irq_get); > > >> * string. > > >> * > > >> * Return: > > >> - * Linux IRQ number on success, or negative errno otherwise. > > >> + * Linux IRQ number on success, zero or negative errno otherwise. > > > > > > I wonder if it would be possible instead to always return a negative error > > > code on error. Returning zero on error is really unconventional and can be > > > expected to be a source of bugs. > > > > Agree, and I did also consider just adding: > > > > if (!ret) > > return -EINVAL; (or another feasible errno) > > > > return ret; > > > > at the end of the fwnode_irq_get_byname(). > > > > However, such a functional change would require auditing the existing > > callers which I have no time right now. > > Oh. I just did grep the callers. It seems to me that there are only a > handful of callers in 6.1-rc2. Auditing those does not seem like a big > task after all. So I guess I can check them if changing the return > value is preferred. Yes, please do so. thanks, greg k-h
diff --git a/drivers/base/property.c b/drivers/base/property.c index 4d6278a84868..df437d10aa08 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -960,7 +960,7 @@ EXPORT_SYMBOL(fwnode_irq_get); * string. * * Return: - * Linux IRQ number on success, or negative errno otherwise. + * Linux IRQ number on success, zero or negative errno otherwise. */ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name) {
The fwnode_irq_get_byname() may return zero on device-tree mapping error. Fix documentation to reflect this as current documentation suggests check: if (ret < 0) is enough to detect the errors. This is not the case. Add zero as a return value indicating error. Fixes: ca0acb511c21 ("device property: Add fwnode_irq_get_byname") Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- drivers/base/property.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)