[PATCH v2 00/12] gpio: implement the configfs testing module

[PATCH v2 00/12] gpio: implement the configfs testing module

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This series adds a new GPIO testing module based on configfs committable items
and sysfs. The goal is to provide a testing driver that will be configurable
at runtime (won't need module reload) and easily extensible. The control over
the attributes is also much more fine-grained than in gpio-mockup.

This series also contains a respin of the patches I sent separately to the
configfs maintainers - these patches implement the concept of committable
items that was well defined for a long time but never actually completed.

Apart from the new driver itself, its selftests and the configfs patches, this
series contains some changes to the bitmap API - most importantly: it adds
devres managed variants of bitmap_alloc() and bitmap_zalloc().

v1 -> v2:
- add selftests for gpio-sim
- add helper programs for selftests
- update the configfs rename callback to work with the new API introduced in
  v5.11
- fix a missing quote in the documentation
- use !! whenever using bits operation that are required to return 0 or 1
- use provided bitmap API instead of reimplementing copy or fill operations
- fix a deadlock in gpio_sim_direction_output()
- add new read-only configfs attributes for mapping of configfs items to GPIO
  device names
- and address other minor issues pointed out in reviews of v1

Bartosz Golaszewski (12):
  configfs: increase the item name length
  configfs: use (1UL << bit) for internal flags
  configfs: implement committable items
  samples: configfs: add a committable group
  lib: bitmap: remove the 'extern' keyword from function declarations
  lib: bitmap: order includes alphabetically
  lib: bitmap: provide devm_bitmap_alloc() and devm_bitmap_zalloc()
  drivers: export device_is_bound()
  gpio: sim: new testing module
  selftests: gpio: provide a helper for reading chip info
  selftests: gpio: add a helper for reading GPIO line names
  selftests: gpio: add test cases for gpio-sim

 Documentation/admin-guide/gpio/gpio-sim.rst   |  72 ++
 Documentation/filesystems/configfs.rst        |   6 +-
 drivers/base/dd.c                             |   1 +
 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-sim.c                       | 878 ++++++++++++++++++
 fs/configfs/configfs_internal.h               |  22 +-
 fs/configfs/dir.c                             | 245 ++++-
 include/linux/bitmap.h                        | 129 +--
 include/linux/configfs.h                      |   3 +-
 lib/bitmap.c                                  |  42 +-
 samples/configfs/configfs_sample.c            | 153 +++
 tools/testing/selftests/gpio/.gitignore       |   2 +
 tools/testing/selftests/gpio/Makefile         |   4 +-
 tools/testing/selftests/gpio/config           |   1 +
 tools/testing/selftests/gpio/gpio-chip-info.c |  57 ++
 tools/testing/selftests/gpio/gpio-line-name.c |  55 ++
 tools/testing/selftests/gpio/gpio-sim.sh      | 229 +++++
 18 files changed, 1822 insertions(+), 86 deletions(-)
 create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst
 create mode 100644 drivers/gpio/gpio-sim.c
 create mode 100644 tools/testing/selftests/gpio/gpio-chip-info.c
 create mode 100644 tools/testing/selftests/gpio/gpio-line-name.c
 create mode 100755 tools/testing/selftests/gpio/gpio-sim.sh

-- 
2.29.1

[PATCH v2 01/12] configfs: increase the item name length

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

20 characters limit for item name is relatively small. Let's increase it
to 32 to fit '04-committable-children' - a name we'll use in the sample
code for committable items.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 include/linux/configfs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 2e8c69b43c64..4f76dcc08134 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -27,7 +27,7 @@
 #include <linux/kref.h>   /* struct kref */
 #include <linux/mutex.h>  /* struct mutex */
 
-#define CONFIGFS_ITEM_NAME_LEN	20
+#define CONFIGFS_ITEM_NAME_LEN	32
 
 struct module;
 
-- 
2.29.1

[PATCH v2 02/12] configfs: use (1UL << bit) for internal flags

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

For better readability and maintenance: use the (1UL << bit) for flag
definitions.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 fs/configfs/configfs_internal.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index 9a3aed249692..b495c9f043d4 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -46,16 +46,16 @@ struct configfs_dirent {
 	struct configfs_fragment *s_frag;
 };
 
-#define CONFIGFS_ROOT		0x0001
-#define CONFIGFS_DIR		0x0002
-#define CONFIGFS_ITEM_ATTR	0x0004
-#define CONFIGFS_ITEM_BIN_ATTR	0x0008
-#define CONFIGFS_ITEM_LINK	0x0020
-#define CONFIGFS_USET_DIR	0x0040
-#define CONFIGFS_USET_DEFAULT	0x0080
-#define CONFIGFS_USET_DROPPING	0x0100
-#define CONFIGFS_USET_IN_MKDIR	0x0200
-#define CONFIGFS_USET_CREATING	0x0400
+#define CONFIGFS_ROOT			(1UL << 0)
+#define CONFIGFS_DIR			(1UL << 1)
+#define CONFIGFS_ITEM_ATTR		(1UL << 2)
+#define CONFIGFS_ITEM_BIN_ATTR		(1UL << 3)
+#define CONFIGFS_ITEM_LINK		(1UL << 5)
+#define CONFIGFS_USET_DIR		(1UL << 6)
+#define CONFIGFS_USET_DEFAULT		(1UL << 7)
+#define CONFIGFS_USET_DROPPING		(1UL << 8)
+#define CONFIGFS_USET_IN_MKDIR		(1UL << 9)
+#define CONFIGFS_USET_CREATING		(1UL << 10)
 #define CONFIGFS_NOT_PINNED	(CONFIGFS_ITEM_ATTR | CONFIGFS_ITEM_BIN_ATTR)
 
 extern struct mutex configfs_symlink_mutex;
-- 
2.29.1

[PATCH v2 03/12] configfs: implement committable items

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This implements configfs committable items. We mostly follow the
documentation except that we extend config_group_ops with uncommit_item()
callback for reverting the changes made by commit_item().

Each committable group has two sub-directories: pending and live. New
items can only be created in pending/. Attributes can only be modified
while the item is in pending/. Once it's ready to be committed, it must
be moved over to live/ using the rename() system call. This is when the
commit_item() function will be called.

Implementation-wise: we reuse the default group mechanism to elegantly
plug the new pseude-groups into configfs. The pending group inherits the
parent group's operations so that config_items can be seamlesly created
in it using the callbacks supplied by the user as part of the committable
group itself.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/filesystems/configfs.rst |   6 +-
 fs/configfs/configfs_internal.h        |   2 +
 fs/configfs/dir.c                      | 245 ++++++++++++++++++++++++-
 include/linux/configfs.h               |   1 +
 4 files changed, 245 insertions(+), 9 deletions(-)

diff --git a/Documentation/filesystems/configfs.rst b/Documentation/filesystems/configfs.rst
index 1d3d6f4a82a9..7e0e7c356450 100644
--- a/Documentation/filesystems/configfs.rst
+++ b/Documentation/filesystems/configfs.rst
@@ -290,6 +290,7 @@ config_item_type::
 		struct config_group *(*make_group)(struct config_group *group,
 						   const char *name);
 		int (*commit_item)(struct config_item *item);
+		int (*uncommit_item)(struct config_item *item);
 		void (*disconnect_notify)(struct config_group *group,
 					  struct config_item *item);
 		void (*drop_item)(struct config_group *group,
@@ -490,9 +491,6 @@ pass up an error.
 Committable Items
 =================
 
-Note:
-     Committable items are currently unimplemented.
-
 Some config_items cannot have a valid initial state.  That is, no
 default values can be specified for the item's attributes such that the
 item can do its work.  Userspace must configure one or more attributes,
@@ -532,4 +530,4 @@ method returns zero and the item is moved to the "live" directory.
 As rmdir(2) does not work in the "live" directory, an item must be
 shutdown, or "uncommitted".  Again, this is done via rename(2), this
 time from the "live" directory back to the "pending" one.  The subsystem
-is notified by the ct_group_ops->uncommit_object() method.
+is notified by the ct_group_ops->uncommit_item() method.
diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index b495c9f043d4..41ac21c82bf5 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -56,6 +56,8 @@ struct configfs_dirent {
 #define CONFIGFS_USET_DROPPING		(1UL << 8)
 #define CONFIGFS_USET_IN_MKDIR		(1UL << 9)
 #define CONFIGFS_USET_CREATING		(1UL << 10)
+#define CONFIGFS_GROUP_PENDING		(1UL << 11)
+#define CONFIGFS_GROUP_LIVE		(1UL << 12)
 #define CONFIGFS_NOT_PINNED	(CONFIGFS_ITEM_ATTR | CONFIGFS_ITEM_BIN_ATTR)
 
 extern struct mutex configfs_symlink_mutex;
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index b6098e02e20b..f3c95c1d5278 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -656,6 +656,13 @@ static void detach_groups(struct config_group *group)
 
 		inode_unlock(d_inode(child));
 
+		/*
+		 * Free memory allocated for the pending and live directories
+		 * of committable groups.
+		 */
+		if (sd->s_type & (CONFIGFS_GROUP_PENDING | CONFIGFS_GROUP_LIVE))
+			kfree(sd->s_element);
+
 		d_delete(child);
 		dput(child);
 	}
@@ -860,6 +867,134 @@ static void configfs_detach_item(struct config_item *item)
 	configfs_remove_dir(item);
 }
 
+static bool is_committable_group(struct config_item *item)
+{
+	const struct config_item_type *type = item->ci_type;
+
+	if (type && type->ct_group_ops &&
+	    type->ct_group_ops->commit_item &&
+	    type->ct_group_ops->uncommit_item)
+		return true;
+
+	return false;
+}
+
+struct pending_group_data {
+	struct config_group group;
+	struct config_item_type type;
+	struct configfs_group_operations group_ops;
+};
+
+struct live_group_data {
+	struct config_group group;
+	struct config_item_type type;
+};
+
+static int create_pending_group(struct config_item *parent_item,
+				struct configfs_fragment *frag)
+{
+	const struct config_item_type *parent_type = parent_item->ci_type;
+	struct pending_group_data *pending;
+	struct configfs_dirent *sd;
+	int ret;
+
+	pending = kzalloc(sizeof(*pending), GFP_KERNEL);
+	if (!pending)
+		return -ENOMEM;
+
+	/*
+	 * Let's inherit the group_ops from the parent except for item
+	 * committing and uncommitting.
+	 */
+	memcpy(&pending->group_ops, parent_type->ct_group_ops,
+	       sizeof(struct configfs_group_operations));
+	pending->type.ct_group_ops = &pending->group_ops;
+	pending->type.ct_group_ops->commit_item = NULL;
+	pending->type.ct_group_ops->uncommit_item = NULL;
+
+	/* Let's directly reuse item_ops. */
+	pending->type.ct_item_ops = parent_type->ct_item_ops;
+	pending->type.ct_owner = parent_type->ct_owner;
+
+	config_group_init_type_name(&pending->group, "pending", &pending->type);
+
+	ret = create_default_group(to_config_group(parent_item),
+				   &pending->group, frag);
+	if (ret) {
+		kfree(pending);
+		return ret;
+	}
+
+	link_group(to_config_group(parent_item), &pending->group);
+
+	sd = pending->group.cg_item.ci_dentry->d_fsdata;
+	/* Allow creating config_items in 'pending' group. */
+	sd->s_type |= (CONFIGFS_GROUP_PENDING | CONFIGFS_USET_DIR);
+
+	return 0;
+}
+
+static int create_live_group(struct config_item *parent_item,
+			     struct configfs_fragment *frag)
+{
+	struct live_group_data *live;
+	struct configfs_dirent *sd;
+	int ret;
+
+	live = kzalloc(sizeof(*live), GFP_KERNEL);
+	if (!live)
+		return -ENOMEM;
+
+	live->type.ct_owner = parent_item->ci_type->ct_owner;
+
+	config_group_init_type_name(&live->group, "live", &live->type);
+
+	ret = create_default_group(to_config_group(parent_item),
+				   &live->group, frag);
+	if (ret) {
+		kfree(live);
+		return ret;
+	}
+
+	link_group(to_config_group(parent_item), &live->group);
+
+	sd = live->group.cg_item.ci_dentry->d_fsdata;
+	sd->s_type |= CONFIGFS_GROUP_LIVE;
+	sd->s_type &= ~CONFIGFS_USET_DIR;
+
+	return 0;
+}
+
+static int create_committable_groups(struct config_item *parent_item,
+				     struct configfs_fragment *frag)
+{
+	struct configfs_dirent *sd;
+	int ret;
+
+	ret = create_pending_group(parent_item, frag);
+	if (ret)
+		return ret;
+
+	ret = create_live_group(parent_item, frag);
+	if (ret) {
+		detach_groups(to_config_group(parent_item));
+		return ret;
+	}
+
+	/* Disallow creating items directly in the committable group. */
+	sd = parent_item->ci_dentry->d_fsdata;
+	sd->s_type &= ~CONFIGFS_USET_DIR;
+
+	return 0;
+}
+
+static void dentry_mark_dead(struct config_item *item, struct dentry *dentry)
+{
+	configfs_detach_item(item);
+	d_inode(dentry)->i_flags |= S_DEAD;
+	dont_mount(dentry);
+}
+
 static int configfs_attach_group(struct config_item *parent_item,
 				 struct config_item *item,
 				 struct dentry *dentry,
@@ -885,11 +1020,15 @@ static int configfs_attach_group(struct config_item *parent_item,
 		inode_lock_nested(d_inode(dentry), I_MUTEX_CHILD);
 		configfs_adjust_dir_dirent_depth_before_populate(sd);
 		ret = populate_groups(to_config_group(item), frag);
-		if (ret) {
-			configfs_detach_item(item);
-			d_inode(dentry)->i_flags |= S_DEAD;
-			dont_mount(dentry);
+		if (ret)
+			dentry_mark_dead(item, dentry);
+
+		if (is_committable_group(item)) {
+			ret = create_committable_groups(item, frag);
+			if (ret)
+				dentry_mark_dead(item, dentry);
 		}
+
 		configfs_adjust_dir_dirent_depth_after_populate(sd);
 		inode_unlock(d_inode(dentry));
 		if (ret)
@@ -966,6 +1105,8 @@ static void configfs_dump_one(struct configfs_dirent *sd, int level)
 	type_print(CONFIGFS_USET_DIR);
 	type_print(CONFIGFS_USET_DEFAULT);
 	type_print(CONFIGFS_USET_DROPPING);
+	type_print(CONFIGFS_GROUP_PENDING);
+	type_print(CONFIGFS_GROUP_LIVE);
 #undef type_print
 }
 
@@ -1457,7 +1598,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 	struct config_item *parent_item;
 	struct config_item *item;
 	struct configfs_subsystem *subsys;
-	struct configfs_dirent *sd;
+	struct configfs_dirent *sd, *parent_sd;
 	struct configfs_fragment *frag;
 	struct module *subsys_owner = NULL, *dead_item_owner = NULL;
 	int ret;
@@ -1476,6 +1617,12 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 		return -EINVAL;
 	}
 
+	parent_sd = dentry->d_parent->d_fsdata;
+	if (parent_sd->s_type & CONFIGFS_GROUP_LIVE) {
+		config_item_put(parent_item);
+		return -EPERM;
+	}
+
 	/* configfs_mkdir() shouldn't have allowed this */
 	BUG_ON(!subsys->su_group.cg_item.ci_type);
 	subsys_owner = subsys->su_group.cg_item.ci_type->ct_owner;
@@ -1562,9 +1709,97 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 	return 0;
 }
 
+static int configfs_rename(struct user_namespace *mnt_userns,
+			   struct inode *old_dir, struct dentry *old_dentry,
+			   struct inode *new_dir, struct dentry *new_dentry,
+			   unsigned int flags)
+{
+	struct configfs_dirent *sd, *old_parent_sd, *new_parent_sd;
+	struct dentry *old_parent_dentry, *new_parent_dentry;
+	struct dentry *committable_group_dentry;
+	struct config_item *committable_group_item, *item, *new_parent_item;
+	struct configfs_subsystem *committable_group_subsys;
+	struct configfs_group_operations *committable_group_ops;
+	int ret = 0;
+
+	if (flags)
+		return -EINVAL;
+
+	old_parent_dentry = old_dentry->d_parent;
+	new_parent_dentry = new_dentry->d_parent;
+
+	sd = old_dentry->d_fsdata;
+	old_parent_sd = old_dentry->d_parent->d_fsdata;
+	new_parent_sd = new_dentry->d_parent->d_fsdata;
+
+	if (!old_parent_sd || !new_parent_sd)
+		return -EPERM;
+
+	/*
+	 * Renaming must always be between a 'pending' and a 'live' group and
+	 * both need to have the same parent. Changing the directory name is
+	 * not allowed.
+	 */
+	if (!((old_parent_sd->s_type & CONFIGFS_GROUP_PENDING) &&
+	      (new_parent_sd->s_type & CONFIGFS_GROUP_LIVE)) &&
+	    !((old_parent_sd->s_type & CONFIGFS_GROUP_LIVE) &&
+	      (new_parent_sd->s_type & CONFIGFS_GROUP_PENDING)))
+		return -EPERM;
+
+	if (old_parent_dentry->d_parent != new_parent_dentry->d_parent)
+		return -EPERM;
+
+	if (strcmp(old_dentry->d_name.name, new_dentry->d_name.name))
+		return -EPERM;
+
+	committable_group_dentry = old_parent_dentry->d_parent;
+	/*
+	 * Grab a reference to the committable group for the duration of
+	 * this function.
+	 */
+	committable_group_item =
+		configfs_get_config_item(committable_group_dentry);
+	committable_group_subsys =
+		to_config_group(committable_group_item)->cg_subsys;
+	committable_group_ops = committable_group_item->ci_type->ct_group_ops;
+
+	item = sd->s_element;
+	new_parent_item = new_parent_sd->s_element;
+
+	if (WARN_ON(!is_committable_group(committable_group_item))) {
+		/* This would be a result of a programming error in configfs. */
+		config_item_put(committable_group_item);
+		return -EPERM;
+	}
+
+	mutex_lock(&committable_group_subsys->su_mutex);
+
+	if ((old_parent_sd->s_type & CONFIGFS_GROUP_PENDING) &&
+	    (new_parent_sd->s_type & CONFIGFS_GROUP_LIVE))
+		ret = committable_group_ops->commit_item(item);
+	else
+		ret = committable_group_ops->uncommit_item(item);
+	if (ret)
+		goto out;
+
+	spin_lock(&configfs_dirent_lock);
+	new_dentry->d_fsdata = sd;
+	list_move(&sd->s_sibling, &new_parent_sd->s_children);
+	item->ci_parent = new_parent_item;
+	d_move(old_dentry, new_dentry);
+	spin_unlock(&configfs_dirent_lock);
+
+out:
+	mutex_unlock(&committable_group_subsys->su_mutex);
+	config_item_put(committable_group_item);
+
+	return ret;
+}
+
 const struct inode_operations configfs_dir_inode_operations = {
 	.mkdir		= configfs_mkdir,
 	.rmdir		= configfs_rmdir,
+	.rename		= configfs_rename,
 	.symlink	= configfs_symlink,
 	.unlink		= configfs_unlink,
 	.lookup		= configfs_lookup,
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 4f76dcc08134..ff6b0e408136 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -219,6 +219,7 @@ struct configfs_group_operations {
 	struct config_item *(*make_item)(struct config_group *group, const char *name);
 	struct config_group *(*make_group)(struct config_group *group, const char *name);
 	int (*commit_item)(struct config_item *item);
+	int (*uncommit_item)(struct config_item *item);
 	void (*disconnect_notify)(struct config_group *group, struct config_item *item);
 	void (*drop_item)(struct config_group *group, struct config_item *item);
 };
-- 
2.29.1

[PATCH v2 04/12] samples: configfs: add a committable group

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add an example of using committable items to configfs samples. Each
config item has two attributes: read-write 'storeme' which works
similarly to other examples in this file and a read-only 'committed'
attribute which changes its value between false and true depending on
whether it's committed or not at the moment.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 samples/configfs/configfs_sample.c | 153 +++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/samples/configfs/configfs_sample.c b/samples/configfs/configfs_sample.c
index f9008be7a8a1..9bef74e4369d 100644
--- a/samples/configfs/configfs_sample.c
+++ b/samples/configfs/configfs_sample.c
@@ -315,6 +315,158 @@ static struct configfs_subsystem group_children_subsys = {
 
 /* ----------------------------------------------------------------- */
 
+/*
+ * 04-committable-children
+ *
+ * This is an example of a committable group.  It's similar to the simple
+ * children example but each config_item has an additional 'committed'
+ * attribute which is read-only and is only modified when the config_item
+ * is moved from the 'pending' to the 'live' directory.
+ */
+
+struct committable_child {
+	struct config_item item;
+	int storeme;
+	bool committed;
+};
+
+static inline struct committable_child *
+to_committable_child(struct config_item *item)
+{
+	return container_of(item, struct committable_child, item);
+}
+
+static ssize_t
+committable_child_storeme_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%d\n", to_committable_child(item)->storeme);
+}
+
+static ssize_t committable_child_storeme_store(struct config_item *item,
+					       const char *page, size_t count)
+{
+	struct committable_child *child = to_committable_child(item);
+	int ret;
+
+	if (child->committed)
+		return -EPERM;
+
+	ret = kstrtoint(page, 10, &child->storeme);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+CONFIGFS_ATTR(committable_child_, storeme);
+
+static ssize_t
+committable_child_committed_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%s\n",
+		to_committable_child(item)->committed ? "true" : "false");
+}
+
+CONFIGFS_ATTR_RO(committable_child_, committed);
+
+static struct configfs_attribute *committable_child_attrs[] = {
+	&committable_child_attr_storeme,
+	&committable_child_attr_committed,
+	NULL,
+};
+
+static void committable_child_release(struct config_item *item)
+{
+	kfree(to_committable_child(item));
+}
+
+static struct configfs_item_operations committable_child_item_ops = {
+	.release	= committable_child_release,
+};
+
+static const struct config_item_type committable_child_type = {
+	.ct_item_ops	= &committable_child_item_ops,
+	.ct_attrs	= committable_child_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+struct committable_children {
+	struct config_group group;
+};
+
+static struct config_item *
+committable_children_make_item(struct config_group *group, const char *name)
+{
+	struct committable_child *child;
+
+	child = kzalloc(sizeof(*child), GFP_KERNEL);
+	if (!child)
+		return ERR_PTR(-ENOMEM);
+
+	config_item_init_type_name(&child->item, name, &committable_child_type);
+
+	return &child->item;
+}
+
+static ssize_t
+committable_children_description_show(struct config_item *item, char *page)
+{
+	return sprintf(page,
+"[04-committable-children]\n"
+"\n"
+"This subsystem allows creation of committable config_items.  The subsystem\n"
+"has two subdirectories: pending and live.  New config_items can only be\n"
+"created in pending/ and they have one writable and readable attribute as\n"
+"well as a single read-only attribute.  The latter is only changed once the\n"
+"item is 'committed'.  This is done by moving the config_item (using\n"
+"rename()) to the live/ directory.  In this example, the storeme attribute\n"
+"becomes 'read-only' once committed.\n");
+}
+
+CONFIGFS_ATTR_RO(committable_children_, description);
+
+static struct configfs_attribute *committable_children_attrs[] = {
+	&committable_children_attr_description,
+	NULL,
+};
+
+static int committable_children_commit_item(struct config_item *item)
+{
+	to_committable_child(item)->committed = true;
+
+	return 0;
+}
+
+static int committable_children_uncommit_item(struct config_item *item)
+{
+	to_committable_child(item)->committed = false;
+
+	return 0;
+}
+
+static struct configfs_group_operations committable_children_group_ops = {
+	.make_item	= committable_children_make_item,
+	.commit_item	= committable_children_commit_item,
+	.uncommit_item	= committable_children_uncommit_item,
+};
+
+static const struct config_item_type committable_children_type = {
+	.ct_group_ops	= &committable_children_group_ops,
+	.ct_attrs	= committable_children_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct configfs_subsystem committable_children_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "04-committable-children",
+			.ci_type = &committable_children_type,
+		},
+	},
+};
+
+/* ----------------------------------------------------------------- */
+
 /*
  * We're now done with our subsystem definitions.
  * For convenience in this module, here's a list of them all.  It
@@ -326,6 +478,7 @@ static struct configfs_subsystem *example_subsys[] = {
 	&childless_subsys.subsys,
 	&simple_children_subsys,
 	&group_children_subsys,
+	&committable_children_subsys,
 	NULL,
 };
 
-- 
2.29.1

[PATCH v2 05/12] lib: bitmap: remove the 'extern' keyword from function declarations

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/bitmap.h | 115 ++++++++++++++++++++---------------------
 1 file changed, 57 insertions(+), 58 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 70a932470b2d..6939a8983026 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -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);
-extern void __bitmap_shift_right(unsigned long *dst, const unsigned long *src,
-				unsigned int shift, unsigned int nbits);
-extern void __bitmap_shift_left(unsigned long *dst, const unsigned long *src,
-				unsigned int shift, unsigned int nbits);
-extern void bitmap_cut(unsigned long *dst, const unsigned long *src,
-		       unsigned int first, unsigned int cut,
-		       unsigned int nbits);
-extern int __bitmap_and(unsigned long *dst, const unsigned long *bitmap1,
+int __bitmap_equal(const unsigned long *bitmap1,
+		   const unsigned long *bitmap2, unsigned int nbits);
+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
@@ -190,33 +189,33 @@ bitmap_find_next_zero_area(unsigned long *map,
 					      align_mask, 0);
 }
 
-extern int bitmap_parse(const char *buf, unsigned int buflen,
+int bitmap_parse(const char *buf, unsigned int buflen,
 			unsigned long *dst, int nbits);
-extern int bitmap_parse_user(const char __user *ubuf, unsigned int ulen,
+int bitmap_parse_user(const char __user *ubuf, unsigned int ulen,
 			unsigned long *dst, int nbits);
-extern int bitmap_parselist(const char *buf, unsigned long *maskp,
+int bitmap_parselist(const char *buf, unsigned long *maskp,
 			int nmaskbits);
-extern int bitmap_parselist_user(const char __user *ubuf, unsigned int ulen,
+int bitmap_parselist_user(const char __user *ubuf, unsigned int ulen,
 			unsigned long *dst, int nbits);
-extern void bitmap_remap(unsigned long *dst, const unsigned long *src,
+void bitmap_remap(unsigned long *dst, const unsigned long *src,
 		const unsigned long *old, const unsigned long *new, unsigned int nbits);
-extern int bitmap_bitremap(int oldbit,
+int bitmap_bitremap(int oldbit,
 		const unsigned long *old, const unsigned long *new, int bits);
-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)))
@@ -265,9 +264,9 @@ static inline void bitmap_copy_clear_tail(unsigned long *dst,
  * therefore conversion is not needed when copying data from/to arrays of u32.
  */
 #if BITS_PER_LONG == 64
