gpio: implement the configfs testing module

From: Bartosz Golaszewski <[email protected]>

The 'extern' keyword doesn't have any benefits in header files. Remove it.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 include/linux/bitmap.h | 115 ++++++++++++++++++++---------------------
 1 file changed, 57 insertions(+), 58 deletions(-)

include/linux/bitmap.h +3 -11

index 70a932470b2d..6939a8983026 100644
@@ -118,54 +118,53 @@
  * Allocation and deallocation of bitmap.
  * Provided in lib/bitmap.c to avoid circular dependency.
  */
-extern unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags);
-extern unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags);
-extern void bitmap_free(const unsigned long *bitmap);
+unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags);
+unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags);
+void bitmap_free(const unsigned long *bitmap);
 
 /*
  * lib/bitmap.c provides these functions:
  */
 
-extern int __bitmap_equal(const unsigned long *bitmap1,
-			  const unsigned long *bitmap2, unsigned int nbits);
-extern bool __pure __bitmap_or_equal(const unsigned long *src1,
-				     const unsigned long *src2,
-				     const unsigned long *src3,
-				     unsigned int nbits);
-extern void __bitmap_complement(unsigned long *dst, const unsigned long *src,
-			unsigned int nbits);

include/linux/bitmap.h +36 -23

+bool __pure __bitmap_or_equal(const unsigned long *src1,
+			      const unsigned long *src2,
+			      const unsigned long *src3,
+			      unsigned int nbits);
+void __bitmap_complement(unsigned long *dst, const unsigned long *src,
+			 unsigned int nbits);
+void __bitmap_shift_right(unsigned long *dst, const unsigned long *src,
+			  unsigned int shift, unsigned int nbits);
+void __bitmap_shift_left(unsigned long *dst, const unsigned long *src,
+			 unsigned int shift, unsigned int nbits);
+void bitmap_cut(unsigned long *dst, const unsigned long *src,
+		unsigned int first, unsigned int cut, unsigned int nbits);
+int __bitmap_and(unsigned long *dst, const unsigned long *bitmap1,
+		 const unsigned long *bitmap2, unsigned int nbits);
+void __bitmap_or(unsigned long *dst, const unsigned long *bitmap1,
+		 const unsigned long *bitmap2, unsigned int nbits);
+void __bitmap_xor(unsigned long *dst, const unsigned long *bitmap1,
+		  const unsigned long *bitmap2, unsigned int nbits);
+int __bitmap_andnot(unsigned long *dst, const unsigned long *bitmap1,
+		    const unsigned long *bitmap2, unsigned int nbits);
+void __bitmap_replace(unsigned long *dst,
+		      const unsigned long *old, const unsigned long *new,
+		      const unsigned long *mask, unsigned int nbits);
+int __bitmap_intersects(const unsigned long *bitmap1,
 			const unsigned long *bitmap2, unsigned int nbits);
-extern void __bitmap_or(unsigned long *dst, const unsigned long *bitmap1,
-			const unsigned long *bitmap2, unsigned int nbits);
-extern void __bitmap_xor(unsigned long *dst, const unsigned long *bitmap1,
-			const unsigned long *bitmap2, unsigned int nbits);
-extern int __bitmap_andnot(unsigned long *dst, const unsigned long *bitmap1,
-			const unsigned long *bitmap2, unsigned int nbits);
-extern void __bitmap_replace(unsigned long *dst,
-			const unsigned long *old, const unsigned long *new,
-			const unsigned long *mask, unsigned int nbits);
-extern int __bitmap_intersects(const unsigned long *bitmap1,
-			const unsigned long *bitmap2, unsigned int nbits);
-extern int __bitmap_subset(const unsigned long *bitmap1,
-			const unsigned long *bitmap2, unsigned int nbits);
-extern int __bitmap_weight(const unsigned long *bitmap, unsigned int nbits);
-extern void __bitmap_set(unsigned long *map, unsigned int start, int len);
-extern void __bitmap_clear(unsigned long *map, unsigned int start, int len);
-
-extern unsigned long bitmap_find_next_zero_area_off(unsigned long *map,
-						    unsigned long size,
-						    unsigned long start,
-						    unsigned int nr,
-						    unsigned long align_mask,
-						    unsigned long align_offset);
+int __bitmap_subset(const unsigned long *bitmap1,
+		    const unsigned long *bitmap2, unsigned int nbits);
+int __bitmap_weight(const unsigned long *bitmap, unsigned int nbits);
+void __bitmap_set(unsigned long *map, unsigned int start, int len);
+void __bitmap_clear(unsigned long *map, unsigned int start, int len);
+
+unsigned long bitmap_find_next_zero_area_off(unsigned long *map,
+					     unsigned long size,
+					     unsigned long start,
+					     unsigned int nr,
+					     unsigned long align_mask,
+					     unsigned long align_offset);
 
 /**
  * bitmap_find_next_zero_area - find a contiguous aligned zero area

include/linux/bitmap.h +8 -8

-extern void bitmap_onto(unsigned long *dst, const unsigned long *orig,
+void bitmap_onto(unsigned long *dst, const unsigned long *orig,
 		const unsigned long *relmap, unsigned int bits);
-extern void bitmap_fold(unsigned long *dst, const unsigned long *orig,
+void bitmap_fold(unsigned long *dst, const unsigned long *orig,
 		unsigned int sz, unsigned int nbits);
-extern int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, int order);
-extern void bitmap_release_region(unsigned long *bitmap, unsigned int pos, int order);
-extern int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order);
+int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, int order);
+void bitmap_release_region(unsigned long *bitmap, unsigned int pos, int order);
+int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order);
 
 #ifdef __BIG_ENDIAN
-extern void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int nbits);
+void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int nbits);
 #else
 #define bitmap_copy_le bitmap_copy
 #endif
-extern unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord, unsigned int nbits);
-extern int bitmap_print_to_pagebuf(bool list, char *buf,
+unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord, unsigned int nbits);
+int bitmap_print_to_pagebuf(bool list, char *buf,
 				   const unsigned long *maskp, int nmaskbits);

#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))

include/linux/bitmap.h +0 -0

 #else
 #define bitmap_from_arr32(bitmap, buf, nbits)			\

Signature · 1 line

2.29.1

Quoted 3 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

The 'extern' keyword doesn't have any benefits in header files. Remove it.

Reviewed-by Andy Shevchenko <[email protected]>

A few nitpicks below.

Quoted 2 lines

Signed-off-by Bartosz Golaszewski <[email protected]>

---

From: Bartosz Golaszewski <[email protected]>

For better readability and maintenance: order the includes in bitmap source files alphabetically.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 include/linux/bitmap.h | 4 ++--
 lib/bitmap.c           | 9 +++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

include/linux/bitmap.h +2 -2

index 6939a8983026..3282db97e06c 100644
@@ -4,10 +4,10 @@
 
 #ifndef __ASSEMBLY__
 
-#include <linux/types.h>
 #include <linux/bitops.h>
-#include <linux/string.h>
 #include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/types.h>
 
 /*
  * bitmaps provide bit arrays that consume one or more unsigned

lib/bitmap.c +5 -4

index 75006c4036e9..78f70d9007ad 100644
@@ -3,17 +3,18 @@
  * lib/bitmap.c
  * Helper functions for bitmap.h.
  */
-#include <linux/export.h>
-#include <linux/thread_info.h>
-#include <linux/ctype.h>
-#include <linux/errno.h>
+
 #include <linux/bitmap.h>
 #include <linux/bitops.h>
 #include <linux/bug.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/string.h>
+#include <linux/thread_info.h>
 #include <linux/uaccess.h>
 
 #include <asm/page.h>

Signature · 1 line

2.29.1

From: Bartosz Golaszewski <[email protected]>

Provide managed variants of bitmap_alloc() and bitmap_zalloc().

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 include/linux/bitmap.h | 10 ++++++++++
 lib/bitmap.c           | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

include/linux/bitmap.h +2 -0

index 3282db97e06c..e41c622db1b8 100644
@@ -9,6 +9,8 @@
 #include <linux/string.h>
 #include <linux/types.h>
 
