[PATCH v1 1/1] gpiolib: Unify access to the device properties

[PATCH v1 1/1] gpiolib: Unify access to the device properties

[Andy Shevchenko]

Some of the functions are using struct fwnode_handle, some struct device
pointer. In the GPIO library the firmware node of the GPIO device is the
same as GPIO node of the GPIO chip. Due to this fact we may use former
to access properties everywhere in the code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51afdc6ac919..c163b354e727 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -367,12 +367,12 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 static int devprop_gpiochip_set_names(struct gpio_chip *chip)
 {
 	struct gpio_device *gdev = chip->gpiodev;
-	const struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
+	struct device *dev = &gdev->dev;
 	const char **names;
 	int ret, i;
 	int count;
 
-	count = fwnode_property_string_array_count(fwnode, "gpio-line-names");
+	count = device_property_string_array_count(dev, "gpio-line-names");
 	if (count < 0)
 		return 0;
 
@@ -385,7 +385,7 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
 	 * gpiochips.
 	 */
 	if (count <= chip->offset) {
-		dev_warn(&gdev->dev, "gpio-line-names too short (length %d), cannot map names for the gpiochip at offset %u\n",
+		dev_warn(dev, "gpio-line-names too short (length %d), cannot map names for the gpiochip at offset %u\n",
 			 count, chip->offset);
 		return 0;
 	}
@@ -394,10 +394,9 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
 	if (!names)
 		return -ENOMEM;
 
-	ret = fwnode_property_read_string_array(fwnode, "gpio-line-names",
-						names, count);
+	ret = device_property_read_string_array(dev, "gpio-line-names", names, count);
 	if (ret < 0) {
-		dev_warn(&gdev->dev, "failed to read GPIO line names\n");
+		dev_warn(dev, "failed to read GPIO line names\n");
 		kfree(names);
 		return ret;
 	}
@@ -448,10 +447,11 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc)
 
 static unsigned int gpiochip_count_reserved_ranges(struct gpio_chip *gc)
 {
+	struct device *dev = &gc->gpiodev->dev;
 	int size;
 
 	/* Format is "start, count, ..." */
-	size = fwnode_property_count_u32(gc->fwnode, "gpio-reserved-ranges");
+	size = device_property_count_u32(dev, "gpio-reserved-ranges");
 	if (size > 0 && size % 2 == 0)
 		return size;
 
@@ -472,6 +472,7 @@ static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
 
 static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc)
 {
+	struct device *dev = &gc->gpiodev->dev;
 	unsigned int size;
 	u32 *ranges;
 	int ret;
@@ -484,7 +485,7 @@ static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc)
 	if (!ranges)
 		return -ENOMEM;
 
-	ret = fwnode_property_read_u32_array(gc->fwnode, "gpio-reserved-ranges", ranges, size);
+	ret = device_property_read_u32_array(dev, "gpio-reserved-ranges", ranges, size);
 	if (ret) {
 		kfree(ranges);
 		return ret;

base-commit: 40059212f99c31f26c69763e560325e59eac02c6
-- 
2.35.1

Re: [PATCH v1 1/1] gpiolib: Unify access to the device properties

[Andy Shevchenko]

On Wed, Nov 16, 2022 at 04:17:28PM +0200, Andy Shevchenko wrote:
> Some of the functions are using struct fwnode_handle, some struct device
> pointer. In the GPIO library the firmware node of the GPIO device is the
> same as GPIO node of the GPIO chip. Due to this fact we may use former
> to access properties everywhere in the code.

Citing myself from another thread to the topic:

Nevertheless, for of_gpiochip_add()/of_gpiochip_remove() and
of_mm_gpiochip_add_data() I still left use of fwnode, because it feels
the right thing to do: we are taking reference on the input data in
such cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Unify access to the device properties

[Brian Masney]

On Wed, Nov 16, 2022 at 04:17:28PM +0200, Andy Shevchenko wrote:
> Some of the functions are using struct fwnode_handle, some struct device
> pointer. In the GPIO library the firmware node of the GPIO device is the
> same as GPIO node of the GPIO chip. Due to this fact we may use former
> to access properties everywhere in the code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Brian Masney <bmasney@redhat.com>

Re: [PATCH v1 1/1] gpiolib: Unify access to the device properties

[Marijn Suijten]

On 2022-11-16 16:17:28, Andy Shevchenko wrote:
> Some of the functions are using struct fwnode_handle, some struct device
> pointer. In the GPIO library the firmware node of the GPIO device is the
> same as GPIO node of the GPIO chip. Due to this fact we may use former
> to access properties everywhere in the code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I presume this is a more complete version of [1], as this also happens
to address the crash on MSM [1], hence:

Tested-by: Marijn Suijten <marijn.suijten@somainline.org>

Just in case, this seems to depend on [3] or it fails to apply cleanly
because of the `const` change on `fwnode` (consider this irrelevant if
it has already been applied, I'm still on -next-20221115 for this
integration branch).

[1]: https://lore.kernel.org/all/Y3S62i7OzocP5QrT@orome/
[2]: https://lore.kernel.org/linux-arm-msm/20221115110800.35gl3j43lmbxm3jb@SoMainline.org/
[3]: https://lore.kernel.org/linux-gpio/20221031-gpiolib-swnode-v3-0-0282162b0fa4@gmail.com/

- Marijn

> ---
>  drivers/gpio/gpiolib.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 51afdc6ac919..c163b354e727 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -367,12 +367,12 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
>  static int devprop_gpiochip_set_names(struct gpio_chip *chip)
>  {
>  	struct gpio_device *gdev = chip->gpiodev;
> -	const struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
> +	struct device *dev = &gdev->dev;
>  	const char **names;
>  	int ret, i;
>  	int count;
>  
> -	count = fwnode_property_string_array_count(fwnode, "gpio-line-names");
> +	count = device_property_string_array_count(dev, "gpio-line-names");
>  	if (count < 0)
>  		return 0;
>  
> @@ -385,7 +385,7 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
>  	 * gpiochips.
>  	 */
>  	if (count <= chip->offset) {
> -		dev_warn(&gdev->dev, "gpio-line-names too short (length %d), cannot map names for the gpiochip at offset %u\n",
> +		dev_warn(dev, "gpio-line-names too short (length %d), cannot map names for the gpiochip at offset %u\n",
>  			 count, chip->offset);
>  		return 0;
>  	}
> @@ -394,10 +394,9 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
>  	if (!names)
>  		return -ENOMEM;
>  
> -	ret = fwnode_property_read_string_array(fwnode, "gpio-line-names",
> -						names, count);
> +	ret = device_property_read_string_array(dev, "gpio-line-names", names, count);
>  	if (ret < 0) {
> -		dev_warn(&gdev->dev, "failed to read GPIO line names\n");
> +		dev_warn(dev, "failed to read GPIO line names\n");
>  		kfree(names);
>  		return ret;
>  	}
> @@ -448,10 +447,11 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc)
>  
>  static unsigned int gpiochip_count_reserved_ranges(struct gpio_chip *gc)
>  {
> +	struct device *dev = &gc->gpiodev->dev;
>  	int size;
>  
>  	/* Format is "start, count, ..." */
> -	size = fwnode_property_count_u32(gc->fwnode, "gpio-reserved-ranges");
> +	size = device_property_count_u32(dev, "gpio-reserved-ranges");
>  	if (size > 0 && size % 2 == 0)
>  		return size;
>  
> @@ -472,6 +472,7 @@ static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
>  
>  static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc)
>  {
> +	struct device *dev = &gc->gpiodev->dev;
>  	unsigned int size;
>  	u32 *ranges;
>  	int ret;
> @@ -484,7 +485,7 @@ static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc)
>  	if (!ranges)
>  		return -ENOMEM;
>  
> -	ret = fwnode_property_read_u32_array(gc->fwnode, "gpio-reserved-ranges", ranges, size);
> +	ret = device_property_read_u32_array(dev, "gpio-reserved-ranges", ranges, size);
>  	if (ret) {
>  		kfree(ranges);
>  		return ret;
> 
> base-commit: 40059212f99c31f26c69763e560325e59eac02c6
> -- 
> 2.35.1
> 

Re: [PATCH v1 1/1] gpiolib: Unify access to the device properties

[Andy Shevchenko]

On Sun, Nov 20, 2022 at 08:16:22PM +0100, Marijn Suijten wrote:
> On 2022-11-16 16:17:28, Andy Shevchenko wrote:
> > Some of the functions are using struct fwnode_handle, some struct device
> > pointer. In the GPIO library the firmware node of the GPIO device is the
> > same as GPIO node of the GPIO chip. Due to this fact we may use former
> > to access properties everywhere in the code.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I presume this is a more complete version of [1], as this also happens
> to address the crash on MSM [1], hence:
> 
> Tested-by: Marijn Suijten <marijn.suijten@somainline.org>

Thank you!

> Just in case, this seems to depend on [3] or it fails to apply cleanly
> because of the `const` change on `fwnode` (consider this irrelevant if
> it has already been applied, I'm still on -next-20221115 for this
> integration branch).

But initial issue was induced by (my) patch which is only in gpio/for-next.
It's supposed to be only Linux Next issue.

> [1]: https://lore.kernel.org/all/Y3S62i7OzocP5QrT@orome/
> [2]: https://lore.kernel.org/linux-arm-msm/20221115110800.35gl3j43lmbxm3jb@SoMainline.org/
> [3]: https://lore.kernel.org/linux-gpio/20221031-gpiolib-swnode-v3-0-0282162b0fa4@gmail.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Unify access to the device properties

[Andy Shevchenko]

On Wed, Nov 16, 2022 at 04:17:28PM +0200, Andy Shevchenko wrote:
> Some of the functions are using struct fwnode_handle, some struct device
> pointer. In the GPIO library the firmware node of the GPIO device is the
> same as GPIO node of the GPIO chip. Due to this fact we may use former
> to access properties everywhere in the code.

Bart, can this be applied?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Unify access to the device properties

[Bartosz Golaszewski]

On Fri, Nov 25, 2022 at 7:30 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Nov 16, 2022 at 04:17:28PM +0200, Andy Shevchenko wrote:
> > Some of the functions are using struct fwnode_handle, some struct device
> > pointer. In the GPIO library the firmware node of the GPIO device is the
> > same as GPIO node of the GPIO chip. Due to this fact we may use former
> > to access properties everywhere in the code.
>
> Bart, can this be applied?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Sure, now applied. Got carried away with this use-after-free from
userspace problem.

Bart

Re: [PATCH v1 1/1] gpiolib: Unify access to the device properties

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 07:01:42PM +0100, Bartosz Golaszewski wrote:
> On Fri, Nov 25, 2022 at 7:30 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Nov 16, 2022 at 04:17:28PM +0200, Andy Shevchenko wrote:

...

> > Bart, can this be applied?
> 
> Sure, now applied. Got carried away with this use-after-free from
> userspace problem.

Thank you!

-- 
With Best Regards,
Andy Shevchenko