-extern void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
+void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
 							unsigned int nbits);
-extern void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
+void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
 							unsigned int nbits);
 #else
 #define bitmap_from_arr32(bitmap, buf, nbits)			\
-- 
2.29.1

Re: [PATCH v2 05/12] lib: bitmap: remove the 'extern' keyword from function declarations

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 11:24:45AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The 'extern' keyword doesn't have any benefits in header files. Remove it.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
A few nitpicks below.

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/bitmap.h | 115 ++++++++++++++++++++---------------------
>  1 file changed, 57 insertions(+), 58 deletions(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 70a932470b2d..6939a8983026 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -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);
> -extern void __bitmap_shift_right(unsigned long *dst, const unsigned long *src,
> -				unsigned int shift, unsigned int nbits);
> -extern void __bitmap_shift_left(unsigned long *dst, const unsigned long *src,
> -				unsigned int shift, unsigned int nbits);
> -extern void bitmap_cut(unsigned long *dst, const unsigned long *src,
> -		       unsigned int first, unsigned int cut,
> -		       unsigned int nbits);
> -extern int __bitmap_and(unsigned long *dst, const unsigned long *bitmap1,
> +int __bitmap_equal(const unsigned long *bitmap1,
> +		   const unsigned long *bitmap2, unsigned int nbits);

Logically better to have

int __bitmap_equal(const unsigned long *bitmap1, const unsigned long *bitmap2,
		   unsigned int nbits);

> +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
> @@ -190,33 +189,33 @@ bitmap_find_next_zero_area(unsigned long *map,
>  					      align_mask, 0);
>  }
>  
> -extern int bitmap_parse(const char *buf, unsigned int buflen,
> +int bitmap_parse(const char *buf, unsigned int buflen,
>  			unsigned long *dst, int nbits);

Can be

int bitmap_parse(const char *buf, unsigned int buflen, unsigned long *dst,
		 int nbits);

And I wonder why nbits here is signed.

> -extern int bitmap_parse_user(const char __user *ubuf, unsigned int ulen,
> +int bitmap_parse_user(const char __user *ubuf, unsigned int ulen,
>  			unsigned long *dst, int nbits);

Ditto.

> -extern int bitmap_parselist(const char *buf, unsigned long *maskp,

> +int bitmap_parselist(const char *buf, unsigned long *maskp,
>  			int nmaskbits);

Now can be one line.

> -extern int bitmap_parselist_user(const char __user *ubuf, unsigned int ulen,
> +int bitmap_parselist_user(const char __user *ubuf, unsigned int ulen,
>  			unsigned long *dst, int nbits);
> -extern void bitmap_remap(unsigned long *dst, const unsigned long *src,
> +void bitmap_remap(unsigned long *dst, const unsigned long *src,
>  		const unsigned long *old, const unsigned long *new, unsigned int nbits);
> -extern int bitmap_bitremap(int oldbit,

> +int bitmap_bitremap(int oldbit,
>  		const unsigned long *old, const unsigned long *new, int bits);

More logical

int bitmap_bitremap(int oldbit, const unsigned long *old,
		    const unsigned long *new, int bits);

Or even

int bitmap_bitremap(int oldbit, const unsigned long *old, const unsigned long *new,
		    int bits);

> -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)))
> @@ -265,9 +264,9 @@ static inline void bitmap_copy_clear_tail(unsigned long *dst,
>   * therefore conversion is not needed when copying data from/to arrays of u32.
>   */
>  #if BITS_PER_LONG == 64
> -extern void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
> +void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
>  							unsigned int nbits);

One line?

> -extern void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
> +void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
>  							unsigned int nbits);

Ditto.

>  #else
>  #define bitmap_from_arr32(bitmap, buf, nbits)			\
> -- 
> 2.29.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 05/12] lib: bitmap: remove the 'extern' keyword from function declarations

[Bartosz Golaszewski]

On Thu, Mar 4, 2021 at 1:59 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 04, 2021 at 11:24:45AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > The 'extern' keyword doesn't have any benefits in header files. Remove it.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> A few nitpicks below.
>
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---

Hi Andy,

regarding this patch and other places where you raise issues with line
breaking: I believe this is purely a question of taste. There are no
guidelines on line breaking in the docs. I will leave it as it is here
because it's not better or worse than your version, just different.
Same for exceeding 80 characters - I personally believe it's justified
when the line looks better but whenever it can be cleanly broken, it's
better to stay within the limit.

Best Regards,
Bartosz

[PATCH v2 06/12] lib: bitmap: order includes alphabetically

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/bitmap.h | 4 ++--
 lib/bitmap.c           | 9 +++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 6939a8983026..3282db97e06c 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -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
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 75006c4036e9..78f70d9007ad 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -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>
-- 
2.29.1

Re: [PATCH v2 06/12] lib: bitmap: order includes alphabetically

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 11:24:46AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> For better readability and maintenance: order the includes in bitmap
> source files alphabetically.

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

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/bitmap.h | 4 ++--
>  lib/bitmap.c           | 9 +++++----
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 6939a8983026..3282db97e06c 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -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
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 75006c4036e9..78f70d9007ad 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -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>
> -- 
> 2.29.1
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 07/12] lib: bitmap: provide devm_bitmap_alloc() and devm_bitmap_zalloc()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/bitmap.h | 10 ++++++++++
 lib/bitmap.c           | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 3282db97e06c..e41c622db1b8 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -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