+struct device;
+
 /*
  * bitmaps provide bit arrays that consume one or more unsigned
  * longs.  The bitmap interface and available operations are listed

include/linux/bitmap.h +0 -0

 /*
  * lib/bitmap.c provides these functions:
  */

lib/bitmap.c +1 -0

index 78f70d9007ad..b4fd7fd084c6 100644
@@ -8,6 +8,7 @@
 #include <linux/bitops.h>
 #include <linux/bug.h>
 #include <linux/ctype.h>
+#include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/export.h>
 #include <linux/kernel.h>

@@ -1263,6 +1264,38 @@ void bitmap_free(const unsigned long *bitmap)
 }
 EXPORT_SYMBOL(bitmap_free);

lib/bitmap.c +8 -0

+{
+	unsigned long *bitmap;
+	int ret;
+
+	bitmap = bitmap_alloc(nbits, flags);
+	if (!bitmap)
+		return NULL;
+

lib/bitmap.c +5 -0

+{
+	return devm_bitmap_alloc(dev, nbits, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(devm_bitmap_zalloc);
+
 #if BITS_PER_LONG == 64
 /**
  * bitmap_from_arr32 - copy the contents of u32 array of bits to bitmap

Signature · 1 line

2.29.1

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

drivers/base/dd.c +1 -1

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);
 
 static void driver_bound(struct device *dev)
 {
--

2.29.1

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

Quoted 16 lines · Geert Uytterhoeven

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

No. Please no. Why is this needed? Feels like someone is doing something really wrong...

NACK.

Quoted 31 lines · Greg KH

Quoted 16 lines · Geert Uytterhoeven

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

No. Please no. Why is this needed? Feels like someone is doing something really wrong...

Quoted 2 lines

NACK.

I should have Cc'ed you the entire series, my bad.

This is the patch that uses this change - it's a new, improved testing module for GPIO using configfs & sysfs as you (I think) suggested a while ago:

https://lkml.org/lkml/2021/3/4/355

The story goes like this: committing the configfs item registers a platform device.

As far as I understand - there's no guarantee that
the device will be bound to a driver before the commit callback (or more specifically platform_device_register_full() in this case) returns so the user may try to retrieve the name of the device immediately (normally user-space should wait for the associated uevent but nobody can force that) by doing:

mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/ cat /sys/kernel/config/gpio-sim/live/foo/dev_name

If the device is not bound at this point, we'll have a crash in the kernel as opposed to just returning -ENODEV.

Please advise on how to handle it without device_is_bound().

Quoted 43 lines · Bartosz Golaszewski

Quoted 31 lines · Greg KH

Quoted 16 lines · Geert Uytterhoeven

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

No. Please no. Why is this needed? Feels like someone is doing something really wrong...

Quoted 2 lines

NACK.

I should have Cc'ed you the entire series, my bad.

This is the patch that uses this change - it's a new, improved testing module for GPIO using configfs & sysfs as you (I think) suggested a while ago:

https://lkml.org/lkml/2021/3/4/355

Quoted 2 lines

The story goes like this: committing the configfs item registers a platform device.

Ick, no, stop there, that's not a "real" device, please do not abuse platform devices like that, you all know I hate this :(

Use the virtbus code instead perhaps?

Quoted 10 lines

As far as I understand - there's no guarantee that
the device will be bound to a driver before the commit callback (or more specifically platform_device_register_full() in this case) returns so the user may try to retrieve the name of the device immediately (normally user-space should wait for the associated uevent but nobody can force that) by doing:

mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/ cat /sys/kernel/config/gpio-sim/live/foo/dev_name

Quoted 2 lines

If the device is not bound at this point, we'll have a crash in the kernel as opposed to just returning -ENODEV.

How will the kernel crash? What has created the dev_name sysfs file before it is possible to be read from? That feels like the root problem.

Quoted 1 line

Please advise on how to handle it without device_is_bound().

Please do not create sysfs files before they can be read from :)

Quoted 51 lines · Greg KH

Quoted 37 lines · Bartosz Golaszewski

Quoted 31 lines · Greg KH

Quoted 16 lines · Geert Uytterhoeven

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

No. Please no. Why is this needed? Feels like someone is doing something really wrong...

Quoted 2 lines

NACK.

I should have Cc'ed you the entire series, my bad.

Quoted 8 lines

This is the patch that uses this change - it's a new, improved testing module for GPIO using configfs & sysfs as you (I think) suggested a while ago:

https://lkml.org/lkml/2021/3/4/355

The story goes like this: committing the configfs item registers a platform device.

Ick, no, stop there, that's not a "real" device, please do not abuse platform devices like that, you all know I hate this :(

Quoted 2 lines

Use the virtbus code instead perhaps?

I have no idea what virtbus is and grepping for it only returns three hits in: ./drivers/pci/iov.c and it's a function argument.

If it stands for virtual bus then for sure it sounds like the right thing but I need to find more info on this.

Quoted 10 lines

Quoted 10 lines

As far as I understand - there's no guarantee that
the device will be bound to a driver before the commit callback (or more specifically platform_device_register_full() in this case) returns so the user may try to retrieve the name of the device immediately (normally user-space should wait for the associated uevent but nobody can force that) by doing:

mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/ cat /sys/kernel/config/gpio-sim/live/foo/dev_name

Quoted 7 lines

Quoted 2 lines

If the device is not bound at this point, we'll have a crash in the kernel as opposed to just returning -ENODEV.

How will the kernel crash? What has created the dev_name sysfs file before it is possible to be read from? That feels like the root problem.

It's not sysfs - it's in configfs. Each chip has a read-only configfs attribute that returns the name of the device - I don't really have a better idea to map the configfs items to devices that committing creates.

Quoted 52 lines · Bartosz Golaszewski

Quoted 51 lines · Greg KH

Quoted 37 lines · Bartosz Golaszewski

Quoted 31 lines · Greg KH

Quoted 16 lines · Geert Uytterhoeven

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

No. Please no. Why is this needed? Feels like someone is doing something really wrong...

Quoted 2 lines

NACK.

I should have Cc'ed you the entire series, my bad.

Quoted 8 lines

This is the patch that uses this change - it's a new, improved testing module for GPIO using configfs & sysfs as you (I think) suggested a while ago:

https://lkml.org/lkml/2021/3/4/355

The story goes like this: committing the configfs item registers a platform device.

Ick, no, stop there, that's not a "real" device, please do not abuse platform devices like that, you all know I hate this :(

Quoted 8 lines

Quoted 2 lines

Use the virtbus code instead perhaps?

I have no idea what virtbus is and grepping for it only returns three hits in: ./drivers/pci/iov.c and it's a function argument.

If it stands for virtual bus then for sure it sounds like the right thing but I need to find more info on this.

Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for the details. "virtbus" was what I think about it as that was my original name for it, but it eventually got merged with a different name.

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

As far as I understand - there's no guarantee that
the device will be bound to a driver before the commit callback (or more specifically platform_device_register_full() in this case) returns so the user may try to retrieve the name of the device immediately (normally user-space should wait for the associated uevent but nobody can force that) by doing:

Quoted 5 lines

mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/ cat /sys/kernel/config/gpio-sim/live/foo/dev_name

If the device is not bound at this point, we'll have a crash in the kernel as opposed to just returning -ENODEV.

How will the kernel crash? What has created the dev_name sysfs file before it is possible to be read from? That feels like the root problem.

Quoted 4 lines

It's not sysfs - it's in configfs. Each chip has a read-only configfs attribute that returns the name of the device - I don't really have a better idea to map the configfs items to devices that committing creates.

Same question, why are you exporting a configfs attribute that can not be read from? Only export it when your driver is bound to the device.

Quoted 86 lines · Greg KH

Quoted 52 lines · Bartosz Golaszewski

Quoted 51 lines · Greg KH

Quoted 37 lines · Bartosz Golaszewski

Quoted 31 lines · Greg KH

Quoted 16 lines · Geert Uytterhoeven

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

No. Please no. Why is this needed? Feels like someone is doing something really wrong...

Quoted 2 lines

NACK.

I should have Cc'ed you the entire series, my bad.

Quoted 8 lines

This is the patch that uses this change - it's a new, improved testing module for GPIO using configfs & sysfs as you (I think) suggested a while ago:

https://lkml.org/lkml/2021/3/4/355

The story goes like this: committing the configfs item registers a platform device.

Ick, no, stop there, that's not a "real" device, please do not abuse platform devices like that, you all know I hate this :(

Quoted 8 lines

Quoted 2 lines

Use the virtbus code instead perhaps?

I have no idea what virtbus is and grepping for it only returns three hits in: ./drivers/pci/iov.c and it's a function argument.

If it stands for virtual bus then for sure it sounds like the right thing but I need to find more info on this.

Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for the details. "virtbus" was what I think about it as that was my original name for it, but it eventually got merged with a different name.

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

As far as I understand - there's no guarantee that
the device will be bound to a driver before the commit callback (or more specifically platform_device_register_full() in this case) returns so the user may try to retrieve the name of the device immediately (normally user-space should wait for the associated uevent but nobody can force that) by doing:

Quoted 5 lines

mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/ cat /sys/kernel/config/gpio-sim/live/foo/dev_name

If the device is not bound at this point, we'll have a crash in the kernel as opposed to just returning -ENODEV.

How will the kernel crash? What has created the dev_name sysfs file before it is possible to be read from? That feels like the root problem.

Quoted 8 lines

Quoted 4 lines

It's not sysfs - it's in configfs. Each chip has a read-only configfs attribute that returns the name of the device - I don't really have a better idea to map the configfs items to devices that committing creates.

Same question, why are you exporting a configfs attribute that can not be read from? Only export it when your driver is bound to the device.

The device doesn't know anything about configfs. Why would it? The configuration of a GPIO chip can't be changed after it's instantiated, this is why we have committable items.

We export a directory in configfs: gpio-sim -> user creates a new directory (item) in gpio-sim/pending/foo and it's not tied to any device yet but exports attributes which we use to configure the device (label, number of lines, line names etc.), then we mv gpio-sim/pending/foo gpio-sim/live and this is when the device gets created and registered with the subsystem. We take all the configured attributes and put them into device properties for both the driver and gpiolib core (for standard properties) to read - just like we would with a regular GPIO driver because this is the goal: test the core code.

Quoted 61 lines · Bartosz Golaszewski

Quoted 60 lines · Greg KH

Quoted 58 lines · Bartosz Golaszewski

Quoted 45 lines · Greg KH

Quoted 43 lines · Bartosz Golaszewski

Quoted 31 lines · Greg KH

Quoted 16 lines · Geert Uytterhoeven

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

No. Please no. Why is this needed? Feels like someone is doing something really wrong...

Quoted 2 lines

NACK.

I should have Cc'ed you the entire series, my bad.

This is the patch that uses this change - it's a new, improved testing module for GPIO using configfs & sysfs as you (I think) suggested a while ago:

https://lkml.org/lkml/2021/3/4/355

Quoted 8 lines

Quoted 2 lines

The story goes like this: committing the configfs item registers a platform device.

Ick, no, stop there, that's not a "real" device, please do not abuse platform devices like that, you all know I hate this :(

Use the virtbus code instead perhaps?

I have no idea what virtbus is and grepping for it only returns three hits in: ./drivers/pci/iov.c and it's a function argument.

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

If it stands for virtual bus then for sure it sounds like the right thing but I need to find more info on this.

Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for the details. "virtbus" was what I think about it as that was my original name for it, but it eventually got merged with a different name.

Quoted 31 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

As far as I understand - there's no guarantee that
the device will be bound to a driver before the commit callback (or more specifically platform_device_register_full() in this case) returns so the user may try to retrieve the name of the device immediately (normally user-space should wait for the associated uevent but nobody can force that) by doing:

Quoted 5 lines

mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/ cat /sys/kernel/config/gpio-sim/live/foo/dev_name

If the device is not bound at this point, we'll have a crash in the kernel as opposed to just returning -ENODEV.

How will the kernel crash? What has created the dev_name sysfs file before it is possible to be read from? That feels like the root problem.

Quoted 8 lines

Quoted 4 lines

It's not sysfs - it's in configfs. Each chip has a read-only configfs attribute that returns the name of the device - I don't really have a better idea to map the configfs items to devices that committing creates.

Same question, why are you exporting a configfs attribute that can not be read from? Only export it when your driver is bound to the device.

The device doesn't know anything about configfs. Why would it? The configuration of a GPIO chip can't be changed after it's instantiated, this is why we have committable items.

Quoted 10 lines

We export a directory in configfs: gpio-sim -> user creates a new directory (item) in gpio-sim/pending/foo and it's not tied to any device yet but exports attributes which we use to configure the device (label, number of lines, line names etc.), then we mv gpio-sim/pending/foo gpio-sim/live and this is when the device gets created and registered with the subsystem. We take all the configured attributes and put them into device properties for both the driver and gpiolib core (for standard properties) to read - just like we would with a regular GPIO driver because this is the goal: test the core code.

Ok, but they why are you trying to have dev_name be an exported thing? I don't understand an attribute here that is visable but can not be read from.

And why not just use the default device name function: dev_name(), which will always return a string that will work no matter if the device is bound to a driver or not.

Quoted 63 lines · Greg KH

Quoted 61 lines · Bartosz Golaszewski

Quoted 60 lines · Greg KH

Quoted 58 lines · Bartosz Golaszewski

Quoted 45 lines · Greg KH

Quoted 43 lines · Bartosz Golaszewski

Quoted 31 lines · Greg KH

Quoted 16 lines · Geert Uytterhoeven

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

No. Please no. Why is this needed? Feels like someone is doing something really wrong...

Quoted 2 lines

NACK.

I should have Cc'ed you the entire series, my bad.

This is the patch that uses this change - it's a new, improved testing module for GPIO using configfs & sysfs as you (I think) suggested a while ago:

https://lkml.org/lkml/2021/3/4/355

Quoted 8 lines

Quoted 2 lines

The story goes like this: committing the configfs item registers a platform device.

Ick, no, stop there, that's not a "real" device, please do not abuse platform devices like that, you all know I hate this :(

Use the virtbus code instead perhaps?

I have no idea what virtbus is and grepping for it only returns three hits in: ./drivers/pci/iov.c and it's a function argument.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

If it stands for virtual bus then for sure it sounds like the right thing but I need to find more info on this.

Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for the details. "virtbus" was what I think about it as that was my original name for it, but it eventually got merged with a different name.

Unless I'm not seeing something - it completely doesn't look like the right solution. This auxiliary bus sounds like MFD with extra steps. Its aim seems to be to provide virtual devices for sub-modules of real devices.

What I have here really is a dummy device for which no HW exists.

Also: while the preferred way is to use configfs to instantiate these simulated devices, then can still be registered from device-tree (this is a feature that was requested and eventually implemented in gpio-mockup which we want to phase out so we can't just drop it). AFAIK only platform devices can be populated from DT.

I guess we could create something like a "virtual bus" that would be there for devices that don't exist on any physical bus but this would end up in big part being the same thing as platform devices.

Quoted 42 lines

Quoted 31 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

As far as I understand - there's no guarantee that
the device will be bound to a driver before the commit callback (or more specifically platform_device_register_full() in this case) returns so the user may try to retrieve the name of the device immediately (normally user-space should wait for the associated uevent but nobody can force that) by doing:

Quoted 5 lines

mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/ cat /sys/kernel/config/gpio-sim/live/foo/dev_name

If the device is not bound at this point, we'll have a crash in the kernel as opposed to just returning -ENODEV.

How will the kernel crash? What has created the dev_name sysfs file before it is possible to be read from? That feels like the root problem.

Quoted 8 lines

Quoted 4 lines

It's not sysfs - it's in configfs. Each chip has a read-only configfs attribute that returns the name of the device - I don't really have a better idea to map the configfs items to devices that committing creates.

Same question, why are you exporting a configfs attribute that can not be read from? Only export it when your driver is bound to the device.

The device doesn't know anything about configfs. Why would it? The configuration of a GPIO chip can't be changed after it's instantiated, this is why we have committable items.

Quoted 10 lines

We export a directory in configfs: gpio-sim -> user creates a new directory (item) in gpio-sim/pending/foo and it's not tied to any device yet but exports attributes which we use to configure the device (label, number of lines, line names etc.), then we mv gpio-sim/pending/foo gpio-sim/live and this is when the device gets created and registered with the subsystem. We take all the configured attributes and put them into device properties for both the driver and gpiolib core (for standard properties) to read - just like we would with a regular GPIO driver because this is the goal: test the core code.

Quoted 4 lines

Ok, but they why are you trying to have dev_name be an exported thing? I don't understand an attribute here that is visable but can not be read from.

Because once the associated configfs item is committed and the device created, it will become readable. The list of attributes is fixed in configfs. I'm not sure what the better approach would be - return "none" if the device handle is NULL?

Quoted 4 lines

And why not just use the default device name function: dev_name(), which will always return a string that will work no matter if the device is bound to a driver or not.

I can do this but then it's possible that user-space gets the name of the device which doesn't exist in sysfs. I guess we can mention that in the documentation.

Quoted 73 lines · Bartosz Golaszewski

Quoted 63 lines · Greg KH

Quoted 61 lines · Bartosz Golaszewski

Quoted 60 lines · Greg KH

Quoted 58 lines · Bartosz Golaszewski

Quoted 45 lines · Greg KH

Quoted 43 lines · Bartosz Golaszewski

Quoted 31 lines · Greg KH

Quoted 16 lines · Geert Uytterhoeven

CC Greg

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Export the symbol for device_is_bound() so that we can use it in gpio-sim to check if the simulated GPIO chip is bound before fetching its driver data from configfs callbacks in order to retrieve the name of the GPIO chip device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 9 lines

drivers/base/dd.c bool device_is_bound(struct device *dev) · line 356

Quoted 9 lines

drivers/base/dd.c +1 -0

index 9179825ff646..c62c02e3490a 100644
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
 {
        return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);

No. Please no. Why is this needed? Feels like someone is doing something really wrong...

Quoted 2 lines

NACK.

I should have Cc'ed you the entire series, my bad.

This is the patch that uses this change - it's a new, improved testing module for GPIO using configfs & sysfs as you (I think) suggested a while ago:

https://lkml.org/lkml/2021/3/4/355

Quoted 8 lines

Quoted 2 lines

The story goes like this: committing the configfs item registers a platform device.

Ick, no, stop there, that's not a "real" device, please do not abuse platform devices like that, you all know I hate this :(

Use the virtbus code instead perhaps?

I have no idea what virtbus is and grepping for it only returns three hits in: ./drivers/pci/iov.c and it's a function argument.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

If it stands for virtual bus then for sure it sounds like the right thing but I need to find more info on this.

Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for the details. "virtbus" was what I think about it as that was my original name for it, but it eventually got merged with a different name.

Quoted 6 lines

Unless I'm not seeing something - it completely doesn't look like the right solution. This auxiliary bus sounds like MFD with extra steps. Its aim seems to be to provide virtual devices for sub-modules of real devices.

What I have here really is a dummy device for which no HW exists.

Then just use a "normal" virtual device. We have loads of them. But if you want to bind a "driver" to it, then use the aux bus please. Do NOT abuse a platform device for this.

Quoted 5 lines

Also: while the preferred way is to use configfs to instantiate these simulated devices, then can still be registered from device-tree (this is a feature that was requested and eventually implemented in gpio-mockup which we want to phase out so we can't just drop it). AFAIK only platform devices can be populated from DT.

If you really are using DT, then ok, a platform device can be used, but you didn't say that :)

Quoted 3 lines

I guess we could create something like a "virtual bus" that would be there for devices that don't exist on any physical bus but this would end up in big part being the same thing as platform devices.

That's what the aux bus code is there for. So maybe you do need to use it.

Quoted 47 lines

Quoted 42 lines

Quoted 31 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

As far as I understand - there's no guarantee that
the device will be bound to a driver before the commit callback (or more specifically platform_device_register_full() in this case) returns so the user may try to retrieve the name of the device immediately (normally user-space should wait for the associated uevent but nobody can force that) by doing:

Quoted 5 lines

mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/ cat /sys/kernel/config/gpio-sim/live/foo/dev_name

If the device is not bound at this point, we'll have a crash in the kernel as opposed to just returning -ENODEV.

How will the kernel crash? What has created the dev_name sysfs file before it is possible to be read from? That feels like the root problem.

Quoted 8 lines

Quoted 4 lines

It's not sysfs - it's in configfs. Each chip has a read-only configfs attribute that returns the name of the device - I don't really have a better idea to map the configfs items to devices that committing creates.

Same question, why are you exporting a configfs attribute that can not be read from? Only export it when your driver is bound to the device.

The device doesn't know anything about configfs. Why would it? The configuration of a GPIO chip can't be changed after it's instantiated, this is why we have committable items.

Quoted 10 lines

We export a directory in configfs: gpio-sim -> user creates a new directory (item) in gpio-sim/pending/foo and it's not tied to any device yet but exports attributes which we use to configure the device (label, number of lines, line names etc.), then we mv gpio-sim/pending/foo gpio-sim/live and this is when the device gets created and registered with the subsystem. We take all the configured attributes and put them into device properties for both the driver and gpiolib core (for standard properties) to read - just like we would with a regular GPIO driver because this is the goal: test the core code.

Quoted 4 lines

Ok, but they why are you trying to have dev_name be an exported thing? I don't understand an attribute here that is visable but can not be read from.

Quoted 4 lines

Because once the associated configfs item is committed and the device created, it will become readable. The list of attributes is fixed in configfs. I'm not sure what the better approach would be - return "none" if the device handle is NULL?

Sounds reasonable, I don't know how configfs works, it's been a decade since I last touched it.

Quoted 5 lines

Quoted 4 lines

And why not just use the default device name function: dev_name(), which will always return a string that will work no matter if the device is bound to a driver or not.

Quoted 3 lines

I can do this but then it's possible that user-space gets the name of the device which doesn't exist in sysfs. I guess we can mention that in the documentation.

Device names can change over time, nothing new there.

From: Bartosz Golaszewski <[email protected]>

Implement a new, modern GPIO testing module controlled by configfs attributes instead of module parameters. The goal of this driver is to provide a replacement for gpio-mockup that will be easily extensible with new features and doesn't require reloading the module to change the setup.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 Documentation/admin-guide/gpio/gpio-sim.rst |  72 ++
 drivers/gpio/Kconfig                        |   8 +
 drivers/gpio/Makefile                       |   1 +
 drivers/gpio/gpio-sim.c                     | 878 ++++++++++++++++++++
 4 files changed, 959 insertions(+)
 create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst
 create mode 100644 drivers/gpio/gpio-sim.c

Documentation/admin-guide/gpio/gpio-sim.rst new file +72 -0

new file mode 100644
index 000000000000..08eac487e35e
@@ -0,0 +1,72 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Configfs GPIO Simulator
+=======================
+
+The configfs GPIO Simulator (gpio-sim) provides a way to create simulated GPIO
+chips for testing purposes. The lines exposed by these chips can be accessed
+using the standard GPIO character device interface as well as manipulated
+using sysfs attributes.
+
+Creating simulated chips
+------------------------
+
+The gpio-sim module registers a configfs subsystem called 'gpio-sim'. It's a
+subsystem with committable items which means two subdirectories are created in
+the filesystem: pending and live. For more information on configfs and
+committable items, please refer to Documentation/filesystems/configfs.rst.
+
+In order to instantiate a new simulated chip, the user needs to mkdir() a new
+directory in pending/. Inside each new directory, there's a set of attributes
+that can be used to configure the new chip. Once the configuration is complete,
+the user needs to use rename() to move the chip to the live/ directory. This
+creates and registers the new device.
+
+In order to destroy a simulated chip, it has to be moved back to pending first
+and then removed using rmdir().
+
+Currently supported configuration attributes are:
+
+  num_lines - an unsigned integer value defining the number of GPIO lines to
+              export
+
+  label - a string defining the label for the GPIO chip
+
+  line_names - a list of GPIO line names in the form of quoted strings
+               separated by commas, e.g.: '"foo", "bar", "", "foobar"'. The
+               number of strings doesn't have to be equal to the value set in
+               the num_lines attribute. If it's lower than the number of lines,
+               the remaining lines are unnamed. If it's larger, the superfluous
+               lines are ignored. A name of the form: '""' means the line
+               should be unnamed.
+
+Additionally two read-only attributes named 'chip_name' and 'dev_name' are
+exposed in order to provide users with a mapping from configfs directories to
+the actual devices created in the kernel. The former returns the name of the
+GPIO device as assigned by gpiolib (i.e. "gpiochip0", "gpiochip1", etc.). The
+latter returns the parent device name as defined by the gpio-sim driver (i.e.
+"gpio-sim.0", "gpio-sim.1", etc.). This allows user-space to map the configfs
+items both to the correct character device file as well as the associated entry
+in sysfs.
+
+Simulated GPIO chips can also be defined in device-tree. The compatible string
+must be: "gpio-simulator". Supported properties are:
+
+  "gpio-sim,label" - chip label
+
+  "gpio-sim,nr-gpios" - number of lines
+
+Other standard GPIO properties (like "gpio-line-names" and gpio-hog) are also
+supported.
+
+Manipulating simulated lines
+----------------------------
+
+Each simulated GPIO chip creates a sysfs attribute group under its device
+directory called 'line-ctrl'. Inside each group, there's a separate attribute
+for each GPIO line. The name of the attribute is of the form 'gpioX' where X
+is the line's offset in the chip.
+
+Reading from a line attribute returns the current value. Writing to it (0 or 1)
+changes the configuration of the simulated pull-up/pull-down resistor
+(1 - pull-up, 0 - pull-down).

drivers/gpio/Kconfig +8 -0

index e3607ec4c2e8..b6b6150d0d04 100644
@@ -1633,6 +1633,14 @@ config GPIO_MOCKUP
 	  tools/testing/selftests/gpio/gpio-mockup.sh. Reference the usage in
 	  it.
 
+config GPIO_SIM
+	tristate "GPIO Simulator Module"
+	select IRQ_SIM
+	select CONFIGFS_FS
+	help
+	  This enables the GPIO simulator - a configfs-based GPIO testing
+	  driver.
+
 endmenu
 
 endif

drivers/gpio/Makefile +1 -0

index c58a90a3c3b1..d717aa85f8e1 100644
@@ -130,6 +130,7 @@ obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
 obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
+obj-$(CONFIG_GPIO_SIM)			+= gpio-sim.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
 obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
 obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o

drivers/gpio/gpio-sim.c new file +284 -0

new file mode 100644
index 000000000000..0ed297127bca
@@ -0,0 +1,878 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * GPIO testing driver based on configfs.
+ *
+ * Copyright (C) 2021 Bartosz Golaszewski <[email protected]>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/bitmap.h>
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/idr.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irq_sim.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/string_helpers.h>
+#include <linux/sysfs.h>
+
+#include "gpiolib.h"
+
+/*
+ * This normally should correspond with the number of attributes exposed over
+ * configfs + sentinel.
+ */
+#define GPIO_SIM_MAX_PROP 4
+
+static DEFINE_IDA(gpio_sim_ida);
+
+struct gpio_sim_chip {
+	struct gpio_chip gc;
+	unsigned long *directions;
+	unsigned long *values;
+	unsigned long *pulls;
+	struct irq_domain *irq_sim;
+	struct mutex lock;
+	struct attribute_group attr_group;
+};
+
+struct gpio_sim_attribute {
+	struct device_attribute dev_attr;
+	unsigned int offset;
+};
+
+static struct gpio_sim_attribute *
+to_gpio_sim_attr(struct device_attribute *dev_attr)
+{
+	return container_of(dev_attr, struct gpio_sim_attribute, dev_attr);
+}
+
+static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
+			       unsigned int offset, int value)
+{
+	int curr_val, irq, irq_type, ret;
+	struct gpio_desc *desc;
+	struct gpio_chip *gc;
+
+	gc = &chip->gc;
+	desc = &gc->gpiodev->descs[offset];
+
+	mutex_lock(&chip->lock);
+
+	if (test_bit(FLAG_REQUESTED, &desc->flags) &&
+	    !test_bit(FLAG_IS_OUT, &desc->flags)) {
+		curr_val = !!test_bit(offset, chip->values);
+		if (curr_val == value)
+			goto set_pull;
+
+		/*
+		 * This is fine - it just means, nobody is listening
+		 * for interrupts on this line, otherwise
+		 * irq_create_mapping() would have been called from
+		 * the to_irq() callback.
+		 */
+		irq = irq_find_mapping(chip->irq_sim, offset);
+		if (!irq)
+			goto set_value;
+
+		irq_type = irq_get_trigger_type(irq);
+
+		if ((value && (irq_type & IRQ_TYPE_EDGE_RISING)) ||
+		    (!value && (irq_type & IRQ_TYPE_EDGE_FALLING))) {
+			ret = irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING,
+						    true);
+			if (ret)
+				goto set_pull;
+		}
+	}
+
+set_value:
+	/* Change the value unless we're actively driving the line. */
+	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
+	    !test_bit(FLAG_IS_OUT, &desc->flags))
+		__assign_bit(offset, chip->values, value);
+
+set_pull:
+	__assign_bit(offset, chip->pulls, value);
+	mutex_unlock(&chip->lock);
+	return 0;
+}
+
+static int gpio_sim_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+	int ret;
+
+	mutex_lock(&chip->lock);
+	ret = !!test_bit(offset, chip->values);
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static void gpio_sim_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__assign_bit(offset, chip->values, value);
+	mutex_unlock(&chip->lock);
+}
+
+static int gpio_sim_get_multiple(struct gpio_chip *gc,
+				 unsigned long *mask, unsigned long *bits)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	bitmap_copy(bits, chip->values, gc->ngpio);
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
+static void gpio_sim_set_multiple(struct gpio_chip *gc,
+				  unsigned long *mask, unsigned long *bits)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	bitmap_copy(chip->values, bits, gc->ngpio);
+	mutex_unlock(&chip->lock);
+}
+
+static int gpio_sim_direction_output(struct gpio_chip *gc,
+				     unsigned int offset, int value)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__clear_bit(offset, chip->directions);
+	__assign_bit(offset, chip->values, value);
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
+static int gpio_sim_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__set_bit(offset, chip->directions);
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
+static int gpio_sim_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+	int direction;
+
+	mutex_lock(&chip->lock);
+	direction = !!test_bit(offset, chip->directions);
+	mutex_unlock(&chip->lock);
+
+	return direction ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+static int gpio_sim_set_config(struct gpio_chip *gc,
+				  unsigned int offset, unsigned long config)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+		return gpio_sim_apply_pull(chip, offset, 1);
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		return gpio_sim_apply_pull(chip, offset, 0);
+	default:
+		break;
+	}
+
+	return -ENOTSUPP;
+}
+
+static int gpio_sim_to_irq(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	return irq_create_mapping(chip->irq_sim, offset);
+}
+
+static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__assign_bit(offset, chip->values, !!test_bit(offset, chip->pulls));
+	mutex_unlock(&chip->lock);
+}
+
+static ssize_t gpio_sim_sysfs_line_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr);
+	struct gpio_sim_chip *chip = dev_get_drvdata(dev);
+	int ret;
+
+	mutex_lock(&chip->lock);
+	ret = sprintf(buf, "%u\n", !!test_bit(line_attr->offset, chip->values));
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static ssize_t gpio_sim_sysfs_line_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t len)
+{
+	struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr);
+	struct gpio_sim_chip *chip = dev_get_drvdata(dev);
+	int ret, val;
+
+	if (len > 2 || (buf[0] != '0' && buf[0] != '1'))
+		return -EINVAL;
+
+	val = buf[0] == '0' ? 0 : 1;
+
+	ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static void gpio_sim_mutex_destroy(void *data)
+{
+	struct mutex *lock = data;
+
+	mutex_destroy(lock);
+}
+
+static void gpio_sim_sysfs_remove(void *data)
+{
+	struct gpio_sim_chip *chip = data;
+
+	sysfs_remove_group(&chip->gc.parent->kobj, &chip->attr_group);
+}
+
+static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
+{
+	unsigned int i, num_lines = chip->gc.ngpio;
+	struct device *dev = chip->gc.parent;
+	struct gpio_sim_attribute *line_attr;
+	struct device_attribute *dev_attr;
+	struct attribute **attrs;
+	int ret;
+
+	attrs = devm_kcalloc(dev, sizeof(*attrs), num_lines + 1, GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
+
+	for (i = 0; i < num_lines; i++) {

drivers/gpio/gpio-sim.c new file +33 -0

+		if (!dev_attr->attr.name)
+			return -ENOMEM;
+
+		dev_attr->attr.mode = 0644;
+
+		dev_attr->show = gpio_sim_sysfs_line_show;
+		dev_attr->store = gpio_sim_sysfs_line_store;
+
+		attrs[i] = &dev_attr->attr;
+	}
+
+	chip->attr_group.name = "line-ctrl";
+	chip->attr_group.attrs = attrs;
+
+	ret = sysfs_create_group(&dev->kobj, &chip->attr_group);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, gpio_sim_sysfs_remove, chip);
+}
+
+static int gpio_sim_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_sim_chip *chip;
+	struct gpio_chip *gc;
+	const char *label;
+	u32 num_lines;
+	int ret;
+
+	ret = device_property_read_u32(dev, "gpio-sim,nr-gpios", &num_lines);
+	if (ret)
+		return ret;

