Messages in this thread | | | Date | Thu, 25 Nov 2021 13:18:08 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 2/2] gpiolib: check the 'ngpios' property in core gpiolib code |
| |
On Thu, Nov 25, 2021 at 11:10:08AM +0100, Bartosz Golaszewski wrote: > On Wed, Nov 24, 2021 at 3:47 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Nov 24, 2021 at 01:28:50PM +0100, Bartosz Golaszewski wrote:
...
> > > + ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios); > > > + if (ret) { > > > + if (ret == -ENODATA) { > > > + chip_err(gc, "tried to insert a GPIO chip with zero lines\n"); > > > + ret = -EINVAL; > > > + } > > > + > > > + goto err_free_descs; > > > + } > > > > And if the property returns 0 in ngpios? > > > > What about the modified suggestion from previous version: > > > > if (gc->ngpio == 0) { > > ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios);
> The comment is a good idea but other than that - it's overcomplicating things.
I don't think so. It is plain and self-explaining each step. See at the end of the message how.
> > if (ret == -ENODATA) > > ngpios = 0; > > else if (ret) > > return ret;
> You still need to goto err_free_descs here.
Right, this doesn't affect the main point / logic here.
> > gc->ngpio = ngpios; > > } > > > > if (gc->ngpio == 0) { > > Why check that again? We already know the driver set it to 0, we > checked it a couple lines before. If we can't get the setting from the > properties then it won't be non 0 here right?
No, it's not right. The check is needed to tell that properties supplied 0.
> > chip_err(gc, "tried to insert a GPIO chip with zero lines\n"); > > ret = -EINVAL; > > goto err_free_descs; > > } > > > > ? > > > > > + gc->ngpio = ngpios; > > > } > > > > > > if (gc->ngpio > FASTPATH_NGPIO)
> I suggest the following:
It's buggy as submitted version (I actually haven't found any difference in the code, but comments).
You see, I propose less changes and straight forward logic:
1. Check if the supplied ->ngpio equal to 0 2. If so, try device properties 2.1. If there is no property found, make sure we a) don't use uninitialized variable, b) we don't change ->ngpio, so it stays 0 2.2. If there is an error, return it as is to the caller 2.3. Assign ->ngpio by value from property (which very well may be 0!) 3. Check ->ngpio for 0 again, if so, issue a message and return -EINVAL to the user.
We have three places where ->ngpio can be 0, all of them I covered.
-- With Best Regards, Andy Shevchenko
| |