@@ -122,6 +124,14 @@ 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);
 
+/*
+ * Managed variants of the above.
+ */
+unsigned long *devm_bitmap_alloc(struct device *dev,
+				 unsigned int nbits, gfp_t flags);
+unsigned long *devm_bitmap_zalloc(struct device *dev,
+				  unsigned int nbits, gfp_t flags);
+
 /*
  * lib/bitmap.c provides these functions:
  */
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 78f70d9007ad..b4fd7fd084c6 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -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);
 
+static void devm_bitmap_free(void *data)
+{
+	unsigned long *bitmap = data;
+
+	bitmap_free(bitmap);
+}
+
+unsigned long *devm_bitmap_alloc(struct device *dev,
+				 unsigned int nbits, gfp_t flags)
+{
+	unsigned long *bitmap;
+	int ret;
+
+	bitmap = bitmap_alloc(nbits, flags);
+	if (!bitmap)
+		return NULL;
+
+	ret = devm_add_action_or_reset(dev, devm_bitmap_free, bitmap);
+	if (ret)
+		return NULL;
+
+	return bitmap;
+}
+EXPORT_SYMBOL(devm_bitmap_alloc);
+
+unsigned long *devm_bitmap_zalloc(struct device *dev,
+				  unsigned int nbits, gfp_t flags)
+{
+	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
-- 
2.29.1

Re: [PATCH v2 07/12] lib: bitmap: provide devm_bitmap_alloc() and devm_bitmap_zalloc()

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 11:24:47AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Provide managed variants of bitmap_alloc() and bitmap_zalloc().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
A few nit-picks below.

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/bitmap.h | 10 ++++++++++
>  lib/bitmap.c           | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 3282db97e06c..e41c622db1b8 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -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
> @@ -122,6 +124,14 @@ 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);