drivers/gpio/gpio-sim.c new file +236 -0

+	chip->values = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->values)
+		return -ENOMEM;
+
+	chip->pulls = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->pulls)
+		return -ENOMEM;
+
+	chip->irq_sim = devm_irq_domain_create_sim(dev, NULL, num_lines);
+	if (IS_ERR(chip->irq_sim))
+		return PTR_ERR(chip->irq_sim);
+
+	mutex_init(&chip->lock);
+	ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy,
+				       &chip->lock);
+	if (ret)
+		return ret;
+
+	gc = &chip->gc;
+	gc->base = -1;
+	gc->ngpio = num_lines;
+	gc->label = label;
+	gc->owner = THIS_MODULE;
+	gc->parent = dev;
+	gc->get = gpio_sim_get;
+	gc->set = gpio_sim_set;
+	gc->get_multiple = gpio_sim_get_multiple;
+	gc->set_multiple = gpio_sim_set_multiple;
+	gc->direction_output = gpio_sim_direction_output;
+	gc->direction_input = gpio_sim_direction_input;
+	gc->get_direction = gpio_sim_get_direction;
+	gc->set_config = gpio_sim_set_config;
+	gc->to_irq = gpio_sim_to_irq;
+	gc->free = gpio_sim_free;
+
+	ret = devm_gpiochip_add_data(dev, gc, chip);
+	if (ret)
+		return ret;
+
+	/* Used by sysfs and configfs callbacks. */
+	dev_set_drvdata(dev, chip);
+
+	ret = gpio_sim_setup_sysfs(chip);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id gpio_sim_of_match[] = {
+	{ .compatible = "gpio-simulator" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gpio_sim_of_match);
+
+static struct platform_driver gpio_sim_driver = {
+	.driver = {
+		.name = "gpio-sim",
+	},
+	.probe = gpio_sim_probe,
+};
+
+struct gpio_sim_chip_config {
+	struct config_item item;
+
+	/*
+	 * If pdev is NULL, the item is 'pending' (waiting for configuration).
+	 * Once the pointer is assigned, the device has been created and the
+	 * item is 'live'.
+	 */
+	struct platform_device *pdev;
+
+	/*
+	 * Each configfs filesystem operation is protected with the subsystem
+	 * mutex. Each separate attribute is protected with the buffer mutex.
+	 * This structure however can be modified by callbacks of different
+	 * attributes so we need another lock.
+	 */
+	struct mutex lock;
+
+	char label[32];
+	unsigned int num_lines;
+	char **line_names;
+	unsigned int num_line_names;
+};
+
+static struct gpio_sim_chip_config *
+to_gpio_sim_chip_config(struct config_item *item)
+{
+	return container_of(item, struct gpio_sim_chip_config, item);
+}
+
+static ssize_t gpio_sim_config_dev_name_show(struct config_item *item,
+					     char *page)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+	struct platform_device *pdev;
+	int ret;
+
+	mutex_lock(&config->lock);
+	pdev = config->pdev;
+	if (pdev && device_is_bound(&pdev->dev))
+		ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
+	else
+		ret = -ENODEV;
+	mutex_unlock(&config->lock);
+
+	return ret;
+}
+
+CONFIGFS_ATTR_RO(gpio_sim_config_, dev_name);
+
+static ssize_t gpio_sim_config_chip_name_show(struct config_item *item,
+					      char *page)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+	struct platform_device *pdev;
+	struct gpio_sim_chip *chip;
+	int ret;
+
+	mutex_lock(&config->lock);
+	pdev = config->pdev;
+	if (pdev && device_is_bound(&pdev->dev)) {
+		chip = dev_get_drvdata(&pdev->dev);
+		ret = sprintf(page, "%s\n", dev_name(&chip->gc.gpiodev->dev));
+	} else {
+		ret = -ENODEV;
+	}
+	mutex_unlock(&config->lock);
+
+	return ret;
+}
+
+CONFIGFS_ATTR_RO(gpio_sim_config_, chip_name);
+
+static ssize_t gpio_sim_config_label_show(struct config_item *item, char *page)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+	int ret;
+
+	mutex_lock(&config->lock);
+	ret = sprintf(page, "%s\n", config->label);
+	mutex_unlock(&config->lock);
+
+	return ret;
+}
+
+static ssize_t gpio_sim_config_label_store(struct config_item *item,
+					   const char *page, size_t count)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+	char *dup, *trimmed;
+	int ret;
+
+	mutex_lock(&config->lock);
+
+	if (config->pdev) {
+		mutex_unlock(&config->lock);
+		return -EBUSY;
+	}
+
+	dup = kstrndup(page, count, GFP_KERNEL);
+	if (!dup) {
+		mutex_unlock(&config->lock);
+		return -ENOMEM;
+	}
+
+	trimmed = strstrip(dup);
+	ret = snprintf(config->label, sizeof(config->label), "%s", trimmed);
+	kfree(dup);
+	if (ret < 0) {
+		mutex_unlock(&config->lock);
+		return ret;
+	}
+
+	mutex_unlock(&config->lock);
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_config_, label);
+
+static ssize_t gpio_sim_config_num_lines_show(struct config_item *item,
+					      char *page)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+	int ret;
+
+	mutex_lock(&config->lock);
+	ret = sprintf(page, "%u\n", config->num_lines);
+	mutex_unlock(&config->lock);
+
+	return ret;
+}
+
+static ssize_t gpio_sim_config_num_lines_store(struct config_item *item,
+					       const char *page, size_t count)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+	unsigned int num_lines;
+	int ret;
+
+	mutex_lock(&config->lock);
+
+	if (config->pdev) {
+		mutex_unlock(&config->lock);
+		return -EBUSY;
+	}
+
+	ret = kstrtouint(page, 10, &num_lines);
+	if (ret) {
+		mutex_unlock(&config->lock);
+		return ret;
+	}
+
+	if (num_lines == 0) {
+		mutex_unlock(&config->lock);
+		return -EINVAL;
+	}
+
+	config->num_lines = num_lines;
+
+	mutex_unlock(&config->lock);
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_config_, num_lines);
+
+static ssize_t gpio_sim_config_line_names_show(struct config_item *item,
+					       char *page)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+	int ret, i, written = 0;
+
+	mutex_lock(&config->lock);
+
+	if (!config->line_names) {

drivers/gpio/gpio-sim.c new file +48 -0

+		if (ret < 0) {
+			mutex_unlock(&config->lock);
+			return ret;
+		}
+
+		written += ret;
+	}
 
