| Message ID | 20221107161027.43384-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/2] gpiolib: of: Prepare of_gpiochip_add() / of_gpiochip_remove() for fwnode | expand |
On Mon, Nov 07, 2022 at 06:10:26PM +0200, Andy Shevchenko wrote: > GPIO library is getting rid of of_node, fwnode should be utilized instead. > Prepare of_gpiochip_add() / of_gpiochip_remove() for fwnode. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/gpio/gpiolib-of.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index be9c34cca322..000020eb78d8 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -1104,9 +1104,11 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip) { return 0; } > > int of_gpiochip_add(struct gpio_chip *chip) > { > + struct device_node *np; > int ret; > > - if (!chip->of_node) > + np = to_of_node(chip->fwnode); > + if (!np) > return 0; > > if (!chip->of_xlate) { > @@ -1123,18 +1125,18 @@ int of_gpiochip_add(struct gpio_chip *chip) > if (ret) > return ret; > > - of_node_get(chip->of_node); > + fwnode_handle_get(chip->fwnode); > > ret = of_gpiochip_scan_gpios(chip); > if (ret) > - of_node_put(chip->of_node); > + fwnode_handle_put(chip->fwnode); > > return ret; > } > > void of_gpiochip_remove(struct gpio_chip *chip) > { > - of_node_put(chip->of_node); > + fwnode_handle_put(chip->fwnode); > } > > void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev) > -- > 2.35.1 >
On Mon, Nov 07, 2022 at 10:20:37AM -0800, Dmitry Torokhov wrote: > On Mon, Nov 07, 2022 at 06:10:27PM +0200, Andy Shevchenko wrote: > > +static unsigned int gpiochip_count_reserved_ranges(struct gpio_chip *gc) > > +{ > > + int size; > > + > > + size = fwnode_property_count_u32(gc->fwnode, "gpio-reserved-ranges"); > > I wonder if a comment why we need even size would not be helpful. Was it in the original code? Anyway, if Bart thinks so as well, I may add it in v2. > > + if (size > 0 && size % 2 == 0) > > + return size; > > + > > + return 0; > > +} > > + > > static int gpiochip_alloc_valid_mask(struct gpio_chip *gc) > > { > > - if (!(of_gpio_need_valid_mask(gc) || gc->init_valid_mask)) > > + if (!(gpiochip_count_reserved_ranges(gc) || gc->init_valid_mask)) > > return 0; > > > > gc->valid_mask = gpiochip_allocate_mask(gc); > > @@ -457,8 +468,47 @@ static int gpiochip_alloc_valid_mask(struct gpio_chip *gc) > > return 0; > > } > > > > +static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc, unsigned int sz) > > +{ > > + u32 *ranges; > > + int ret; > > + > > + ranges = kmalloc_array(sz, sizeof(*ranges), GFP_KERNEL); > > + if (!ranges) > > + return -ENOMEM; > > + > > + ret = fwnode_property_read_u32_array(gc->fwnode, "gpio-reserved-ranges", ranges, sz); > > + if (ret) { > > + kfree(ranges); > > + return ret; > > + } > > + > > + while (sz) { > > + u32 count = ranges[--sz]; > > + u32 start = ranges[--sz]; > > I know we checked sz validity, but I wonder if re-checking it in this > function would not insulate us from errors creeping in after some other > code refactoring. I'm not sure I understand what you meant. The fwnode_property_read_u32_array() will fail if the given sz is too big for the real data, so while (sz) would never even go on the invalid data. > In any case, > > Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Thank you!
On Mon, Nov 07, 2022 at 11:09:19PM +0200, Andy Shevchenko wrote: > On Mon, Nov 07, 2022 at 10:20:37AM -0800, Dmitry Torokhov wrote: > > On Mon, Nov 07, 2022 at 06:10:27PM +0200, Andy Shevchenko wrote: > > > +static unsigned int gpiochip_count_reserved_ranges(struct gpio_chip *gc) > > > +{ > > > + int size; > > > + > > > + size = fwnode_property_count_u32(gc->fwnode, "gpio-reserved-ranges"); > > > > I wonder if a comment why we need even size would not be helpful. > > Was it in the original code? > Anyway, if Bart thinks so as well, I may add it in v2. > > > > + if (size > 0 && size % 2 == 0) > > > + return size; > > > + > > > + return 0; > > > +} > > > + > > > static int gpiochip_alloc_valid_mask(struct gpio_chip *gc) > > > { > > > - if (!(of_gpio_need_valid_mask(gc) || gc->init_valid_mask)) > > > + if (!(gpiochip_count_reserved_ranges(gc) || gc->init_valid_mask)) > > > return 0; > > > > > > gc->valid_mask = gpiochip_allocate_mask(gc); > > > @@ -457,8 +468,47 @@ static int gpiochip_alloc_valid_mask(struct gpio_chip *gc) > > > return 0; > > > } > > > > > > +static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc, unsigned int sz) > > > +{ > > > + u32 *ranges; > > > + int ret; > > > + > > > + ranges = kmalloc_array(sz, sizeof(*ranges), GFP_KERNEL); > > > + if (!ranges) > > > + return -ENOMEM; > > > + > > > + ret = fwnode_property_read_u32_array(gc->fwnode, "gpio-reserved-ranges", ranges, sz); > > > + if (ret) { > > > + kfree(ranges); > > > + return ret; > > > + } > > > + > > > + while (sz) { > > > + u32 count = ranges[--sz]; > > > + u32 start = ranges[--sz]; > > > > I know we checked sz validity, but I wonder if re-checking it in this > > function would not insulate us from errors creeping in after some other > > code refactoring. > > I'm not sure I understand what you meant. The fwnode_property_read_u32_array() > will fail if the given sz is too big for the real data, so while (sz) would > never even go on the invalid data. I am more worried about sz being odd and the loop ending up trying to dereference ranges[-1]. Thanks.
On Mon, Nov 07, 2022 at 01:40:53PM -0800, Dmitry Torokhov wrote: > On Mon, Nov 07, 2022 at 11:09:19PM +0200, Andy Shevchenko wrote: > > On Mon, Nov 07, 2022 at 10:20:37AM -0800, Dmitry Torokhov wrote: > > > On Mon, Nov 07, 2022 at 06:10:27PM +0200, Andy Shevchenko wrote: ... > > > > +static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc, unsigned int sz) > > > > +{ > > > > + u32 *ranges; > > > > + int ret; > > > > + > > > > + ranges = kmalloc_array(sz, sizeof(*ranges), GFP_KERNEL); > > > > + if (!ranges) > > > > + return -ENOMEM; > > > > + > > > > + ret = fwnode_property_read_u32_array(gc->fwnode, "gpio-reserved-ranges", ranges, sz); > > > > + if (ret) { > > > > + kfree(ranges); > > > > + return ret; > > > > + } > > > > + > > > > + while (sz) { > > > > + u32 count = ranges[--sz]; > > > > + u32 start = ranges[--sz]; > > > > > > I know we checked sz validity, but I wonder if re-checking it in this > > > function would not insulate us from errors creeping in after some other > > > code refactoring. > > > > I'm not sure I understand what you meant. The fwnode_property_read_u32_array() > > will fail if the given sz is too big for the real data, so while (sz) would > > never even go on the invalid data. > > I am more worried about sz being odd and the loop ending up trying to > dereference ranges[-1]. I see. What if we take amount of ranges as the parameter and convert to size by multiplying by 2?
On Tue, Nov 08, 2022 at 10:41:43AM +0200, Andy Shevchenko wrote: > On Mon, Nov 07, 2022 at 01:40:53PM -0800, Dmitry Torokhov wrote: > > On Mon, Nov 07, 2022 at 11:09:19PM +0200, Andy Shevchenko wrote: ... > > I am more worried about sz being odd and the loop ending up trying to > > dereference ranges[-1]. > > I see. What if we take amount of ranges as the parameter and convert to size by > multiplying by 2? Okay, I found a way how to avoid additional churn and add validation. I will incorporate that in v2 with your tags.
On Mon, Nov 7, 2022 at 5:10 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > GPIO library is getting rid of of_node, fwnode should be utilized instead. > Prepare of_gpiochip_add() / of_gpiochip_remove() for fwnode. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib-of.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index be9c34cca322..000020eb78d8 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -1104,9 +1104,11 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip) { return 0; } > > int of_gpiochip_add(struct gpio_chip *chip) > { > + struct device_node *np; > int ret; > > - if (!chip->of_node) > + np = to_of_node(chip->fwnode); > + if (!np) > return 0; > > if (!chip->of_xlate) { > @@ -1123,18 +1125,18 @@ int of_gpiochip_add(struct gpio_chip *chip) > if (ret) > return ret; > > - of_node_get(chip->of_node); > + fwnode_handle_get(chip->fwnode); > > ret = of_gpiochip_scan_gpios(chip); > if (ret) > - of_node_put(chip->of_node); > + fwnode_handle_put(chip->fwnode); > > return ret; > } > > void of_gpiochip_remove(struct gpio_chip *chip) > { > - of_node_put(chip->of_node); > + fwnode_handle_put(chip->fwnode); > } > > void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev) > -- > 2.35.1 > Applied, thanks! Bart
On Wed, Nov 09, 2022 at 02:13:55PM +0100, Bartosz Golaszewski wrote: > On Wed, Nov 9, 2022 at 2:13 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Mon, Nov 7, 2022 at 5:10 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: ... > > Applied, thanks! > > > I actually applied v2 and both the patches from this series. Thank you!
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index be9c34cca322..000020eb78d8 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -1104,9 +1104,11 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip) { return 0; } int of_gpiochip_add(struct gpio_chip *chip) { + struct device_node *np; int ret; - if (!chip->of_node) + np = to_of_node(chip->fwnode); + if (!np) return 0; if (!chip->of_xlate) { @@ -1123,18 +1125,18 @@ int of_gpiochip_add(struct gpio_chip *chip) if (ret) return ret; - of_node_get(chip->of_node); + fwnode_handle_get(chip->fwnode); ret = of_gpiochip_scan_gpios(chip); if (ret) - of_node_put(chip->of_node); + fwnode_handle_put(chip->fwnode); return ret; } void of_gpiochip_remove(struct gpio_chip *chip) { - of_node_put(chip->of_node); + fwnode_handle_put(chip->fwnode); } void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev)
GPIO library is getting rid of of_node, fwnode should be utilized instead. Prepare of_gpiochip_add() / of_gpiochip_remove() for fwnode. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-of.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)