> +/*
> + * Managed variants of the above.
> + */

One line?

> +unsigned long *devm_bitmap_alloc(struct device *dev,
> +				 unsigned int nbits, gfp_t flags);
> +unsigned long *devm_bitmap_zalloc(struct device *dev,
> +				  unsigned int nbits, gfp_t flags);

Both can be oneliners.

>  /*
>   * lib/bitmap.c provides these functions:
>   */
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 78f70d9007ad..b4fd7fd084c6 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -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);
>  
> +static void devm_bitmap_free(void *data)
> +{
> +	unsigned long *bitmap = data;
> +
> +	bitmap_free(bitmap);
> +}

> +unsigned long *devm_bitmap_alloc(struct device *dev,
> +				 unsigned int nbits, gfp_t flags)

One line?

> +{
> +	unsigned long *bitmap;
> +	int ret;
> +
> +	bitmap = bitmap_alloc(nbits, flags);
> +	if (!bitmap)
> +		return NULL;
> +
> +	ret = devm_add_action_or_reset(dev, devm_bitmap_free, bitmap);
> +	if (ret)
> +		return NULL;
> +
> +	return bitmap;
> +}
> +EXPORT_SYMBOL(devm_bitmap_alloc);

> +unsigned long *devm_bitmap_zalloc(struct device *dev,
> +				  unsigned int nbits, gfp_t flags)