+	mutex_unlock(&config->lock);
+	return written;
+}
+
+static ssize_t gpio_sim_config_line_names_store(struct config_item *item,
+						const char *page, size_t count)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+	unsigned int num_new_names = 1, num_old_names, name_idx = 0;
+	bool in_quote = false, got_comma = true;
+	char **new_names, **old_names, *name, c;
+	const char *start = page;
+	size_t pos, name_len;
+	int err = -EINVAL;
+
+	mutex_lock(&config->lock);
+
+	if (config->pdev) {
+		mutex_unlock(&config->lock);
+		return -EBUSY;
+	}
+
+	/*
+	 * Line names are stored in a pointer array so that we can easily
+	 * pass them down to the GPIO subsystem in a "gpio-line-names"
+	 * property.
+	 *
+	 * Line names must be passed as a list of quoted names separated by
+	 * commas, for example: '"foo", "bar", "foobar"'.
+	 */
+
+	for (pos = 0; pos < count; pos++) {
+		/*
+		 * Just count the commas and assume the number if strings
+		 * equals the number of commas + 1. If the format is wrong
+		 * we'll bail out anyway.
+		 */
+		if (page[pos] == ',')
+			num_new_names++;
+	}
+

drivers/gpio/gpio-sim.c new file +11 -0

