[PATCH 0/3] gpiolib: generalize GPIO line names property

[PATCH 0/3] gpiolib: generalize GPIO line names property

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I initially sent this as part of the gpio-mockup overhaul but since
these patches are indepentent and the work on gpio-mockup may become
more complicated - I'm sending these separately.

The only change is adding additional property helpers to count strings
in array.

Bartosz Golaszewski (3):
  device: property: add helpers to count items in string arrays
  gpiolib: generalize devprop_gpiochip_set_names() for device properties
  gpiolib: unexport devprop_gpiochip_set_names()

 drivers/gpio/Makefile          |  1 -
 drivers/gpio/gpiolib-acpi.c    |  3 --
 drivers/gpio/gpiolib-devprop.c | 20 ++++++-------
 drivers/gpio/gpiolib-of.c      |  5 ----
 drivers/gpio/gpiolib.c         | 55 +++++++++++++++++++++++++++++++---
 include/linux/gpio/driver.h    |  3 --
 include/linux/property.h       | 13 ++++++++
 7 files changed, 74 insertions(+), 26 deletions(-)

-- 
2.26.1

[PATCH 1/3] device: property: add helpers to count items in string arrays

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Instead of doing the following:

    count = device_property_read_string_array(dev, propname, NULL, 0);

Let's provide inline helpers with hardcoded arguments for counting
strings in property arrays.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/property.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 9f805c442819..1fa5e250a8ea 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -170,6 +170,12 @@ static inline int device_property_count_u64(struct device *dev, const char *prop
 	return device_property_read_u64_array(dev, propname, NULL, 0);
 }
 
+static inline int device_property_count_strings(struct device *dev,
+						const char *propname)
+{
+	return device_property_read_string_array(dev, propname, NULL, 0);
+}
+
 static inline bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
 					     const char *propname)
 {
@@ -224,6 +230,13 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
 	return fwnode_property_read_u64_array(fwnode, propname, NULL, 0);
 }
 
+static inline int
+fwnode_property_count_strings(const struct fwnode_handle *fwnode,
+			      const char *propname)
+{
+	return fwnode_property_read_string_array(fwnode, propname, NULL, 0);
+}
+
 struct software_node;
 
 /**
-- 
2.26.1

[PATCH 2/3] gpiolib: generalize devprop_gpiochip_set_names() for device properties

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

devprop_gpiochip_set_names() is overly complicated with taking the
fwnode argument (which requires using dev_fwnode() & of_fwnode_handle()
in ACPI and OF GPIO code respectively). Let's just switch to using the
generic device properties.

This allows us to pull the code setting line names directly into
gpiochip_add_data_with_key() instead of handling it separately for
ACPI and OF.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib-acpi.c    |  3 ---
 drivers/gpio/gpiolib-devprop.c | 20 ++++++++++----------
 drivers/gpio/gpiolib-of.c      |  5 -----
 drivers/gpio/gpiolib.c         |  8 ++++----
 include/linux/gpio/driver.h    |  3 +--
 5 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 54ca3c18b291..834a12f3219e 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1221,9 +1221,6 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
 		return;
 	}
 
-	if (!chip->names)
-		devprop_gpiochip_set_names(chip, dev_fwnode(chip->parent));
-
 	acpi_gpiochip_request_regions(acpi_gpio);
 	acpi_gpiochip_scan_gpios(acpi_gpio);
 	acpi_walk_dep_device_list(handle);
diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c
index 26741032fa9e..29ee00d7730e 100644
--- a/drivers/gpio/gpiolib-devprop.c
+++ b/drivers/gpio/gpiolib-devprop.c
@@ -17,25 +17,23 @@
 /**
  * devprop_gpiochip_set_names - Set GPIO line names using device properties
  * @chip: GPIO chip whose lines should be named, if possible
- * @fwnode: Property Node containing the gpio-line-names property
  *
  * Looks for device property "gpio-line-names" and if it exists assigns
  * GPIO line names for the chip. The memory allocated for the assigned
- * names belong to the underlying firmware node and should not be released
+ * names belong to the underlying software node and should not be released
  * by the caller.
  */