One line?

> +{
> +	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
> -- 
> 2.29.1
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 08/12] drivers: export device_is_bound()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9179825ff646..c62c02e3490a 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -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

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Geert Uytterhoeven]

CC Greg

On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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 <bgolaszewski@baylibre.com>
> ---
>  drivers/base/dd.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9179825ff646..c62c02e3490a 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -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

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Greg KH]

On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> CC Greg
> 
> On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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 <bgolaszewski@baylibre.com>
> > ---
> >  drivers/base/dd.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 9179825ff646..c62c02e3490a 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -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.

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Bartosz Golaszewski]

On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > CC Greg
> >
> > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > 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 <bgolaszewski@baylibre.com>
> > > ---
> > >  drivers/base/dd.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > index 9179825ff646..c62c02e3490a 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -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.
>

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().

Best Regards,
Bartosz

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Greg KH]

On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > > CC Greg
> > >
> > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > 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 <bgolaszewski@baylibre.com>
> > > > ---
> > > >  drivers/base/dd.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > index 9179825ff646..c62c02e3490a 100644
> > > > --- a/drivers/base/dd.c
> > > > +++ b/drivers/base/dd.c
> > > > @@ -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.
> >
> 
> 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.

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?

> 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.

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.

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

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

thanks,

greg k-h

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Bartosz Golaszewski]

On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > > > CC Greg
> > > >
> > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > >
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > >
> > > > > 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 <bgolaszewski@baylibre.com>
> > > > > ---
> > > > >  drivers/base/dd.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > index 9179825ff646..c62c02e3490a 100644
> > > > > --- a/drivers/base/dd.c
> > > > > +++ b/drivers/base/dd.c
> > > > > @@ -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.
> > >
> >
> > 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.
>
> 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.

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

> > 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.
>
> 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.

> > Please advise on how to handle it without device_is_bound().
>
> Please do not create sysfs files before they can be read from :)
>

Don't worry, I don't.

Bartosz

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Greg KH]

On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > > > > CC Greg
> > > > >
> > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > >
> > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > > >
> > > > > > 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 <bgolaszewski@baylibre.com>
> > > > > > ---
> > > > > >  drivers/base/dd.c | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > index 9179825ff646..c62c02e3490a 100644
> > > > > > --- a/drivers/base/dd.c
> > > > > > +++ b/drivers/base/dd.c
> > > > > > @@ -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.
> > > >
> > >
> > > 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.
> >
> > 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.
> 
> 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.

> > > 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.
> >
> > 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.

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.

thanks,

greg k-h

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Bartosz Golaszewski]

On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > > > > > CC Greg
> > > > > >
> > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > >
> > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > > > >
> > > > > > > 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 <bgolaszewski@baylibre.com>
> > > > > > > ---
> > > > > > >  drivers/base/dd.c | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > index 9179825ff646..c62c02e3490a 100644
> > > > > > > --- a/drivers/base/dd.c
> > > > > > > +++ b/drivers/base/dd.c
> > > > > > > @@ -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.
> > > > >
> > > >
> > > > 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.
> > >
> > > 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.
> >
> > 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.
>
> > > > 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.
> > >
> > > 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.
>
> 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.

Configfs doesn't even allow to dynamically export and unexport attributes.

Bart

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Greg KH]

On Fri, Mar 05, 2021 at 11:58:18AM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> > > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > > > > > > CC Greg
> > > > > > >
> > > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > >
> > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > > > > >
> > > > > > > > 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 <bgolaszewski@baylibre.com>
> > > > > > > > ---
> > > > > > > >  drivers/base/dd.c | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > > index 9179825ff646..c62c02e3490a 100644
> > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > @@ -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.
> > > > > >
> > > > >
> > > > > 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.
> > > >
> > > > 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.
> > >
> > > 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.
> >
> > > > > 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.
> > > >
> > > > 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.
> >
> > 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.

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.

thanks,

greg k-h

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Bartosz Golaszewski]

On Fri, Mar 5, 2021 at 12:27 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 05, 2021 at 11:58:18AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> > > > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > CC Greg
> > > > > > > >
> > > > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > > >
> > > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > > > > > >
> > > > > > > > > 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 <bgolaszewski@baylibre.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/base/dd.c | 1 +
> > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > > > index 9179825ff646..c62c02e3490a 100644
> > > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > > @@ -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.
> > > > > > >
> > > > > >
> > > > > > 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.
> > > > >
> > > > > 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.
> > > >
> > > > 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.

> > > > > > 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.
> > > > >
> > > > > 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.
> > >
> > > 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.
>
> 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?

> 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.

Bartosz

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Greg KH]

On Fri, Mar 05, 2021 at 03:20:27PM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 5, 2021 at 12:27 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Mar 05, 2021 at 11:58:18AM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:
> > > > > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > CC Greg
> > > > > > > > >
> > > > > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > > > >
> > > > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > > > > > > >
> > > > > > > > > > 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 <bgolaszewski@baylibre.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/base/dd.c | 1 +
> > > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > > > > index 9179825ff646..c62c02e3490a 100644
> > > > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > > > @@ -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.
> > > > > > > >
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > 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.
> > > > >
> > > > > 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.

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.

> 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 :)

> 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.

> > > > > > > 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.
> > > > > >
> > > > > > 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.
> > > >
> > > > 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.
> >
> > 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?

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

> > 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.

Device names can change over time, nothing new there.

thanks,

greg k-h

Re: [PATCH v2 08/12] drivers: export device_is_bound()

[Bartosz Golaszewski]

On Fri, Mar 5, 2021 at 4:01 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 05, 2021 at 03:20:27PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Mar 5, 2021 at 12:27 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Mar 05, 2021 at 11:58:18AM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:
> > > > > > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> > > > > > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > > CC Greg
> > > > > > > > > >
> > > > > > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > > > > >
> > > > > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > > > > > > > >
> > > > > > > > > > > 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 <bgolaszewski@baylibre.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/base/dd.c | 1 +
> > > > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > > > > > index 9179825ff646..c62c02e3490a 100644
> > > > > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > > > > @@ -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.
> > > > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > 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.
>
> 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.
>
> > 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 :)
>

My bad. Yes we need to use DT. And platform device does sound like the
best approach.

> > 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.
>

I'm fine with that if it can be instantiated from DT but it doesn't seem so.

> > > > > > > > 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.
> > > > > > >
> > > > > > > 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.
> > > > >
> > > > > 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.
> > >
> > > 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?
>
> Sounds reasonable, I don't know how configfs works, it's been a decade
> since I last touched it.
>
> > > 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.
>
> Device names can change over time, nothing new there.
>

Ok will change in v3. I'll Cc you next time.

Bart

[PATCH v2 09/12] gpio: sim: new testing module

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 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