+	for (pos = 0; pos < count; pos++) {
+		c = page[pos];
+
+		if (in_quote) {
+			if (c == '"') {
+				/* This is the end of the name. */
+				in_quote = got_comma = false;
+				name_len = (page + pos) - start;
+				if (name_len == 0) {
+					/* Name is empty (passed as ""). */
+					name_idx++;

drivers/gpio/gpio-sim.c new file +140 -0

+				new_names[name_idx++] = name;
+			}
+		} else {
+			if (c == '"') {
+				/* Enforce separating names with commas. */
+				if (!got_comma)
+					goto err_out;
+
+				start = page + pos + 1;
+				in_quote = true;
+			} else if (c == ',') {
+				if (!got_comma)
+					got_comma = true;
+				else
+					/* Double commas are not allowed. */
+					goto err_out;
+			} else if (!isspace(c)) {
+				goto err_out;
+			}
+		}
+	}
+
+	/*
+	 * End of input sanity checks, must not have a comma at the end and
+	 * must have finished scanning the last name.
+	 */
+	if (in_quote || got_comma)
+		goto err_out;
+
+	old_names = config->line_names;
+	num_old_names = config->num_line_names;
+	config->line_names = new_names;
+	config->num_line_names = num_new_names;
+
+	mutex_unlock(&config->lock);
+	kfree_strarray(old_names, num_old_names);
+	return count;
+
+err_out:
+	mutex_unlock(&config->lock);
+	kfree_strarray(new_names, name_idx);
+	return err;
+}
+
+CONFIGFS_ATTR(gpio_sim_config_, line_names);
+
+static struct configfs_attribute *gpio_sim_config_attrs[] = {
+	&gpio_sim_config_attr_dev_name,
+	&gpio_sim_config_attr_chip_name,
+	&gpio_sim_config_attr_label,
+	&gpio_sim_config_attr_num_lines,
+	&gpio_sim_config_attr_line_names,
+	NULL
+};
+
+static void gpio_sim_chip_config_release(struct config_item *item)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+
+	mutex_destroy(&config->lock);
+	kfree_strarray(config->line_names, config->num_line_names);
+	kfree(config);
+}
+
+static struct configfs_item_operations gpio_sim_config_item_ops = {
+	.release	= gpio_sim_chip_config_release,
+};
+
+static const struct config_item_type gpio_sim_chip_config_type = {
+	.ct_item_ops	= &gpio_sim_config_item_ops,
+	.ct_attrs	= gpio_sim_config_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_item *
+gpio_sim_config_make_item(struct config_group *group, const char *name)
+{
+	struct gpio_sim_chip_config *config;
+
+	config = kzalloc(sizeof(*config), GFP_KERNEL);
+	if (!config)
+		return ERR_PTR(-ENOMEM);
+
+	config_item_init_type_name(&config->item, name,
+				   &gpio_sim_chip_config_type);
+	config->num_lines = 1;
+	mutex_init(&config->lock);
+
+	return &config->item;
+}
+
+static int gpio_sim_config_commit_item(struct config_item *item)
+{
+	struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+	struct property_entry properties[GPIO_SIM_MAX_PROP];
+	struct platform_device_info pdevinfo;
+	struct platform_device *pdev;
+	unsigned int prop_idx = 0;
+
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+	memset(properties, 0, sizeof(properties));
+
+	mutex_lock(&config->lock);
+
+	properties[prop_idx++] = PROPERTY_ENTRY_U32("gpio-sim,nr-gpios",
+						    config->num_lines);
+
+	if (config->label[0] != '\0')
+		properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
+							       config->label);
+
+	if (config->line_names)
+		properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+						"gpio-line-names",
+						config->line_names,
+						config->num_line_names);
+
+	pdevinfo.id = ida_alloc(&gpio_sim_ida, GFP_KERNEL);
+	if (pdevinfo.id < 0) {
+		mutex_unlock(&config->lock);
+		return pdevinfo.id;
+	}
+
+	pdevinfo.name = "gpio-sim";
+	pdevinfo.properties = properties;
+
+	pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(pdev)) {
+		ida_free(&gpio_sim_ida, pdevinfo.id);
+		mutex_unlock(&config->lock);
+		return PTR_ERR(pdev);
+	}
+
+	config->pdev = pdev;
+	mutex_unlock(&config->lock);
+
+	return 0;
+}
+
+static int gpio_sim_config_uncommit_item(struct config_item *item)