-void devprop_gpiochip_set_names(struct gpio_chip *chip,
-				const struct fwnode_handle *fwnode)
+int devprop_gpiochip_set_names(struct gpio_chip *chip)
 {
 	struct gpio_device *gdev = chip->gpiodev;
+	struct device *dev = chip->parent;
 	const char **names;
 	int ret, i;
 	int count;
 
-	count = fwnode_property_read_string_array(fwnode, "gpio-line-names",
-						  NULL, 0);
+	count = device_property_count_strings(dev, "gpio-line-names");
 	if (count < 0)
-		return;
+		return 0;
 
 	if (count > gdev->ngpio) {
 		dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d",
@@ -45,19 +43,21 @@ void devprop_gpiochip_set_names(struct gpio_chip *chip,
 
 	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
 	if (!names)
-		return;
+		return -ENOMEM;
 
-	ret = fwnode_property_read_string_array(fwnode, "gpio-line-names",
+	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");
 		kfree(names);
-		return;
+		return ret;
 	}
 
 	for (i = 0; i < count; i++)
 		gdev->descs[i].name = names[i];
 
 	kfree(names);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(devprop_gpiochip_set_names);
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index bd31dd3b6a75..2f895a2b8411 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -1026,11 +1026,6 @@ int of_gpiochip_add(struct gpio_chip *chip)
 	if (ret)
 		return ret;
 
-	/* If the chip defines names itself, these take precedence */
-	if (!chip->names)
-		devprop_gpiochip_set_names(chip,
-					   of_fwnode_handle(chip->of_node));
-
 	of_node_get(chip->of_node);
 
 	ret = of_gpiochip_scan_gpios(chip);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 80137c1b3cdc..0d390f0ec32c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -340,9 +340,6 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 	struct gpio_device *gdev = gc->gpiodev;
 	int i;
 
-	if (!gc->names)
-		return 0;
-
 	/* First check all names if they are unique */
 	for (i = 0; i != gc->ngpio; ++i) {
 		struct gpio_desc *gpio;
@@ -621,7 +618,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	INIT_LIST_HEAD(&gdev->pin_ranges);
 #endif
 
-	ret = gpiochip_set_desc_names(gc);
+	if (gc->names)
+		ret = gpiochip_set_desc_names(gc);
+	else
+		ret = devprop_gpiochip_set_names(gc);
 	if (ret)
 		goto err_remove_from_list;
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index d1cef5c2715c..56485a040b82 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -756,8 +756,7 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    enum gpiod_flags dflags);
 void gpiochip_free_own_desc(struct gpio_desc *desc);
 
-void devprop_gpiochip_set_names(struct gpio_chip *gc,
-				const struct fwnode_handle *fwnode);
+int devprop_gpiochip_set_names(struct gpio_chip *gc);
 
 #ifdef CONFIG_GPIOLIB
 
-- 
2.26.1

[PATCH 3/3] gpiolib: unexport devprop_gpiochip_set_names()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Now that devprop_gpiochip_set_names() is only used in a single place
inside drivers/gpio/gpiolib.c, there's no need anymore for it to be
exported or to even live in its own source file. Pull this function into
the core source file for gpiolib.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/Makefile       |  1 -
 drivers/gpio/gpiolib.c      | 47 +++++++++++++++++++++++++++++++++++++
 include/linux/gpio/driver.h |  2 --
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 4f9abff4f2dc..639275eb4e4d 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -6,7 +6,6 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
 obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-devres.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-legacy.o
-obj-$(CONFIG_GPIOLIB)		+= gpiolib-devprop.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-cdev.o
 obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
 obj-$(CONFIG_GPIO_SYSFS)	+= gpiolib-sysfs.o
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0d390f0ec32c..2b2526c5de51 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -358,6 +358,53 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 	return 0;
 }
 
+/*
+ * devprop_gpiochip_set_names - Set GPIO line names using device properties
+ * @chip: GPIO chip whose lines should be named, if possible
+ *
+ * Looks for device property "gpio-line-names" and if it exists assigns
+ * GPIO line names for the chip. The memory allocated for the assigned
+ * names belong to the underlying software node and should not be released
+ * by the caller.
+ */
+static int devprop_gpiochip_set_names(struct gpio_chip *chip)
+{
+	struct gpio_device *gdev = chip->gpiodev;
+	struct device *dev = chip->parent;
+	const char **names;
+	int ret, i;
+	int count;
+
+	count = device_property_count_strings(dev, "gpio-line-names");
+	if (count < 0)
+		return 0;
+
+	if (count > gdev->ngpio) {
+		dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d",
+			 count, gdev->ngpio);
+		count = gdev->ngpio;
+	}
+
+	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
+	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");
+		kfree(names);
+		return ret;
+	}
+
+	for (i = 0; i < count; i++)
+		gdev->descs[i].name = names[i];
+
+	kfree(names);
+
+	return 0;
+}
+
 static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc)
 {
 	unsigned long *p;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 56485a040b82..4a7e295c3640 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -756,8 +756,6 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    enum gpiod_flags dflags);
 void gpiochip_free_own_desc(struct gpio_desc *desc);
 