diff --git a/Documentation/admin-guide/gpio/gpio-sim.rst b/Documentation/admin-guide/gpio/gpio-sim.rst
new file mode 100644
index 000000000000..08eac487e35e
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-sim.rst
@@ -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).
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..b6b6150d0d04 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -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
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..d717aa85f8e1 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -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
diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
new file mode 100644
index 000000000000..0ed297127bca
--- /dev/null
+++ b/drivers/gpio/gpio-sim.c
@@ -0,0 +1,878 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * GPIO testing driver based on configfs.
+ *
+ * Copyright (C) 2021 Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ */
+
+#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++) {
+		line_attr = devm_kzalloc(dev, sizeof(*line_attr), GFP_KERNEL);
+		if (!line_attr)
+			return -ENOMEM;
+
+		line_attr->offset = i;
+
+		dev_attr = &line_attr->dev_attr;
+
+		dev_attr->attr.name = devm_kasprintf(dev, GFP_KERNEL,
+						     "gpio%u", i);
+		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;
+
+	ret = device_property_read_string(dev, "gpio-sim,label", &label);
+	if (ret)
+		label = dev_name(dev);
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->directions = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->directions)
+		return -ENOMEM;
+
+	/* Default to input mode. */
+	bitmap_fill(chip->directions, num_lines);
+
+	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) {
+		mutex_unlock(&config->lock);
+		return sprintf(page, "\n");
+	}
+
+	for (i = 0; i < config->num_line_names; i++) {
+		ret = sprintf(page + written,
+			i < config->num_line_names - 1 ?
+				"\"%s\", " : "\"%s\"\n",
+			config->line_names[i] ?: "");
+		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++;
+	}
+
+	new_names = kcalloc(num_new_names, sizeof(char *), GFP_KERNEL);
+	if (!new_names) {
+		mutex_unlock(&config->lock);
+		return -ENOMEM;
+	}
+
+	/*
+	 * FIXME If anyone knows a better way to parse that - please let me
+	 * know.
+	 */
+	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++;
+					continue;
+				}
+
+				name = kzalloc(name_len + 1, GFP_KERNEL);
+				if (!name) {
+					err = -ENOMEM;
+					goto err_out;
+				}
+
+				memcpy(name, start, name_len);
+				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)
+{
+	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;
+	ida_free(&gpio_sim_ida, id);
+	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 <bgolaszewski@baylibre.com>");
+MODULE_DESCRIPTION("GPIO Simulator Module");
+MODULE_LICENSE("GPL");
-- 
2.29.1

Re: [PATCH v2 09/12] gpio: sim: new testing module

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 11:24:49AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> 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?


> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  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
> 
> diff --git a/Documentation/admin-guide/gpio/gpio-sim.rst b/Documentation/admin-guide/gpio/gpio-sim.rst
> new file mode 100644
> index 000000000000..08eac487e35e
> --- /dev/null
> +++ b/Documentation/admin-guide/gpio/gpio-sim.rst
> @@ -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).
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e3607ec4c2e8..b6b6150d0d04 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -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
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c58a90a3c3b1..d717aa85f8e1 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -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
> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> new file mode 100644
> index 000000000000..0ed297127bca
> --- /dev/null
> +++ b/drivers/gpio/gpio-sim.c
> @@ -0,0 +1,878 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * GPIO testing driver based on configfs.
> + *
> + * Copyright (C) 2021 Bartosz Golaszewski <bgolaszewski@baylibre.com>
> + */
> +
> +#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++) {
> +		line_attr = devm_kzalloc(dev, sizeof(*line_attr), GFP_KERNEL);
> +		if (!line_attr)
> +			return -ENOMEM;
> +
> +		line_attr->offset = i;
> +
> +		dev_attr = &line_attr->dev_attr;

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

Reads better as one line.

> +		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;
> +
> +	ret = device_property_read_string(dev, "gpio-sim,label", &label);
> +	if (ret)
> +		label = dev_name(dev);
> +
> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->directions = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);

What's the point to have "z" variant here...

> +	if (!chip->directions)
> +		return -ENOMEM;
> +
> +	/* Default to input mode. */
> +	bitmap_fill(chip->directions, num_lines);

...followed by filling all 1:s here?