drivers/gpio/gpio-sim.c new file +61 -0

+	mutex_unlock(&config->lock);
+
+	return 0;
+}
+
+static struct configfs_group_operations gpio_sim_config_group_ops = {
+	.make_item	= gpio_sim_config_make_item,
+	.commit_item	= gpio_sim_config_commit_item,
+	.uncommit_item	= gpio_sim_config_uncommit_item,
+};
+
+static const struct config_item_type gpio_sim_config_type = {
+	.ct_group_ops	= &gpio_sim_config_group_ops,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct configfs_subsystem gpio_sim_config_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf	= "gpio-sim",
+			.ci_type	= &gpio_sim_config_type,
+		},
+	},
+};
+
+static int __init gpio_sim_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&gpio_sim_driver);
+	if (ret) {
+		pr_err("Error %d while registering the platform driver\n", ret);
+		return ret;
+	}
+
+	config_group_init(&gpio_sim_config_subsys.su_group);
+	mutex_init(&gpio_sim_config_subsys.su_mutex);
+	ret = configfs_register_subsystem(&gpio_sim_config_subsys);
+	if (ret) {
+		pr_err("Error %d while registering the configfs subsystem %s\n",
+		       ret, gpio_sim_config_subsys.su_group.cg_item.ci_namebuf);
+		mutex_destroy(&gpio_sim_config_subsys.su_mutex);
+		platform_driver_unregister(&gpio_sim_driver);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(gpio_sim_init);
+
+static void __exit gpio_sim_exit(void)
+{
+	configfs_unregister_subsystem(&gpio_sim_config_subsys);
+	mutex_destroy(&gpio_sim_config_subsys.su_mutex);
+	platform_driver_unregister(&gpio_sim_driver);
+}
+module_exit(gpio_sim_exit);
+
+MODULE_AUTHOR("Bartosz Golaszewski <[email protected]>");
+MODULE_DESCRIPTION("GPIO Simulator Module");
+MODULE_LICENSE("GPL");

Signature · 1 line

2.29.1

Quoted 2 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Quoted 5 lines

Implement a new, modern GPIO testing module controlled by configfs attributes instead of module parameters. The goal of this driver is to provide a replacement for gpio-mockup that will be easily extensible with new features and doesn't require reloading the module to change the setup.

Shall we put a reference to this in the gpio-mockup documentation and mark the latter deprecated?

drivers/gpio/gpio-sim.c line 294

Quoted 3 lines

drivers/gpio/gpio-sim.c new file +2 -0

+             dev_attr->attr.name = devm_kasprintf(dev, GFP_KERNEL,
+                                                  "gpio%u", i);

Reads better as one line.

drivers/gpio/gpio-sim.c line 588

Quoted 5 lines

drivers/gpio/gpio-sim.c new file +4 -0

+             ret = sprintf(page + written,
+                     i < config->num_line_names - 1 ?
+                             "\"%s\", " : "\"%s\"\n",
+                     config->line_names[i] ?: "");

Indentation here looks not the best...

drivers/gpio/gpio-sim.c line 647

Quoted 5 lines

drivers/gpio/gpio-sim.c new file +5 -0

+
+     /*
+      * FIXME If anyone knows a better way to parse that - please let me
+      * know.
+      */

If comma can be replaced with ' ' (space) then why not to use next_arg() from cmdline.c? I.o.w. do you have strong opinion why should we use comma here?

Quoted 10 lines

drivers/gpio/gpio-sim.c new file +10 -0

+
+static int gpio_sim_config_uncommit_item(struct config_item *item)
+{
+     struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+     int id;
+
+     mutex_lock(&config->lock);
+     id = config->pdev->id;
+     platform_device_unregister(config->pdev);
+     config->pdev = NULL;

Quoted 1 line

+ ida_free(&gpio_sim_ida, id);

Isn't it atomic per se? I mean that IDA won't give the same ID until you free it. I.o.w. why is it under the mutex?

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

drivers/gpio/gpio-sim.c new file +5 -0

+
+     /*
+      * FIXME If anyone knows a better way to parse that - please let me
+      * know.
+      */

Quoted 3 lines

If comma can be replaced with ' ' (space) then why not to use next_arg() from cmdline.c? I.o.w. do you have strong opinion why should we use comma here?

My opinion is not very strong but I wanted to make the list of names resemble what we pass to the gpio-line-names property in device tree. Doesn't next_arg() react differently to string of the form: "foo=bar"?

Quoted 5 lines

Quoted 1 line

+ ida_free(&gpio_sim_ida, id);

Isn't it atomic per se? I mean that IDA won't give the same ID until you free it. I.o.w. why is it under the mutex?

You're right but if we rapidly create and destroy chips we'll be left with holes in the numbering (because new devices would be created before the IDA numbers are freed, so the driver would take a larger number that's currently free). It doesn't hurt but it would look worse IMO. Do you have a strong opinion on this?

Quoted 11 lines · Bartosz Golaszewski

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

drivers/gpio/gpio-sim.c new file +5 -0

+
+     /*
+      * FIXME If anyone knows a better way to parse that - please let me
+      * know.
+      */

Quoted 7 lines

Quoted 3 lines

If comma can be replaced with ' ' (space) then why not to use next_arg() from cmdline.c? I.o.w. do you have strong opinion why should we use comma here?

My opinion is not very strong but I wanted to make the list of names resemble what we pass to the gpio-line-names property in device tree. Doesn't next_arg() react differently to string of the form: "foo=bar"?

It's ambiguous here.

So, the strings '"foo=bar"' and 'foo=bar' (w/o single quotes!) are indeed parsed differently, i.e.

        '"foo=bar"' -> 'foo=bar',
while
        "foo=bar" -> 'foo' + 'bar'.

...

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

+ ida_free(&gpio_sim_ida, id);

Isn't it atomic per se? I mean that IDA won't give the same ID until you free it. I.o.w. why is it under the mutex?

Quoted 5 lines

You're right but if we rapidly create and destroy chips we'll be left with holes in the numbering (because new devices would be created before the IDA numbers are freed, so the driver would take a larger number that's currently free). It doesn't hurt but it would look worse IMO. Do you have a strong opinion on this?

It's not strong per se, but I would rather follow the 2nd rule of locking:
don't protect something which doesn't need it.

--
With Best Regards,
Andy Shevchenko

Quoted 20 lines · Andy Shevchenko

Quoted 11 lines · Bartosz Golaszewski

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

drivers/gpio/gpio-sim.c new file +5 -0

+
+     /*
+      * FIXME If anyone knows a better way to parse that - please let me
+      * know.
+      */

Quoted 7 lines

Quoted 3 lines

If comma can be replaced with ' ' (space) then why not to use next_arg() from cmdline.c? I.o.w. do you have strong opinion why should we use comma here?

My opinion is not very strong but I wanted to make the list of names resemble what we pass to the gpio-line-names property in device tree. Doesn't next_arg() react differently to string of the form: "foo=bar"?

Quoted 8 lines

It's ambiguous here.

So, the strings '"foo=bar"' and 'foo=bar' (w/o single quotes!) are indeed parsed differently, i.e.

        '"foo=bar"' -> 'foo=bar',
while
        "foo=bar" -> 'foo' + 'bar'.

IMO '"foo", "bar", "", "foobar"' looks better than '"foo" "bar" "" "foobar"' and I'm also not sure next_arg will understand an empty quote?

If you're not objecting strongly, then I would prefer my version.

Quoted 31 lines · Bartosz Golaszewski

Quoted 20 lines · Andy Shevchenko

Quoted 11 lines · Bartosz Golaszewski

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

drivers/gpio/gpio-sim.c new file +5 -0

+
+     /*
+      * FIXME If anyone knows a better way to parse that - please let me
+      * know.
+      */

Quoted 7 lines

Quoted 3 lines

If comma can be replaced with ' ' (space) then why not to use next_arg() from cmdline.c? I.o.w. do you have strong opinion why should we use comma here?

My opinion is not very strong but I wanted to make the list of names resemble what we pass to the gpio-line-names property in device tree. Doesn't next_arg() react differently to string of the form: "foo=bar"?

Quoted 8 lines

It's ambiguous here.

So, the strings '"foo=bar"' and 'foo=bar' (w/o single quotes!) are indeed parsed differently, i.e.

        '"foo=bar"' -> 'foo=bar',
while
        "foo=bar" -> 'foo' + 'bar'.

Quoted 3 lines

IMO '"foo", "bar", "", "foobar"' looks better than '"foo" "bar" "" "foobar"' and I'm also not sure next_arg will understand an empty quote?

I guess it understands it. But I agree that comma-separated it would look better.

Quoted 1 line

If you're not objecting strongly, then I would prefer my version.

I have strong opinion not to open code "yet another parser".

So, grepping on 'strsep(.*, ",")' shows a lot of code that wants something like this. Interesting are the net/9p cases. This in particular pointed out to lib/parser.c which in turn shows promising match_strlcpy() / match_strdup(). I haven't looked deeply though.

That said, I agree that next_arg() is not the best here.

--
With Best Regards,
Andy Shevchenko

Quoted 1 line · Andy Shevchenko

On Mon, Mar 08, 2021 at 03:23:31PM +0100, Bartosz Golaszewski wrote:

Quoted 8 lines

I have strong opinion not to open code "yet another parser".

So, grepping on 'strsep(.*, ",")' shows a lot of code that wants something like this. Interesting are the net/9p cases. This in particular pointed out to lib/parser.c which in turn shows promising match_strlcpy() / match_strdup(). I haven't looked deeply though.

That said, I agree that next_arg() is not the best here.

Shall we revisit this once it's upstream with a generalization for separating comma separated strings?

Quoted 3 lines · Bartosz Golaszewski

Quoted 1 line · Andy Shevchenko

On Mon, Mar 08, 2021 at 03:23:31PM +0100, Bartosz Golaszewski wrote:

...

Quoted 7 lines

Quoted 7 lines

I have strong opinion not to open code "yet another parser".

So, grepping on 'strsep(.*, ",")' shows a lot of code that wants something like this. Interesting are the net/9p cases. This in particular pointed out to lib/parser.c which in turn shows promising match_strlcpy() / match_strdup(). I haven't looked deeply though.

Quoted 4 lines

Quoted 1 line

That said, I agree that next_arg() is not the best here.

Shall we revisit this once it's upstream with a generalization for separating comma separated strings?

How can we guarantee it won't be forgotten?

Quoted 17 lines · Andy Shevchenko

Quoted 3 lines · Bartosz Golaszewski

Quoted 1 line · Andy Shevchenko

On Mon, Mar 08, 2021 at 03:23:31PM +0100, Bartosz Golaszewski wrote:

...

Quoted 9 lines

Quoted 8 lines

I have strong opinion not to open code "yet another parser".

So, grepping on 'strsep(.*, ",")' shows a lot of code that wants something like this. Interesting are the net/9p cases. This in particular pointed out to lib/parser.c which in turn shows promising match_strlcpy() / match_strdup(). I haven't looked deeply though.

That said, I agree that next_arg() is not the best here.

Quoted 5 lines

Quoted 2 lines

Shall we revisit this once it's upstream with a generalization for separating comma separated strings?

How can we guarantee it won't be forgotten?

I will add a REVISIT comment, so *obviously* it ***will*** be revisited. :)