-int devprop_gpiochip_set_names(struct gpio_chip *gc);
-
 #ifdef CONFIG_GPIOLIB
 
 /* lock/unlock as IRQ */
-- 
2.26.1

Re: [PATCH 1/3] device: property: add helpers to count items in string arrays

[Andy Shevchenko]

On Tue, Sep 08, 2020 at 02:58:11PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Instead of doing the following:
> 
>     count = device_property_read_string_array(dev, propname, NULL, 0);
> 
> Let's provide inline helpers with hardcoded arguments for counting
> strings in property arrays.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks!

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/property.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 9f805c442819..1fa5e250a8ea 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -170,6 +170,12 @@ static inline int device_property_count_u64(struct device *dev, const char *prop
>  	return device_property_read_u64_array(dev, propname, NULL, 0);
>  }
>  
> +static inline int device_property_count_strings(struct device *dev,
> +						const char *propname)
> +{
> +	return device_property_read_string_array(dev, propname, NULL, 0);
> +}
> +
>  static inline bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
>  					     const char *propname)
>  {
> @@ -224,6 +230,13 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
>  	return fwnode_property_read_u64_array(fwnode, propname, NULL, 0);
>  }
>  
> +static inline int
> +fwnode_property_count_strings(const struct fwnode_handle *fwnode,
> +			      const char *propname)
> +{
> +	return fwnode_property_read_string_array(fwnode, propname, NULL, 0);
> +}
> +
>  struct software_node;
>  
>  /**
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] gpiolib: unexport devprop_gpiochip_set_names()

[Andy Shevchenko]

On Tue, Sep 08, 2020 at 02:58:13PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Now that devprop_gpiochip_set_names() is only used in a single place
> inside drivers/gpio/gpiolib.c, there's no need anymore for it to be
> exported or to even live in its own source file. Pull this function into
> the core source file for gpiolib.

Did you miss to remove old file?

>  drivers/gpio/Makefile       |  1 -
>  drivers/gpio/gpiolib.c      | 47 +++++++++++++++++++++++++++++++++++++
>  include/linux/gpio/driver.h |  2 --

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] gpiolib: unexport devprop_gpiochip_set_names()

[Bartosz Golaszewski]

On Tue, Sep 8, 2020 at 3:52 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 08, 2020 at 02:58:13PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Now that devprop_gpiochip_set_names() is only used in a single place
> > inside drivers/gpio/gpiolib.c, there's no need anymore for it to be
> > exported or to even live in its own source file. Pull this function into
> > the core source file for gpiolib.
>
> Did you miss to remove old file?
>
> >  drivers/gpio/Makefile       |  1 -
> >  drivers/gpio/gpiolib.c      | 47 +++++++++++++++++++++++++++++++++++++
> >  include/linux/gpio/driver.h |  2 --
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Arr yes, rebase failed me. :(

Bart

Re: [PATCH 0/3] gpiolib: generalize GPIO line names property

[Anders Roxell]

On Tue, 8 Sep 2020 at 18:40, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> I initially sent this as part of the gpio-mockup overhaul but since
> these patches are indepentent and the work on gpio-mockup may become
> more complicated - I'm sending these separately.
>
> The only change is adding additional property helpers to count strings
> in array.
>
> Bartosz Golaszewski (3):
>   device: property: add helpers to count items in string arrays
>   gpiolib: generalize devprop_gpiochip_set_names() for device properties
>   gpiolib: unexport devprop_gpiochip_set_names()

I do an arm64 allmodconfig build fron linux-next (tag: next-20200915) and
run that in qemu. When I run I see the following output (see full log [1]):
"BUG: KASAN: null-ptr-deref in device_property_read_string_array".


[ 6186.339462][    T1] unittest-gpio
testcase-data:overlay-node:test-bus:gpio@0: no pinctrl handle
[ 6186.346148][    T1] gpiochip_find_base: found new base at 507
[ 6186.348684][    T1]
==================================================================
[ 6186.351563][    T1] BUG: KASAN: null-ptr-deref in
device_property_read_string_array+0x40/0xa0
[ 6186.355157][    T1] Read of size 8 at addr 0000000000000570 by task
swapper/0/1
[ 6186.358212][    T1]
[ 6186.359361][    T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G
W         5.9.0-rc5-next-20200915-00006-g104c8fe4916b #1
[ 6186.363877][    T1] Hardware name: linux,dummy-virt (DT)
[ 6186.366156][    T1] Call trace:
[ 6186.367540][    T1]  dump_backtrace+0x0/0x320
[ 6186.369446][    T1]  show_stack+0x38/0x60
[ 6186.371282][    T1]  dump_stack+0x1d4/0x278
[ 6186.373193][    T1]  __kasan_report+0x148/0x180
[ 6186.375265][    T1]  kasan_report+0x44/0xe0
[ 6186.377168][    T1]  __asan_load8+0xbc/0xe0
[ 6186.379069][    T1]  device_property_read_string_array+0x40/0xa0
[ 6186.381741][    T1]  devprop_gpiochip_set_names.isra.0+0x4c/0x200
[ 6186.384394][    T1]  gpiochip_add_data_with_key+0x75c/0xf80
[ 6186.386876][    T1]  unittest_gpio_probe+0xf4/0x1e0
[ 6186.389049][    T1]  platform_drv_probe+0xac/0x160
[ 6186.391184][    T1]  really_probe+0x430/0xaa0
[ 6186.393136][    T1]  really_probe_debug+0x3c/0xe0
[ 6186.395238][    T1]  driver_probe_device+0x134/0x1c0
[ 6186.397443][    T1]  device_driver_attach+0xec/0x180
[ 6186.399639][    T1]  __driver_attach+0x1f0/0x220
[ 6186.401718][    T1]  bus_for_each_dev+0x104/0x1c0
[ 6186.403796][    T1]  driver_attach+0x44/0x60
[ 6186.405731][    T1]  bus_add_driver+0x214/0x3c0
[ 6186.407745][    T1]  driver_register+0x1a8/0x240
[ 6186.409835][    T1]  __platform_driver_register+0x90/0xa0
[ 6186.412207][    T1]  of_unittest_overlay_gpio+0x20c/0x7cc
[ 6186.414595][    T1]  of_unittest_overlay+0x748/0x7c0
[ 6186.416810][    T1]  of_unittest+0x148/0x184
[ 6186.418732][    T1]  do_one_initcall+0xc4/0x280
[ 6186.420782][    T1]  do_initcalls+0x148/0x1ac
[ 6186.422758][    T1]  kernel_init_freeable+0x158/0x1a0
[ 6186.425023][    T1]  kernel_init+0x24/0x1f0
[ 6186.426938][    T1]  ret_from_fork+0x10/0x18
[ 6186.428894][    T1]
==================================================================
[ 6186.433241][    T1] Unable to handle kernel read from unreadable
memory at virtual address 0000000000000570
[ 6186.437207][    T1] Mem abort info:
[ 6186.438639][    T1]   ESR = 0x96000004
[ 6186.440536][    T1]   EC = 0x25: DABT (current EL), IL = 32 bits
[ 6186.442791][    T1]   SET = 0, FnV = 0
[ 6186.444660][    T1]   EA = 0, S1PTW = 0
[ 6186.446233][    T1] Data abort info:
[ 6186.447938][    T1]   ISV = 0, ISS = 0x00000004
[ 6186.449749][    T1]   CM = 0, WnR = 0
[ 6186.451222][    T1] [0000000000000570] user address but active_mm is swapper
[ 6186.454000][    T1] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[ 6186.456422][    T1] Modules linked in:
[ 6186.458232][    T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G    B
W         5.9.0-rc5-next-20200915-00006-g104c8fe4916b #1
[ 6186.462833][    T1] Hardware name: linux,dummy-virt (DT)
[ 6186.465170][    T1] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
[ 6186.467910][    T1] pc : device_property_read_string_array+0x40/0xa0
[ 6186.470653][    T1] lr : device_property_read_string_array+0x40/0xa0
[ 6186.473380][    T1] sp : ffff000069827770
[ 6186.475138][    T1] x29: ffff000069827770 x28: ffffa00014a2cc20
[ 6186.477806][    T1] x27: ffff000068794760 x26: ffff000068794800
[ 6186.480444][    T1] x25: ffff000068794000 x24: ffff0000674e1094
[ 6186.483107][    T1] x23: 0000000000000000 x22: 0000000000000000
[ 6186.485794][    T1] x21: ffffa00012d61ca0 x20: ffffa00012d61200
[ 6186.488457][    T1] x19: 0000000000000000 x18: 00000000000014b8
[ 6186.491100][    T1] x17: 00000000000014f8 x16: 0000000000001438
[ 6186.493779][    T1] x15: 00000000f1f1f1f1 x14: 0000000000000003
[ 6186.496405][    T1] x13: 00000000000ca688 x12: ffff80000d304e7b
[ 6186.499084][    T1] x11: 1fffe0000d304e7a x10: ffff80000d304e7a
[ 6186.501775][    T1] x9 : ffffa00012702b2c x8 : ffff0000698273d7
[ 6186.504409][    T1] x7 : 0000000000000001 x6 : 00007ffff2cfb186
[ 6186.507074][    T1] x5 : 0000000000000000 x4 : dfffa00000000000
[ 6186.509706][    T1] x3 : ffffa000126f85c4 x2 : 0000000000000007
[ 6186.512352][    T1] x1 : ffff00006981c040 x0 : 0000000000000001
[ 6186.515009][    T1] Call trace:
[ 6186.516511][    T1]  device_property_read_string_array+0x40/0xa0
[ 6186.519155][    T1]  devprop_gpiochip_set_names.isra.0+0x4c/0x200
[ 6186.521806][    T1]  gpiochip_add_data_with_key+0x75c/0xf80
[ 6186.524294][    T1]  unittest_gpio_probe+0xf4/0x1e0
[ 6186.526518][    T1]  platform_drv_probe+0xac/0x160
[ 6186.528632][    T1]  really_probe+0x430/0xaa0
[ 6186.530600][    T1]  really_probe_debug+0x3c/0xe0
[ 6186.532679][    T1]  driver_probe_device+0x134/0x1c0
[ 6186.534936][    T1]  device_driver_attach+0xec/0x180
[ 6186.537119][    T1]  __driver_attach+0x1f0/0x220
[ 6186.539182][    T1]  bus_for_each_dev+0x104/0x1c0
[ 6186.541315][    T1]  driver_attach+0x44/0x60
[ 6186.543233][    T1]  bus_add_driver+0x214/0x3c0
[ 6186.545307][    T1]  driver_register+0x1a8/0x240
[ 6186.547373][    T1]  __platform_driver_register+0x90/0xa0
[ 6186.549754][    T1]  of_unittest_overlay_gpio+0x20c/0x7cc
[ 6186.552105][    T1]  of_unittest_overlay+0x748/0x7c0
[ 6186.554272][    T1]  of_unittest+0x148/0x184
[ 6186.556193][    T1]  do_one_initcall+0xc4/0x280
[ 6186.558248][    T1]  do_initcalls+0x148/0x1ac
[ 6186.560227][    T1]  kernel_init_freeable+0x158/0x1a0
[ 6186.562492][    T1]  kernel_init+0x24/0x1f0
[ 6186.564395][    T1]  ret_from_fork+0x10/0x18
[ 6186.566404][    T1] Code: aa0303f7 97b54003 9115c260 97c3ca39 (f942ba74)
[ 6186.569375][    T1] ---[ end trace f489669ae669dad0 ]---
[ 6186.571688][    T1] Kernel panic - not syncing: Oops: Fatal exception
[ 6186.574448][    T1] Kernel Offset: disabled
[ 6186.576306][    T1] CPU features: 0x0240002,20002004
[ 6186.578453][    T1] Memory Limit: none
[ 6186.580215][    T1] ---[ end Kernel panic - not syncing: Oops:
Fatal exception ]---


Cheers,
Anders
[1] http://ix.io/2xDy

Re: [PATCH 0/3] gpiolib: generalize GPIO line names property

[Andy Shevchenko]

On Tue, Sep 15, 2020 at 02:01:56PM +0200, Anders Roxell wrote:
> On Tue, 8 Sep 2020 at 18:40, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > I initially sent this as part of the gpio-mockup overhaul but since
> > these patches are indepentent and the work on gpio-mockup may become
> > more complicated - I'm sending these separately.
> >
> > The only change is adding additional property helpers to count strings
> > in array.
> >
> > Bartosz Golaszewski (3):
> >   device: property: add helpers to count items in string arrays
> >   gpiolib: generalize devprop_gpiochip_set_names() for device properties
> >   gpiolib: unexport devprop_gpiochip_set_names()

Ha-ha, OF unittest is of_node centric. definitely there is no backed device.

Bart, it seems we are stuck with fwnode interface.

> [ 6186.379069][    T1]  device_property_read_string_array+0x40/0xa0
> [ 6186.381741][    T1]  devprop_gpiochip_set_names.isra.0+0x4c/0x200
> [ 6186.384394][    T1]  gpiochip_add_data_with_key+0x75c/0xf80
> [ 6186.386876][    T1]  unittest_gpio_probe+0xf4/0x1e0

> [ 6186.433241][    T1] Unable to handle kernel read from unreadable
> memory at virtual address 0000000000000570
> [ 6186.437207][    T1] Mem abort info:
> [ 6186.438639][    T1]   ESR = 0x96000004
> [ 6186.440536][    T1]   EC = 0x25: DABT (current EL), IL = 32 bits
> [ 6186.442791][    T1]   SET = 0, FnV = 0
> [ 6186.444660][    T1]   EA = 0, S1PTW = 0
> [ 6186.446233][    T1] Data abort info:
> [ 6186.447938][    T1]   ISV = 0, ISS = 0x00000004
> [ 6186.449749][    T1]   CM = 0, WnR = 0
> [ 6186.451222][    T1] [0000000000000570] user address but active_mm is swapper
> [ 6186.454000][    T1] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [ 6186.456422][    T1] Modules linked in:
> [ 6186.458232][    T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G    B
> W         5.9.0-rc5-next-20200915-00006-g104c8fe4916b #1
> [ 6186.462833][    T1] Hardware name: linux,dummy-virt (DT)
> [ 6186.465170][    T1] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
> [ 6186.467910][    T1] pc : device_property_read_string_array+0x40/0xa0
> [ 6186.470653][    T1] lr : device_property_read_string_array+0x40/0xa0

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] gpiolib: generalize GPIO line names property

[Bartosz Golaszewski]

On Tue, Sep 15, 2020 at 3:12 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 15, 2020 at 02:01:56PM +0200, Anders Roxell wrote:
> > On Tue, 8 Sep 2020 at 18:40, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > I initially sent this as part of the gpio-mockup overhaul but since
> > > these patches are indepentent and the work on gpio-mockup may become
> > > more complicated - I'm sending these separately.
> > >
> > > The only change is adding additional property helpers to count strings
> > > in array.
> > >
> > > Bartosz Golaszewski (3):
> > >   device: property: add helpers to count items in string arrays
> > >   gpiolib: generalize devprop_gpiochip_set_names() for device properties
> > >   gpiolib: unexport devprop_gpiochip_set_names()
>
> Ha-ha, OF unittest is of_node centric. definitely there is no backed device.
>
> Bart, it seems we are stuck with fwnode interface.
>

Wait what?! This means the implementation is wrong - the whole concept
of device properties is to be generic and to hide the underlying
fwnode or OF properties. If anything we should fix
device/base/property.c to fall back to OF.

What is happening exactly? If all fwnode code compiled out?

I'll try to give it a spin and see what can be done but I don't like
that device_property_* functions fail if you have OF but not fwnode.

Bart

Re: [PATCH 0/3] gpiolib: generalize GPIO line names property

[Andy Shevchenko]

On Tue, Sep 15, 2020 at 04:12:28PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 15, 2020 at 02:01:56PM +0200, Anders Roxell wrote:
> > On Tue, 8 Sep 2020 at 18:40, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > I initially sent this as part of the gpio-mockup overhaul but since
> > > these patches are indepentent and the work on gpio-mockup may become
> > > more complicated - I'm sending these separately.
> > >
> > > The only change is adding additional property helpers to count strings
> > > in array.
> > >
> > > Bartosz Golaszewski (3):
> > >   device: property: add helpers to count items in string arrays
> > >   gpiolib: generalize devprop_gpiochip_set_names() for device properties
> > >   gpiolib: unexport devprop_gpiochip_set_names()
> 
> Ha-ha, OF unittest is of_node centric. definitely there is no backed device.
> 
> Bart, it seems we are stuck with fwnode interface.

Hmm... There is a platform device. So, it means that it fails along these
lines:
  return IS_ENABLED(CONFIG_OF) && dev->of_node ? &dev->of_node->fwnode : dev->fwnode;
so, who should set fwnode for of_node?

> > [ 6186.379069][    T1]  device_property_read_string_array+0x40/0xa0
> > [ 6186.381741][    T1]  devprop_gpiochip_set_names.isra.0+0x4c/0x200
> > [ 6186.384394][    T1]  gpiochip_add_data_with_key+0x75c/0xf80
> > [ 6186.386876][    T1]  unittest_gpio_probe+0xf4/0x1e0
> 
> > [ 6186.433241][    T1] Unable to handle kernel read from unreadable
> > memory at virtual address 0000000000000570
> > [ 6186.437207][    T1] Mem abort info:
> > [ 6186.438639][    T1]   ESR = 0x96000004
> > [ 6186.440536][    T1]   EC = 0x25: DABT (current EL), IL = 32 bits
> > [ 6186.442791][    T1]   SET = 0, FnV = 0
> > [ 6186.444660][    T1]   EA = 0, S1PTW = 0
> > [ 6186.446233][    T1] Data abort info:
> > [ 6186.447938][    T1]   ISV = 0, ISS = 0x00000004
> > [ 6186.449749][    T1]   CM = 0, WnR = 0
> > [ 6186.451222][    T1] [0000000000000570] user address but active_mm is swapper
> > [ 6186.454000][    T1] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > [ 6186.456422][    T1] Modules linked in:
> > [ 6186.458232][    T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G    B
> > W         5.9.0-rc5-next-20200915-00006-g104c8fe4916b #1
> > [ 6186.462833][    T1] Hardware name: linux,dummy-virt (DT)
> > [ 6186.465170][    T1] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
> > [ 6186.467910][    T1] pc : device_property_read_string_array+0x40/0xa0
> > [ 6186.470653][    T1] lr : device_property_read_string_array+0x40/0xa0
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] gpiolib: generalize GPIO line names property

[Andy Shevchenko]

On Tue, Sep 15, 2020 at 03:16:42PM +0200, Bartosz Golaszewski wrote:
> On Tue, Sep 15, 2020 at 3:12 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Sep 15, 2020 at 02:01:56PM +0200, Anders Roxell wrote:
> > > On Tue, 8 Sep 2020 at 18:40, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > I initially sent this as part of the gpio-mockup overhaul but since
> > > > these patches are indepentent and the work on gpio-mockup may become
> > > > more complicated - I'm sending these separately.
> > > >
> > > > The only change is adding additional property helpers to count strings
> > > > in array.
> > > >
> > > > Bartosz Golaszewski (3):
> > > >   device: property: add helpers to count items in string arrays
> > > >   gpiolib: generalize devprop_gpiochip_set_names() for device properties
> > > >   gpiolib: unexport devprop_gpiochip_set_names()
> >
> > Ha-ha, OF unittest is of_node centric. definitely there is no backed device.
> >
> > Bart, it seems we are stuck with fwnode interface.
> >
> 
> Wait what?! This means the implementation is wrong - the whole concept
> of device properties is to be generic and to hide the underlying
> fwnode or OF properties. If anything we should fix
> device/base/property.c to fall back to OF.
> 
> What is happening exactly? If all fwnode code compiled out?
> 
> I'll try to give it a spin and see what can be done but I don't like
> that device_property_* functions fail if you have OF but not fwnode.

Read my next reply. It seems only OF unittest issue

% git grep -n -w fwnode -- drivers/of/unittest* | wc -l
0
% git grep -n -w fwnode -- drivers/of/ | wc -l
50

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] gpiolib: generalize GPIO line names property

[Bartosz Golaszewski]

On Tue, Sep 15, 2020 at 3:16 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 15, 2020 at 04:12:28PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 15, 2020 at 02:01:56PM +0200, Anders Roxell wrote:
> > > On Tue, 8 Sep 2020 at 18:40, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > I initially sent this as part of the gpio-mockup overhaul but since
> > > > these patches are indepentent and the work on gpio-mockup may become
> > > > more complicated - I'm sending these separately.
> > > >
> > > > The only change is adding additional property helpers to count strings
> > > > in array.
> > > >
> > > > Bartosz Golaszewski (3):
> > > >   device: property: add helpers to count items in string arrays
> > > >   gpiolib: generalize devprop_gpiochip_set_names() for device properties
> > > >   gpiolib: unexport devprop_gpiochip_set_names()
> >
> > Ha-ha, OF unittest is of_node centric. definitely there is no backed device.
> >
> > Bart, it seems we are stuck with fwnode interface.
>
> Hmm... There is a platform device. So, it means that it fails along these
> lines:
>   return IS_ENABLED(CONFIG_OF) && dev->of_node ? &dev->of_node->fwnode : dev->fwnode;
> so, who should set fwnode for of_node?
>

It's strange because the device for this unittest is registered
similarly to how we do it in gpio-mockup where this function works.
I've not investigated the internals but somewhere someone sets the
fwnode for these platform devices.

Bartosz

Re: [PATCH 0/3] gpiolib: generalize GPIO line names property

[Bartosz Golaszewski]

On Tue, Sep 15, 2020 at 2:02 PM Anders Roxell <anders.roxell@linaro.org> wrote:
>
> On Tue, 8 Sep 2020 at 18:40, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > I initially sent this as part of the gpio-mockup overhaul but since
> > these patches are indepentent and the work on gpio-mockup may become
> > more complicated - I'm sending these separately.
> >
> > The only change is adding additional property helpers to count strings
> > in array.
> >
> > Bartosz Golaszewski (3):
> >   device: property: add helpers to count items in string arrays
> >   gpiolib: generalize devprop_gpiochip_set_names() for device properties
> >   gpiolib: unexport devprop_gpiochip_set_names()
>
> I do an arm64 allmodconfig build fron linux-next (tag: next-20200915) and
> run that in qemu. When I run I see the following output (see full log [1]):
> "BUG: KASAN: null-ptr-deref in device_property_read_string_array".
>
>

FYI: this fails because someone passes a NULL struct device * to
dev_fwnode() - this is probably caused by some ordering issues in this
patch. I'm working on it.

Bartosz