> +	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) {
> +		mutex_unlock(&config->lock);
> +		return sprintf(page, "\n");
> +	}
> +
> +	for (i = 0; i < config->num_line_names; i++) {

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

Indentation here looks not the best...

> +		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++;
> +	}
> +
> +	new_names = kcalloc(num_new_names, sizeof(char *), GFP_KERNEL);
> +	if (!new_names) {
> +		mutex_unlock(&config->lock);
> +		return -ENOMEM;
> +	}
> +
> +	/*
> +	 * 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?

> +	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++;
> +					continue;
> +				}

> +				name = kzalloc(name_len + 1, GFP_KERNEL);
> +				if (!name) {
> +					err = -ENOMEM;
> +					goto err_out;
> +				}
> +
> +				memcpy(name, start, name_len);

kmemdup()/kstrndup() or so?

> +				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)
> +{
> +	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;

> +	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?

> +	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 <bgolaszewski@baylibre.com>");
> +MODULE_DESCRIPTION("GPIO Simulator Module");
> +MODULE_LICENSE("GPL");
> -- 
> 2.29.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/12] gpio: sim: new testing module

[Bartosz Golaszewski]

On Thu, Mar 4, 2021 at 2:15 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 04, 2021 at 11:24:49AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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?
>

I don't think it's necessary right away. Let's phase out gpio-mockup
once this one gets some attention (for example: after libgpiod
switches to using it).

[snip]

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

Yeah, so the removal of the 80 characters limit should not be abused
when there's no need for it - this doesn't look that bad really with a
broken line. Same elsewhere where the limit is exceeded.

[snip]

>
> > +             ret = sprintf(page + written,
> > +                     i < config->num_line_names - 1 ?
> > +                             "\"%s\", " : "\"%s\"\n",
> > +                     config->line_names[i] ?: "");
>
> Indentation here looks not the best...
>

So this is the place where it may make sense to go over 80 chars.

[snip]

> > +
> > +     /*
> > +      * 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?
>

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"?

[snip]

> > +
> > +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;
>
> > +     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?

[snip]

I'll address issues I didn't comment on.

Thanks for the review!
Bart

Re: [PATCH v2 09/12] gpio: sim: new testing module

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 09:15:29PM +0100, Bartosz Golaszewski wrote:
> On Thu, Mar 4, 2021 at 2:15 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Mar 04, 2021 at 11:24:49AM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > +
> > > +     /*
> > > +      * 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?
> >
> 
> 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'.

...

> > > +     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?

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

Re: [PATCH v2 09/12] gpio: sim: new testing module

[Bartosz Golaszewski]

On Fri, Mar 5, 2021 at 11:15 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 04, 2021 at 09:15:29PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Mar 4, 2021 at 2:15 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Mar 04, 2021 at 11:24:49AM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> > > > +
> > > > +     /*
> > > > +      * 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?
> > >
> >
> > 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'.
>

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.

> ...
>
> > > > +     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?
>
> 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.
>

OK, makes sense.

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v2 09/12] gpio: sim: new testing module

[Andy Shevchenko]

On Mon, Mar 08, 2021 at 03:23:31PM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 5, 2021 at 11:15 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Mar 04, 2021 at 09:15:29PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Mar 4, 2021 at 2:15 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Mar 04, 2021 at 11:24:49AM +0100, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > +
> > > > > +     /*
> > > > > +      * 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?
> > > >
> > >
> > > 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'.
> >
> 
> 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.

> 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

Re: [PATCH v2 09/12] gpio: sim: new testing module

[Bartosz Golaszewski]

On Mon, Mar 8, 2021 at 4:05 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Mar 08, 2021 at 03:23:31PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Mar 5, 2021 at 11:15 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Mar 04, 2021 at 09:15:29PM +0100, Bartosz Golaszewski wrote:
> > > > On Thu, Mar 4, 2021 at 2:15 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Thu, Mar 04, 2021 at 11:24:49AM +0100, Bartosz Golaszewski wrote:
> > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> > > > > > +
> > > > > > +     /*
> > > > > > +      * 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?
> > > > >
> > > >
> > > > 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'.
> > >
> >
> > 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.
>
> > 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
>
>

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

Bart

Re: [PATCH v2 09/12] gpio: sim: new testing module

[Andy Shevchenko]

On Mon, Mar 08, 2021 at 04:13:33PM +0100, Bartosz Golaszewski wrote:
> On Mon, Mar 8, 2021 at 4:05 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Mar 08, 2021 at 03:23:31PM +0100, Bartosz Golaszewski wrote:

...

> > 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?

How can we guarantee it won't be forgotten?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/12] gpio: sim: new testing module

[Bartosz Golaszewski]

On Mon, Mar 8, 2021 at 4:32 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Mar 08, 2021 at 04:13:33PM +0100, Bartosz Golaszewski wrote:
> > On Mon, Mar 8, 2021 at 4:05 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Mar 08, 2021 at 03:23:31PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > 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?
>
> How can we guarantee it won't be forgotten?
>

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

Bartosz

Re: [PATCH v2 09/12] gpio: sim: new testing module

[Andy Shevchenko]

On Mon, Mar 08, 2021 at 04:37:10PM +0100, Bartosz Golaszewski wrote:
> On Mon, Mar 8, 2021 at 4:32 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Mar 08, 2021 at 04:13:33PM +0100, Bartosz Golaszewski wrote:
> > > On Mon, Mar 8, 2021 at 4:05 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Mar 08, 2021 at 03:23:31PM +0100, Bartosz Golaszewski wrote:
> >
> > ...
> >
> > > > 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?
> >
> > How can we guarantee it won't be forgotten?
> >
> 
> I will add a REVISIT comment, so *obviously* it ***will*** be revisited. :)

Fine by me!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 10/12] selftests: gpio: provide a helper for reading chip info

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a simple program that allows to retrieve chip properties from the
GPIO character device. This will be used in gpio-sim selftests.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 tools/testing/selftests/gpio/.gitignore       |  1 +
 tools/testing/selftests/gpio/Makefile         |  2 +-
 tools/testing/selftests/gpio/gpio-chip-info.c | 57 +++++++++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/gpio/gpio-chip-info.c

diff --git a/tools/testing/selftests/gpio/.gitignore b/tools/testing/selftests/gpio/.gitignore
index a4969f7ee020..4ea4f58dab1a 100644
--- a/tools/testing/selftests/gpio/.gitignore
+++ b/tools/testing/selftests/gpio/.gitignore
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 gpio-mockup-cdev
+gpio-chip-info
diff --git a/tools/testing/selftests/gpio/Makefile b/tools/testing/selftests/gpio/Makefile
index 39f2bbe8dd3d..84b48547f94c 100644
--- a/tools/testing/selftests/gpio/Makefile
+++ b/tools/testing/selftests/gpio/Makefile
@@ -2,6 +2,6 @@
 
 TEST_PROGS := gpio-mockup.sh
 TEST_FILES := gpio-mockup-sysfs.sh
-TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev
+TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev gpio-chip-info
 
 include ../lib.mk
diff --git a/tools/testing/selftests/gpio/gpio-chip-info.c b/tools/testing/selftests/gpio/gpio-chip-info.c
new file mode 100644
index 000000000000..4d26fa7c254a
--- /dev/null
+++ b/tools/testing/selftests/gpio/gpio-chip-info.c
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * GPIO character device helper for reading chip information.
+ *
+ * Copyright (C) 2021 Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ */
+
+#include <fcntl.h>
+#include <linux/gpio.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/types.h>
+
+static void print_usage(void)
+{
+	printf("usage:\n");
+	printf("  gpio-chip-info <chip path> [name|label|num-lines]\n");
+}
+
+int main(int argc, char **argv)
+{
+	struct gpiochip_info info;
+	int fd, ret;
+
+	if (argc !=3) {
+		print_usage();
+		return EXIT_FAILURE;
+	}
+
+	fd = open(argv[1], O_RDWR);
+	if (fd < 0) {
+		perror("unable to open the GPIO chip");
+		return EXIT_FAILURE;
+	}
+
+	memset(&info, 0, sizeof(info));
+	ret = ioctl(fd, GPIO_GET_CHIPINFO_IOCTL, &info);
+	if (ret) {
+		perror("chip info ioctl failed");
+		return EXIT_FAILURE;
+	}
+
+	if (strcmp(argv[2], "name") == 0) {
+		printf("%s\n", info.name);
+	} else if (strcmp(argv[2], "label") == 0) {
+		printf("%s\n", info.label);
+	} else if (strcmp(argv[2], "num-lines") == 0) {
+		printf("%u\n", info.lines);
+	} else {
+		fprintf(stderr, "unknown command: %s\n", argv[2]);
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}
-- 
2.29.1

[PATCH v2 11/12] selftests: gpio: add a helper for reading GPIO line names

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a simple program that allows to read GPIO line names from the
character device. This will be used in gpio-sim selftests.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 tools/testing/selftests/gpio/.gitignore       |  1 +
 tools/testing/selftests/gpio/Makefile         |  2 +-
 tools/testing/selftests/gpio/gpio-line-name.c | 55 +++++++++++++++++++
 3 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/gpio/gpio-line-name.c

diff --git a/tools/testing/selftests/gpio/.gitignore b/tools/testing/selftests/gpio/.gitignore
index 4ea4f58dab1a..ededb077a3a6 100644
--- a/tools/testing/selftests/gpio/.gitignore
+++ b/tools/testing/selftests/gpio/.gitignore
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 gpio-mockup-cdev
 gpio-chip-info
+gpio-line-name
diff --git a/tools/testing/selftests/gpio/Makefile b/tools/testing/selftests/gpio/Makefile
index 84b48547f94c..d7d8f1985d99 100644
--- a/tools/testing/selftests/gpio/Makefile
+++ b/tools/testing/selftests/gpio/Makefile
@@ -2,6 +2,6 @@
 
 TEST_PROGS := gpio-mockup.sh
 TEST_FILES := gpio-mockup-sysfs.sh
-TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev gpio-chip-info
+TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev gpio-chip-info gpio-line-name
 
 include ../lib.mk
diff --git a/tools/testing/selftests/gpio/gpio-line-name.c b/tools/testing/selftests/gpio/gpio-line-name.c
new file mode 100644
index 000000000000..a52e75bc37ba
--- /dev/null
+++ b/tools/testing/selftests/gpio/gpio-line-name.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * GPIO character device helper for reading line names.
+ *
+ * Copyright (C) 2021 Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ */
+
+#include <fcntl.h>
+#include <linux/gpio.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/types.h>
+
+static void print_usage(void)
+{
+	printf("usage:\n");
+	printf("  gpio-line-name <chip path> <line offset>\n");
+}
+
+int main(int argc, char **argv)
+{
+	struct gpio_v2_line_info info;
+	int fd, ret;
+	char *endp;
+
+	if (argc != 3) {
+		print_usage();
+		return EXIT_FAILURE;
+	}
+
+	fd = open(argv[1], O_RDWR);
+	if (fd < 0) {
+		perror("unable to open the GPIO chip");
+		return EXIT_FAILURE;
+	}
+
+	memset(&info, 0, sizeof(info));
+	info.offset = strtoul(argv[2], &endp, 10);
+	if (*endp != '\0') {
+		print_usage();
+		return EXIT_FAILURE;
+	}
+
+	ret = ioctl(fd, GPIO_V2_GET_LINEINFO_IOCTL, &info);
+	if (ret) {
+		perror("line info ioctl failed");
+		return EXIT_FAILURE;
+	}
+
+	printf("%s\n", info.name);
+
+	return EXIT_SUCCESS;
+}
-- 
2.29.1

[PATCH v2 12/12] selftests: gpio: add test cases for gpio-sim

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a set of tests for the new gpio-sim module. This is a pure shell
test-suite and uses the helper programs available in the gpio selftests
directory. These test-cases only test the functionalities exposed by the
gpio-sim driver, not those handled by core gpiolib code.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 tools/testing/selftests/gpio/Makefile    |   2 +-
 tools/testing/selftests/gpio/config      |   1 +
 tools/testing/selftests/gpio/gpio-sim.sh | 229 +++++++++++++++++++++++
 3 files changed, 231 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/gpio/gpio-sim.sh

diff --git a/tools/testing/selftests/gpio/Makefile b/tools/testing/selftests/gpio/Makefile
index d7d8f1985d99..4c6df61c76a8 100644
--- a/tools/testing/selftests/gpio/Makefile
+++ b/tools/testing/selftests/gpio/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
-TEST_PROGS := gpio-mockup.sh
+TEST_PROGS := gpio-mockup.sh gpio-sim.sh
 TEST_FILES := gpio-mockup-sysfs.sh
 TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev gpio-chip-info gpio-line-name
 
diff --git a/tools/testing/selftests/gpio/config b/tools/testing/selftests/gpio/config
index ce100342c20b..409a8532facc 100644
--- a/tools/testing/selftests/gpio/config
+++ b/tools/testing/selftests/gpio/config
@@ -1,3 +1,4 @@
 CONFIG_GPIOLIB=y
 CONFIG_GPIO_CDEV=y
 CONFIG_GPIO_MOCKUP=m
+CONFIG_GPIO_SIM=m
diff --git a/tools/testing/selftests/gpio/gpio-sim.sh b/tools/testing/selftests/gpio/gpio-sim.sh
new file mode 100755
index 000000000000..9fd13ab8bec6
--- /dev/null
+++ b/tools/testing/selftests/gpio/gpio-sim.sh
@@ -0,0 +1,229 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2021 Bartosz Golaszewski <bgolaszewski@baylibre.com>
+
+BASE_DIR=`dirname $0`
+CONFIGFS_DIR="/sys/kernel/config/gpio-sim"
+PENDING_DIR=$CONFIGFS_DIR/pending
+LIVE_DIR=$CONFIGFS_DIR/live
+MODULE="gpio-sim"
+
+fail() {
+	echo "$*" >&2
+	echo "GPIO $MODULE test FAIL"
+	exit 1
+}
+
+skip() {
+	echo "$*" >&2
+	echo "GPIO $MODULE test SKIP"
+	exit 4
+}
+
+configfs_cleanup() {
+	for DIR in `ls $LIVE_DIR`; do
+		mv $LIVE_DIR/$DIR $PENDING_DIR
+	done
+
+	for DIR in `ls $PENDING_DIR`; do
+		rmdir $PENDING_DIR/$DIR
+	done
+}
+
+create_pending_chip() {
+	local NAME="$1"
+	local LABEL="$2"
+	local NUM_LINES="$3"
+	local LINE_NAMES="$4"
+	local CHIP_DIR="$PENDING_DIR/$NAME"
+
+	mkdir $CHIP_DIR
+	test -n "$LABEL" && echo $LABEL > $CHIP_DIR/label
+	test -n "$NUM_LINES" && echo $NUM_LINES > $CHIP_DIR/num_lines
+	if [ -n "$LINE_NAMES" ]; then
+		echo $LINE_NAMES 2> /dev/null > $CHIP_DIR/line_names
+		# This one can fail
+		if [ "$?" -ne "0" ]; then
+			return 1
+		fi
+	fi
+}
+
+create_live_chip() {
+	local CHIP_DIR="$PENDING_DIR/$1"
+
+	create_pending_chip "$@" || fail "unable to create the chip configfs item"
+	mv $CHIP_DIR $LIVE_DIR || fail "unable to commit the chip configfs item"
+}
+
+remove_pending_chip() {
+	local NAME="$1"
+
+	rmdir $PENDING_DIR/$NAME || fail "unable to remove the chip configfs item"
+}
+
+remove_live_chip() {
+	local NAME="$1"
+
+	mv $LIVE_DIR/$NAME $PENDING_DIR || fail "unable to uncommit the chip configfs item"
+	remove_pending_chip "$@"
+}
+
+configfs_chip_name() {
+	local CHIP="$1"
+
+	cat $LIVE_DIR/$CHIP/chip_name 2> /dev/null || return 1
+}
+
+configfs_dev_name() {
+	local CHIP="$1"
+
+	cat $LIVE_DIR/$CHIP/dev_name 2> /dev/null || return 1
+}
+
+get_chip_num_lines() {
+	local CHIP="$1"
+
+	$BASE_DIR/gpio-chip-info /dev/`configfs_chip_name $CHIP` num-lines
+}
+
+get_chip_label() {
+	local CHIP="$1"
+
+	$BASE_DIR/gpio-chip-info /dev/`configfs_chip_name $CHIP` label
+}
+
+get_line_name() {
+	local CHIP="$1"
+	local OFFSET="$2"
+
+	$BASE_DIR/gpio-line-name /dev/`configfs_chip_name $CHIP` $OFFSET
+}
+
+sysfs_set_pull() {
+	local CHIP="$1"
+	local OFFSET="$2"
+	local PULL="$3"
+	local SYSFSPATH="/sys/devices/platform/`configfs_dev_name $CHIP`/line-ctrl/gpio$OFFSET"
+
+	echo $PULL > $SYSFSPATH
+}
+
+# Load the gpio-sim module. This will pull in configfs if needed too.
+modprobe gpio-sim || skip "unable to load the gpio-sim module"
+# Make sure configfs is mounted at /sys/kernel/config. Wait a bit if needed.
+for IDX in `seq 5`; do
+	if [ "$IDX" -eq "5" ]; then
+		skip "configfs not mounted at /sys/kernel/config"
+	fi
+
+	mountpoint -q /sys/kernel/config && break
+	sleep 0.1
+done
+# If the module was already loaded: remove all previous chips
+configfs_cleanup
+
+trap "exit 1" SIGTERM SIGINT
+trap configfs_cleanup EXIT
+
+echo "1. chip_name and dev_name attributes"
+
+echo "1.1. Chip name is communicated to user"
+create_live_chip chip
+test -n `cat $LIVE_DIR/chip/chip_name` || fail "chip_name doesn't work"
+remove_live_chip chip
+
+echo "1.2. chip_name returns an error if chip is still pending"
+create_pending_chip chip
+configfs_chip_name chip && fail "chip_name doesn't return error for a pending chip"
+remove_pending_chip chip
+
+echo "1.3. Device name is communicated to user"
+create_live_chip chip
+test -n `cat $LIVE_DIR/chip/dev_name` || fail "dev_name doesn't work"
+remove_live_chip chip
+
+echo "1.4. dev_name returns an error if chip is still pending"
+create_pending_chip chip
+configfs_dev_name chip && fail "dev_name doesn't return error for a pending chip"
+remove_pending_chip chip
+
+echo "2. Creating simulated chips"
+
+echo "2.1. Default number of lines is 1"
+create_live_chip chip
+test "`get_chip_num_lines chip`" = "1" || fail "default number of lines is not 1"
+remove_live_chip chip
+
+echo "2.2. Number of lines can be specified"
+create_live_chip chip test-label 16
+test "`get_chip_num_lines chip`" = "16" || fail "number of lines is not 16"
+remove_live_chip chip
+
+echo "2.3. Label can be set"
+create_live_chip chip foobar
+test "`get_chip_label chip`" = "foobar" || fail "label is incorrect"
+remove_live_chip chip
+
+echo "2.4. Label can be left empty"
+create_live_chip chip
+test -z "`cat $LIVE_DIR/chip/label`" || fail "label is not empty"
+remove_live_chip chip
+
+echo "2.5. Line names can be configured"
+create_live_chip chip test-label 16 '"foo", "", "bar"'
+test "`get_line_name chip 0`" = "foo" || fail "line name is incorrect"
+test "`get_line_name chip 2`" = "bar" || fail "line name is incorrect"
+remove_live_chip chip
+
+echo "2.6. Errors in line names are detected"
+create_pending_chip chip test-label 8 '"foo", bar' && fail "incorrect line name accepted"
+remove_pending_chip chip
+create_pending_chip chip test-label 8 '"foo" "bar"' && fail "incorrect line name accepted"
+remove_pending_chip chip
+
+echo "2.7. Multiple chips can be created"
+create_live_chip chip0
+create_live_chip chip1
+create_live_chip chip2
+remove_live_chip chip0
+remove_live_chip chip1
+remove_live_chip chip2
+
+echo "3. Controlling simulated chips"
+
+echo "3.3. Pull can be set over sysfs"
+create_live_chip chip test-label 8
+sysfs_set_pull chip 0 1
+$BASE_DIR/gpio-mockup-cdev /dev/`configfs_chip_name chip` 0
+test "$?" = "1" || fail "pull set incorrectly"
+sysfs_set_pull chip 0 0
+$BASE_DIR/gpio-mockup-cdev /dev/`configfs_chip_name chip` 1
+test "$?" = "0" || fail "pull set incorrectly"
+remove_live_chip chip
+
+echo "3.4. Incorrect input in sysfs is rejected"
+create_live_chip chip test-label 8
+SYSFS_PATH="/sys/devices/platform/`configfs_dev_name chip`/line-ctrl/gpio0"
+echo 2 > $SYSFS_PATH 2> /dev/null && fail "invalid input not detectec"
+remove_live_chip chip
+
+echo "4. Simulated GPIO chips are functional"
+
+echo "4.1. Values can be read from sysfs"
+create_live_chip chip test-label 8
+SYSFS_PATH="/sys/devices/platform/`configfs_dev_name chip`/line-ctrl/gpio0"
+test `cat $SYSFS_PATH` = "0" || fail "incorrect value read from sysfs"
+$BASE_DIR/gpio-mockup-cdev -s 1 /dev/`configfs_chip_name chip` 0 &
+sleep 0.1 # FIXME Any better way?
+test `cat $SYSFS_PATH` = "1" || fail "incorrect value read from sysfs"
+kill $!
+remove_live_chip chip
+
+echo "4.2. Bias settings work correctly"
+create_live_chip chip test-label 8
+$BASE_DIR/gpio-mockup-cdev -b pull-up /dev/`configfs_chip_name chip` 0
+test `cat $SYSFS_PATH` = "1" || fail "bias setting does not work"
+remove_live_chip chip
+
+echo "GPIO $MODULE test PASS"
-- 
2.29.1