Hi all,
Guenter and Oliver have been pointing out a few limitations of the
driver core's ability to create files properly (i.e. in a way that
doesn't race with userspace.) The driver core allows this, but it
doesn't export that ability to drivers very easily, and for binary
files, not at all.
So here's a set of 6 patches that I'll be queueing up to go to Linus in
time for 3.11 so that people can start using them in their driver
subsystems. It adds some new macros to make using attributes and
attribute groups easier, adds binary file capabilities to attribute
groups, and finally, lets subsystems (like platform drivers) set a
attribute group for when their device is created.
If anyone has any problems with these patches, please let me know.
Guenter, I've tweaked your original patch a bit, changing the name of
the function and putting the kernel doc comments in the correct place so
the build doesn't complain about it.
I also have a set of follow-on patches, about 50+ big so far, that goes
through the kernel and converts different drivers and subsystems to
properly use attribute groups, instead of open-coding binary files and
attributes. Those patches will be sent out later, and will be for 3.12
as they aren't needed at the moment, this infrastructure changes are
needed first.
thanks,
greg k-h
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
include/linux/sysfs.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9cd20c8..f62ff01 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -94,6 +94,15 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
+#define ATTRIBUTE_GROUPS(name) \
+static const struct attribute_group name##_group = { \
+ .attrs = name##_attrs, \
+}; \
+static const struct attribute_group *name##_groups[] = { \
+ &name##_group, \
+ NULL, \
+}
+
#define attr_name(_attr) (_attr).attr.name
struct file;
--
1.8.3.rc0.20.gb99dd2e
groups should be able to support binary attributes, just like it
supports "normal" attributes. This lets us only handle one type of
structure, groups, throughout the driver core and subsystems, making
binary attributes a "full fledged" part of the driver model, and not
something just "tacked on".
Reported-by: Oliver Schinagl <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
fs/sysfs/group.c | 18 ++++++++++++++++--
include/linux/sysfs.h | 4 ++--
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aec3d5c..d8d8a8f 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -20,16 +20,19 @@ static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
- int i;
+ struct bin_attribute *const* bin_attr;
- for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+ for (attr = grp->attrs; *attr; attr++)
sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+ for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
+ sysfs_remove_bin_file(kobj, *bin_attr);
}
static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp, int update)
{
struct attribute *const* attr;
+ struct bin_attribute *const* bin_attr;
int error = 0, i;
for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
@@ -52,6 +55,17 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
}
if (error)
remove_files(dir_sd, kobj, grp);
+
+ for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
+ if (update)
+ sysfs_remove_bin_file(kobj, *bin_attr);
+ error = sysfs_create_bin_file(kobj, *bin_attr);
+ if (error)
+ break;
+ }
+ if (error)
+ remove_files(dir_sd, kobj, grp);
+
return error;
}
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index d50a96b..2c3b6a3 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -21,6 +21,7 @@
struct kobject;
struct module;
+struct bin_attribute;
enum kobj_ns_type;
struct attribute {
@@ -59,10 +60,9 @@ struct attribute_group {
umode_t (*is_visible)(struct kobject *,
struct attribute *, int);
struct attribute **attrs;
+ struct bin_attribute **bin_attrs;
};
-
-
/**
* Use these macros to make defining attributes easier. See include/linux/device.h
* for examples..
--
1.8.3.rc0.20.gb99dd2e
From: Guenter Roeck <[email protected]>
From: Guenter Roeck <[email protected]>
device_create_groups lets callers create devices as well as associated
sysfs attributes with a single call. This avoids race conditions seen
if sysfs attributes on new devices are created later.
[fixed up comment block placement and add checks for printk buffer
formats - gregkh]
Signed-off-by: Guenter Roeck <[email protected]>
Cc: Jean Delvare <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/base/core.c | 111 ++++++++++++++++++++++++++++++++++++-------------
include/linux/device.h | 5 +++
2 files changed, 88 insertions(+), 28 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index dc3ea23..a8aae18 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1667,34 +1667,11 @@ static void device_create_release(struct device *dev)
kfree(dev);
}
-/**
- * device_create_vargs - creates a device and registers it with sysfs
- * @class: pointer to the struct class that this device should be registered to
- * @parent: pointer to the parent struct device of this new device, if any
- * @devt: the dev_t for the char device to be added
- * @drvdata: the data to be added to the device for callbacks
- * @fmt: string for the device's name
- * @args: va_list for the device's name
- *
- * This function can be used by char device classes. A struct device
- * will be created in sysfs, registered to the specified class.
- *
- * A "dev" file will be created, showing the dev_t for the device, if
- * the dev_t is not 0,0.
- * If a pointer to a parent struct device is passed in, the newly created
- * struct device will be a child of that device in sysfs.
- * The pointer to the struct device will be returned from the call.
- * Any further sysfs files that might be required can be created using this
- * pointer.
- *
- * Returns &struct device pointer on success, or ERR_PTR() on error.
- *
- * Note: the struct class passed to this function must have previously
- * been created with a call to class_create().
- */
-struct device *device_create_vargs(struct class *class, struct device *parent,
- dev_t devt, void *drvdata, const char *fmt,
- va_list args)
+static struct device *
+device_create_groups_vargs(struct class *class, struct device *parent,
+ dev_t devt, void *drvdata,
+ const struct attribute_group **groups,
+ const char *fmt, va_list args)
{
struct device *dev = NULL;
int retval = -ENODEV;
@@ -1711,6 +1688,7 @@ struct device *device_create_vargs(struct class *class, struct device *parent,
dev->devt = devt;
dev->class = class;
dev->parent = parent;
+ dev->groups = groups;
dev->release = device_create_release;
dev_set_drvdata(dev, drvdata);
@@ -1728,6 +1706,39 @@ error:
put_device(dev);
return ERR_PTR(retval);
}
+
+/**
+ * device_create_vargs - creates a device and registers it with sysfs
+ * @class: pointer to the struct class that this device should be registered to
+ * @parent: pointer to the parent struct device of this new device, if any
+ * @devt: the dev_t for the char device to be added
+ * @drvdata: the data to be added to the device for callbacks
+ * @fmt: string for the device's name
+ * @args: va_list for the device's name
+ *
+ * This function can be used by char device classes. A struct device
+ * will be created in sysfs, registered to the specified class.
+ *
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct device is passed in, the newly created
+ * struct device will be a child of that device in sysfs.
+ * The pointer to the struct device will be returned from the call.
+ * Any further sysfs files that might be required can be created using this
+ * pointer.
+ *
+ * Returns &struct device pointer on success, or ERR_PTR() on error.
+ *
+ * Note: the struct class passed to this function must have previously
+ * been created with a call to class_create().
+ */
+struct device *device_create_vargs(struct class *class, struct device *parent,
+ dev_t devt, void *drvdata, const char *fmt,
+ va_list args)
+{
+ return device_create_groups_vargs(class, parent, devt, drvdata, NULL,
+ fmt, args);
+}
EXPORT_SYMBOL_GPL(device_create_vargs);
/**
@@ -1767,6 +1778,50 @@ struct device *device_create(struct class *class, struct device *parent,
}
EXPORT_SYMBOL_GPL(device_create);
+/**
+ * device_create_with_groups - creates a device and registers it with sysfs
+ * @class: pointer to the struct class that this device should be registered to
+ * @parent: pointer to the parent struct device of this new device, if any
+ * @devt: the dev_t for the char device to be added
+ * @drvdata: the data to be added to the device for callbacks
+ * @groups: NULL-terminated list of attribute groups to be created
+ * @fmt: string for the device's name
+ *
+ * This function can be used by char device classes. A struct device
+ * will be created in sysfs, registered to the specified class.
+ * Additional attributes specified in the groups parameter will also
+ * be created automatically.
+ *
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct device is passed in, the newly created
+ * struct device will be a child of that device in sysfs.
+ * The pointer to the struct device will be returned from the call.
+ * Any further sysfs files that might be required can be created using this
+ * pointer.
+ *
+ * Returns &struct device pointer on success, or ERR_PTR() on error.
+ *
+ * Note: the struct class passed to this function must have previously
+ * been created with a call to class_create().
+ */
+struct device *device_create_with_groups(struct class *class,
+ struct device *parent, dev_t devt,
+ void *drvdata,
+ const struct attribute_group **groups,
+ const char *fmt, ...)
+{
+ va_list vargs;
+ struct device *dev;
+
+ va_start(vargs, fmt);
+ dev = device_create_groups_vargs(class, parent, devt, drvdata, groups,
+ fmt, vargs);
+ va_end(vargs);
+ return dev;
+}
+EXPORT_SYMBOL_GPL(device_create_with_groups);
+
static int __match_devt(struct device *dev, const void *data)
{
const dev_t *devt = data;
diff --git a/include/linux/device.h b/include/linux/device.h
index 1d546a0..dce20e4 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -928,6 +928,11 @@ extern __printf(5, 6)
struct device *device_create(struct class *cls, struct device *parent,
dev_t devt, void *drvdata,
const char *fmt, ...);
+extern __printf(6, 7)
+struct device *device_create_with_groups(struct class *cls,
+ struct device *parent, dev_t devt, void *drvdata,
+ const struct attribute_group **groups,
+ const char *fmt, ...);
extern void device_destroy(struct class *cls, dev_t devt);
/*
--
1.8.3.rc0.20.gb99dd2e
Make it easier to create attributes without having to always audit the
mode settings.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
include/linux/device.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index bcf8c0d..1d546a0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -512,6 +512,10 @@ ssize_t device_store_bool(struct device *dev, struct device_attribute *attr,
#define DEVICE_ATTR(_name, _mode, _show, _store) \
struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define DEVICE_ATTR_RW(_name) \
+ struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
+#define DEVICE_ATTR_RO(_name) \
+ struct device_attribute dev_attr_##_name = __ATTR_RO(_name)
#define DEVICE_ULONG_ATTR(_name, _mode, _var) \
struct dev_ext_attribute dev_attr_##_name = \
{ __ATTR(_name, _mode, device_show_ulong, device_store_ulong), &(_var) }
--
1.8.3.rc0.20.gb99dd2e
This makes it easier to create static binary attributes, which is needed
in a number of drivers, instead of "open coding" them.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
include/linux/sysfs.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index f62ff01..d50a96b 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -132,6 +132,15 @@ struct bin_attribute {
*/
#define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&(bin_attr)->attr)
+/* macro to create static binary attributes easier */
+#define BIN_ATTR(_name, _mode, _read, _write, _size) \
+struct bin_attribute bin_attr_##_name = { \
+ .attr = {.name = __stringify(_name), .mode = _mode }, \
+ .read = _read, \
+ .write = _write, \
+ .size = _size, \
+}
+
struct sysfs_ops {
ssize_t (*show)(struct kobject *, struct attribute *,char *);
ssize_t (*store)(struct kobject *,struct attribute *,const char *, size_t);
--
1.8.3.rc0.20.gb99dd2e
A number of parts of the kernel created their own version of this, might
as well have the sysfs core provide it instead.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
include/linux/sysfs.h | 2 ++
kernel/events/core.c | 2 --
mm/backing-dev.c | 2 --
3 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index e2cee22..9cd20c8 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -79,6 +79,8 @@ struct attribute_group {
.show = _name##_show, \
}
+#define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
+
#define __ATTR_NULL { .attr = { .name = NULL } }
#ifdef CONFIG_DEBUG_LOCK_ALLOC
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1db3af9..45a21ae 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6212,8 +6212,6 @@ perf_event_mux_interval_ms_store(struct device *dev,
return count;
}
-#define __ATTR_RW(attr) __ATTR(attr, 0644, attr##_show, attr##_store)
-
static struct device_attribute pmu_dev_attrs[] = {
__ATTR_RO(type),
__ATTR_RW(perf_event_mux_interval_ms),
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index d014ee5..e04454c 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -232,8 +232,6 @@ static ssize_t stable_pages_required_show(struct device *dev,
bdi_cap_stable_pages_required(bdi) ? 1 : 0);
}
-#define __ATTR_RW(attr) __ATTR(attr, 0644, attr##_show, attr##_store)
-
static struct device_attribute bdi_dev_attrs[] = {
__ATTR_RW(read_ahead_kb),
__ATTR_RW(min_ratio),
--
1.8.3.rc0.20.gb99dd2e
On 07/10/13 22:05, Greg Kroah-Hartman wrote:
> Hi all,
Hey Greg,
>
> Guenter and Oliver have been pointing out a few limitations of the
> driver core's ability to create files properly (i.e. in a way that
> doesn't race with userspace.) The driver core allows this, but it
> doesn't export that ability to drivers very easily, and for binary
> files, not at all.
Exactly, even the patch you supplied didn't help, as it created the
binary attributes for devices, not device_drivers or platform_drivers.
>
> So here's a set of 6 patches that I'll be queueing up to go to Linus in
> time for 3.11 so that people can start using them in their driver
> subsystems. It adds some new macros to make using attributes and
> attribute groups easier, adds binary file capabilities to attribute
> groups, and finally, lets subsystems (like platform drivers) set a
> attribute group for when their device is created.
Oh, that does some like iceing on the cake.
>
> If anyone has any problems with these patches, please let me know.
>
I will try this patch set immediately and report back.
> Guenter, I've tweaked your original patch a bit, changing the name of
> the function and putting the kernel doc comments in the correct place so
> the build doesn't complain about it.
>
> I also have a set of follow-on patches, about 50+ big so far, that goes
> through the kernel and converts different drivers and subsystems to
> properly use attribute groups, instead of open-coding binary files and
> attributes. Those patches will be sent out later, and will be for 3.12
> as they aren't needed at the moment, this infrastructure changes are
> needed first.
>
> thanks,
Thank you Greg, this is far more robust that what I cooked up (adding
bin_attrs to struct device_driver. I was just going to send it to the
list as well when I saw this message :)
>
> greg k-h
>
Oliver
On Wed, Jul 10, 2013 at 01:05:08PM -0700, Greg Kroah-Hartman wrote:
> Hi all,
>
> Guenter and Oliver have been pointing out a few limitations of the
> driver core's ability to create files properly (i.e. in a way that
> doesn't race with userspace.) The driver core allows this, but it
> doesn't export that ability to drivers very easily, and for binary
> files, not at all.
>
> So here's a set of 6 patches that I'll be queueing up to go to Linus in
> time for 3.11 so that people can start using them in their driver
> subsystems. It adds some new macros to make using attributes and
> attribute groups easier, adds binary file capabilities to attribute
> groups, and finally, lets subsystems (like platform drivers) set a
> attribute group for when their device is created.
>
> If anyone has any problems with these patches, please let me know.
>
> Guenter, I've tweaked your original patch a bit, changing the name of
> the function and putting the kernel doc comments in the correct place so
> the build doesn't complain about it.
>
I like it - thanks a lot for picking that up. I think there are several
drivers who could and should use the new API call (gpio, infiniband/umad,
c2port, ptp, scsi/osst, android/timed_output, asus_oled are some examples).
For hwmon, I ended up having to handle device creation internally after all.
Reason is that I also had to set device->type to a new hwmon device type.
That in turn was necessary to be able to support the hwmon 'name' attribute
properly (it has to be optional, meaning I could not use class->dev_attrs but
had to use device_type->groups instead). I hope I can send out a revised
series of patches soon .. only I'll leave for pto next week and I am bugged
down with other work right now, so it may have to wait until after I return.
Wonder if it would make sense to support a core driver API call for that
purpose as well - ie one that not only has the additional groups argument,
but one that (also ?) accepts a pointer to the the device type. I didn't spend
any time tracking down potential users, though, so I may be off track here.
> I also have a set of follow-on patches, about 50+ big so far, that goes
> through the kernel and converts different drivers and subsystems to
> properly use attribute groups, instead of open-coding binary files and
> attributes. Those patches will be sent out later, and will be for 3.12
> as they aren't needed at the moment, this infrastructure changes are
> needed first.
>
Wow ... you really got into cleanup mode!
Thanks,
Guenter
We should be using groups, not attribute lists, for classes to allow
subdirectories, and soon, binary files. Groups are just more flexible
overall, so add them.
The dev_attrs list will go away after all in-kernel users are converted
to use dev_groups.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
Forgot this one in the series, it adds support to classes for groups, so
that we can handle binary attributes properly for them as well.
drivers/base/core.c | 8 +++++++-
include/linux/device.h | 4 +++-
2 files changed, 10 insertions(+), 2 deletions(-)
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -528,9 +528,12 @@ static int device_add_attrs(struct devic
int error;
if (class) {
- error = device_add_attributes(dev, class->dev_attrs);
+ error = device_add_groups(dev, class->dev_groups);
if (error)
return error;
+ error = device_add_attributes(dev, class->dev_attrs);
+ if (error)
+ goto err_remove_class_groups;
error = device_add_bin_attributes(dev, class->dev_bin_attrs);
if (error)
goto err_remove_class_attrs;
@@ -563,6 +566,9 @@ static int device_add_attrs(struct devic
err_remove_class_attrs:
if (class)
device_remove_attributes(dev, class->dev_attrs);
+ err_remove_class_groups:
+ if (class)
+ device_remove_groups(dev, class->dev_groups);
return error;
}
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -313,6 +313,7 @@ int subsys_virtual_register(struct bus_t
* @name: Name of the class.
* @owner: The module owner.
* @class_attrs: Default attributes of this class.
+ * @dev_groups: Default attributes of the devices that belong to the class.
* @dev_attrs: Default attributes of the devices belong to the class.
* @dev_bin_attrs: Default binary attributes of the devices belong to the class.
* @dev_kobj: The kobject that represents this class and links it into the hierarchy.
@@ -342,7 +343,8 @@ struct class {
struct module *owner;
struct class_attribute *class_attrs;
- struct device_attribute *dev_attrs;
+ struct device_attribute *dev_attrs; /* use dev_groups instead */
+ const struct attribute_group **dev_groups;
struct bin_attribute *dev_bin_attrs;
struct kobject *dev_kobj;
On Wed, Jul 10, 2013 at 03:05:31PM -0700, Greg Kroah-Hartman wrote:
>
> We should be using groups, not attribute lists, for classes to allow
> subdirectories, and soon, binary files. Groups are just more flexible
> overall, so add them.
>
> The dev_attrs list will go away after all in-kernel users are converted
> to use dev_groups.
>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
Nice one ... that solves my problem.
Thanks!
Guenter
> ---
> Forgot this one in the series, it adds support to classes for groups, so
> that we can handle binary attributes properly for them as well.
>
> drivers/base/core.c | 8 +++++++-
> include/linux/device.h | 4 +++-
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -528,9 +528,12 @@ static int device_add_attrs(struct devic
> int error;
>
> if (class) {
> - error = device_add_attributes(dev, class->dev_attrs);
> + error = device_add_groups(dev, class->dev_groups);
> if (error)
> return error;
> + error = device_add_attributes(dev, class->dev_attrs);
> + if (error)
> + goto err_remove_class_groups;
> error = device_add_bin_attributes(dev, class->dev_bin_attrs);
> if (error)
> goto err_remove_class_attrs;
> @@ -563,6 +566,9 @@ static int device_add_attrs(struct devic
> err_remove_class_attrs:
> if (class)
> device_remove_attributes(dev, class->dev_attrs);
> + err_remove_class_groups:
> + if (class)
> + device_remove_groups(dev, class->dev_groups);
>
> return error;
> }
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -313,6 +313,7 @@ int subsys_virtual_register(struct bus_t
> * @name: Name of the class.
> * @owner: The module owner.
> * @class_attrs: Default attributes of this class.
> + * @dev_groups: Default attributes of the devices that belong to the class.
> * @dev_attrs: Default attributes of the devices belong to the class.
> * @dev_bin_attrs: Default binary attributes of the devices belong to the class.
> * @dev_kobj: The kobject that represents this class and links it into the hierarchy.
> @@ -342,7 +343,8 @@ struct class {
> struct module *owner;
>
> struct class_attribute *class_attrs;
> - struct device_attribute *dev_attrs;
> + struct device_attribute *dev_attrs; /* use dev_groups instead */
> + const struct attribute_group **dev_groups;
> struct bin_attribute *dev_bin_attrs;
> struct kobject *dev_kobj;
>
>
We should be using groups, not attribute lists, for classes to allow
subdirectories, and soon, binary files. Groups are just more flexible
overall, so add them.
The dev_attrs list will go away after all in-kernel users are converted
to use dev_groups.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
v2: forgot to remove groups from classes when the device is removed, it
still worked fine, but it's nice to clean up things properly.
drivers/base/core.c | 9 ++++++++-
include/linux/device.h | 4 +++-
2 files changed, 11 insertions(+), 2 deletions(-)
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -528,9 +528,12 @@ static int device_add_attrs(struct devic
int error;
if (class) {
- error = device_add_attributes(dev, class->dev_attrs);
+ error = device_add_groups(dev, class->dev_groups);
if (error)
return error;
+ error = device_add_attributes(dev, class->dev_attrs);
+ if (error)
+ goto err_remove_class_groups;
error = device_add_bin_attributes(dev, class->dev_bin_attrs);
if (error)
goto err_remove_class_attrs;
@@ -563,6 +566,9 @@ static int device_add_attrs(struct devic
err_remove_class_attrs:
if (class)
device_remove_attributes(dev, class->dev_attrs);
+ err_remove_class_groups:
+ if (class)
+ device_remove_groups(dev, class->dev_groups);
return error;
}
@@ -581,6 +587,7 @@ static void device_remove_attrs(struct d
if (class) {
device_remove_attributes(dev, class->dev_attrs);
device_remove_bin_attributes(dev, class->dev_bin_attrs);
+ device_remove_groups(dev, class->dev_groups);
}
}
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -313,6 +313,7 @@ int subsys_virtual_register(struct bus_t
* @name: Name of the class.
* @owner: The module owner.
* @class_attrs: Default attributes of this class.
+ * @dev_groups: Default attributes of the devices that belong to the class.
* @dev_attrs: Default attributes of the devices belong to the class.
* @dev_bin_attrs: Default binary attributes of the devices belong to the class.
* @dev_kobj: The kobject that represents this class and links it into the hierarchy.
@@ -342,7 +343,8 @@ struct class {
struct module *owner;
struct class_attribute *class_attrs;
- struct device_attribute *dev_attrs;
+ struct device_attribute *dev_attrs; /* use dev_groups instead */
+ const struct attribute_group **dev_groups;
struct bin_attribute *dev_bin_attrs;
struct kobject *dev_kobj;
On Wed, Jul 10, 2013 at 03:04:41PM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 01:05:08PM -0700, Greg Kroah-Hartman wrote:
> > Hi all,
> >
> > Guenter and Oliver have been pointing out a few limitations of the
> > driver core's ability to create files properly (i.e. in a way that
> > doesn't race with userspace.) The driver core allows this, but it
> > doesn't export that ability to drivers very easily, and for binary
> > files, not at all.
> >
> > So here's a set of 6 patches that I'll be queueing up to go to Linus in
> > time for 3.11 so that people can start using them in their driver
> > subsystems. It adds some new macros to make using attributes and
> > attribute groups easier, adds binary file capabilities to attribute
> > groups, and finally, lets subsystems (like platform drivers) set a
> > attribute group for when their device is created.
> >
> > If anyone has any problems with these patches, please let me know.
> >
> > Guenter, I've tweaked your original patch a bit, changing the name of
> > the function and putting the kernel doc comments in the correct place so
> > the build doesn't complain about it.
> >
> I like it - thanks a lot for picking that up. I think there are several
> drivers who could and should use the new API call (gpio, infiniband/umad,
> c2port, ptp, scsi/osst, android/timed_output, asus_oled are some examples).
Yes, I'll be sweeping the tree for the next few released doing all of
this. The amount of sysfs "cruft" that has accumulated in the past few
years since I last looked is crazy...
> For hwmon, I ended up having to handle device creation internally after all.
> Reason is that I also had to set device->type to a new hwmon device type.
> That in turn was necessary to be able to support the hwmon 'name' attribute
> properly (it has to be optional, meaning I could not use class->dev_attrs but
> had to use device_type->groups instead). I hope I can send out a revised
> series of patches soon .. only I'll leave for pto next week and I am bugged
> down with other work right now, so it may have to wait until after I return.
>
> Wonder if it would make sense to support a core driver API call for that
> purpose as well - ie one that not only has the additional groups argument,
> but one that (also ?) accepts a pointer to the the device type. I didn't spend
> any time tracking down potential users, though, so I may be off track here.
I don't understand the problem, for USB we have different "types" of
devices on the bus just fine. Or are you saying that the function you
created isn't going to be used in the hwmon core in the end?
I doubt device_create() really needs to worry about different types, but
I could be wrong :)
thanks,
greg k-h
On Wed, Jul 10, 2013 at 03:18:12PM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 03:05:31PM -0700, Greg Kroah-Hartman wrote:
> >
> > We should be using groups, not attribute lists, for classes to allow
> > subdirectories, and soon, binary files. Groups are just more flexible
> > overall, so add them.
> >
> > The dev_attrs list will go away after all in-kernel users are converted
> > to use dev_groups.
> >
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> Nice one ... that solves my problem.
Thanks, I'll work on 'struct bus_type' next, moving that to only use
groups, but that's not as important from what I can tell, busses don't
seem to abuse sysfs as bad as drivers and devices do :)
thanks,
greg k-h
On Wed, Jul 10, 2013 at 03:23:37PM -0700, Greg Kroah-Hartman wrote:
> On Wed, Jul 10, 2013 at 03:04:41PM -0700, Guenter Roeck wrote:
> > On Wed, Jul 10, 2013 at 01:05:08PM -0700, Greg Kroah-Hartman wrote:
> > > Hi all,
> > >
> > > Guenter and Oliver have been pointing out a few limitations of the
> > > driver core's ability to create files properly (i.e. in a way that
> > > doesn't race with userspace.) The driver core allows this, but it
> > > doesn't export that ability to drivers very easily, and for binary
> > > files, not at all.
> > >
> > > So here's a set of 6 patches that I'll be queueing up to go to Linus in
> > > time for 3.11 so that people can start using them in their driver
> > > subsystems. It adds some new macros to make using attributes and
> > > attribute groups easier, adds binary file capabilities to attribute
> > > groups, and finally, lets subsystems (like platform drivers) set a
> > > attribute group for when their device is created.
> > >
> > > If anyone has any problems with these patches, please let me know.
> > >
> > > Guenter, I've tweaked your original patch a bit, changing the name of
> > > the function and putting the kernel doc comments in the correct place so
> > > the build doesn't complain about it.
> > >
> > I like it - thanks a lot for picking that up. I think there are several
> > drivers who could and should use the new API call (gpio, infiniband/umad,
> > c2port, ptp, scsi/osst, android/timed_output, asus_oled are some examples).
>
> Yes, I'll be sweeping the tree for the next few released doing all of
> this. The amount of sysfs "cruft" that has accumulated in the past few
> years since I last looked is crazy...
>
> > For hwmon, I ended up having to handle device creation internally after all.
> > Reason is that I also had to set device->type to a new hwmon device type.
> > That in turn was necessary to be able to support the hwmon 'name' attribute
> > properly (it has to be optional, meaning I could not use class->dev_attrs but
> > had to use device_type->groups instead). I hope I can send out a revised
> > series of patches soon .. only I'll leave for pto next week and I am bugged
> > down with other work right now, so it may have to wait until after I return.
> >
> > Wonder if it would make sense to support a core driver API call for that
> > purpose as well - ie one that not only has the additional groups argument,
> > but one that (also ?) accepts a pointer to the the device type. I didn't spend
> > any time tracking down potential users, though, so I may be off track here.
>
> I don't understand the problem, for USB we have different "types" of
> devices on the bus just fine. Or are you saying that the function you
> created isn't going to be used in the hwmon core in the end?
>
> I doubt device_create() really needs to worry about different types, but
> I could be wrong :)
>
All I needed was the dev_groups field you just added to the class structure.
I am currently merging your patches into my code, so you'll get some free test
coverage ...
Thanks,
Guenter
On Wed, Jul 10, 2013 at 01:05:10PM -0700, Greg Kroah-Hartman wrote:
> To make it easier for driver subsystems to work with attribute groups,
> create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
> typing for the most common use for attribute groups.
>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> include/linux/sysfs.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 9cd20c8..f62ff01 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -94,6 +94,15 @@ struct attribute_group {
> #define __ATTR_IGNORE_LOCKDEP __ATTR
> #endif
>
> +#define ATTRIBUTE_GROUPS(name) \
Would it be possible to add is_visible as additional argument ?
Thanks,
Guenter
> +static const struct attribute_group name##_group = { \
> + .attrs = name##_attrs, \
> +}; \
> +static const struct attribute_group *name##_groups[] = { \
> + &name##_group, \
> + NULL, \
> +}
> +
> #define attr_name(_attr) (_attr).attr.name
>
> struct file;
> --
> 1.8.3.rc0.20.gb99dd2e
>
>
On Wed, Jul 10, 2013 at 03:48:56PM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 01:05:10PM -0700, Greg Kroah-Hartman wrote:
> > To make it easier for driver subsystems to work with attribute groups,
> > create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
> > typing for the most common use for attribute groups.
> >
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > include/linux/sysfs.h | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index 9cd20c8..f62ff01 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -94,6 +94,15 @@ struct attribute_group {
> > #define __ATTR_IGNORE_LOCKDEP __ATTR
> > #endif
> >
> > +#define ATTRIBUTE_GROUPS(name) \
>
> Would it be possible to add is_visible as additional argument ?
I thought about the other options (multiple groups, is_visible, and
bin_attrs), but those are almost all not the "normal" usage, so I figure
it's worth the extra 6 lines of text if you want to support is_visible
or binary attributes.
After converting all of the class code to use groups, it seems to match
up with the defaults, but if in the future it's more popular than
expected, I'll gladly add it.
thanks,
greg k-h
On Wed, Jul 10, 2013 at 01:05:13PM -0700, Greg Kroah-Hartman wrote:
> groups should be able to support binary attributes, just like it
> supports "normal" attributes. This lets us only handle one type of
> structure, groups, throughout the driver core and subsystems, making
> binary attributes a "full fledged" part of the driver model, and not
> something just "tacked on".
>
> Reported-by: Oliver Schinagl <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> fs/sysfs/group.c | 18 ++++++++++++++++--
> include/linux/sysfs.h | 4 ++--
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index aec3d5c..d8d8a8f 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -20,16 +20,19 @@ static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> const struct attribute_group *grp)
> {
> struct attribute *const* attr;
> - int i;
> + struct bin_attribute *const* bin_attr;
checkpatch doesn't like the above and similar declarations below.
Yes, I know, nitpick, just saying ;).
>
> - for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> + for (attr = grp->attrs; *attr; attr++)
> sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
> + for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
> + sysfs_remove_bin_file(kobj, *bin_attr);
> }
>
> static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> const struct attribute_group *grp, int update)
> {
> struct attribute *const* attr;
> + struct bin_attribute *const* bin_attr;
> int error = 0, i;
>
> for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> @@ -52,6 +55,17 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> }
> if (error)
> remove_files(dir_sd, kobj, grp);
You might want to abort here if there was an error.
> +
> + for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
What happens here if grp->bin_attrs is NULL ? Doesn't that cause a NULL
reference when accessing *bin_attr ?
[ The problem didn't exist before since attr was supposed to be non-NULL.
It may now be a problem if only bin_attr is provided, ie if there are
only binary attribute files.
Or maybe I am just tired and there is no problem... ]
> + if (update)
> + sysfs_remove_bin_file(kobj, *bin_attr);
> + error = sysfs_create_bin_file(kobj, *bin_attr);
> + if (error)
> + break;
> + }
> + if (error)
> + remove_files(dir_sd, kobj, grp);
> +
> return error;
> }
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index d50a96b..2c3b6a3 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -21,6 +21,7 @@
>
> struct kobject;
> struct module;
> +struct bin_attribute;
> enum kobj_ns_type;
>
> struct attribute {
> @@ -59,10 +60,9 @@ struct attribute_group {
> umode_t (*is_visible)(struct kobject *,
> struct attribute *, int);
> struct attribute **attrs;
> + struct bin_attribute **bin_attrs;
> };
>
> -
> -
> /**
> * Use these macros to make defining attributes easier. See include/linux/device.h
> * for examples..
> --
> 1.8.3.rc0.20.gb99dd2e
>
>
On Wed, Jul 10, 2013 at 03:57:30PM -0700, Greg Kroah-Hartman wrote:
> On Wed, Jul 10, 2013 at 03:48:56PM -0700, Guenter Roeck wrote:
> > On Wed, Jul 10, 2013 at 01:05:10PM -0700, Greg Kroah-Hartman wrote:
> > > To make it easier for driver subsystems to work with attribute groups,
> > > create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
> > > typing for the most common use for attribute groups.
> > >
> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > > ---
> > > include/linux/sysfs.h | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > > index 9cd20c8..f62ff01 100644
> > > --- a/include/linux/sysfs.h
> > > +++ b/include/linux/sysfs.h
> > > @@ -94,6 +94,15 @@ struct attribute_group {
> > > #define __ATTR_IGNORE_LOCKDEP __ATTR
> > > #endif
> > >
> > > +#define ATTRIBUTE_GROUPS(name) \
> >
> > Would it be possible to add is_visible as additional argument ?
>
> I thought about the other options (multiple groups, is_visible, and
> bin_attrs), but those are almost all not the "normal" usage, so I figure
> it's worth the extra 6 lines of text if you want to support is_visible
> or binary attributes.
>
Ok, fair enough.
Thanks,
Guenter
On Wed, Jul 10, 2013 at 04:00:57PM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 01:05:13PM -0700, Greg Kroah-Hartman wrote:
> > @@ -52,6 +55,17 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> > }
> > if (error)
> > remove_files(dir_sd, kobj, grp);
>
> You might want to abort here if there was an error.
Good point, I'll update it.
> > +
> > + for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
>
> What happens here if grp->bin_attrs is NULL ? Doesn't that cause a NULL
> reference when accessing *bin_attr ?
>
> [ The problem didn't exist before since attr was supposed to be non-NULL.
> It may now be a problem if only bin_attr is provided, ie if there are
> only binary attribute files.
> Or maybe I am just tired and there is no problem... ]
I thought I tested this patch out, but I think you are right, let me go
do a rebuild and test it again...
thanks,
greg k-h
On Wed, Jul 10, 2013 at 04:59:28PM -0700, Greg Kroah-Hartman wrote:
> On Wed, Jul 10, 2013 at 04:00:57PM -0700, Guenter Roeck wrote:
> > > + for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> >
> > What happens here if grp->bin_attrs is NULL ? Doesn't that cause a NULL
> > reference when accessing *bin_attr ?
> >
> > [ The problem didn't exist before since attr was supposed to be non-NULL.
> > It may now be a problem if only bin_attr is provided, ie if there are
> > only binary attribute files.
> > Or maybe I am just tired and there is no problem... ]
>
> I thought I tested this patch out, but I think you are right, let me go
> do a rebuild and test it again...
You were right, good catch, as-is, the kernel dies at boot, I've fixed
this and will resend the series, thanks so much for the review.
greg k-h
Hi all,
Here's the second iteration of the patchset to add better attribute
group support to the driver core and sysfs.
I've tested these (shocker!) and everything works fine with them (I'm
sending this from Linus's latest kernel with these 7 on top of it.)
I'd like to send these to Linus for 3.11 unless someone objects.
Oliver, please use this series instead of the last one, it has fixes
that Guenter pointed out that would have crashed your box at boot.
changes from v2:
- actually boots
- 7th patch added properly
- added BUS_ATTR, CLASS_ATTR, and DRIVER_ATTR RW and RO macros
to help with converting code to use attributes properly.
thanks,
greg k-h
A number of parts of the kernel created their own version of this, might
as well have the sysfs core provide it instead.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
include/linux/sysfs.h | 2 ++
kernel/events/core.c | 2 --
mm/backing-dev.c | 2 --
3 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index e2cee22..9cd20c8 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -79,6 +79,8 @@ struct attribute_group {
.show = _name##_show, \
}
+#define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
+
#define __ATTR_NULL { .attr = { .name = NULL } }
#ifdef CONFIG_DEBUG_LOCK_ALLOC
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1833bc5..95191bf 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6212,8 +6212,6 @@ perf_event_mux_interval_ms_store(struct device *dev,
return count;
}
-#define __ATTR_RW(attr) __ATTR(attr, 0644, attr##_show, attr##_store)
-
static struct device_attribute pmu_dev_attrs[] = {
__ATTR_RO(type),
__ATTR_RW(perf_event_mux_interval_ms),
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index d014ee5..e04454c 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -232,8 +232,6 @@ static ssize_t stable_pages_required_show(struct device *dev,
bdi_cap_stable_pages_required(bdi) ? 1 : 0);
}
-#define __ATTR_RW(attr) __ATTR(attr, 0644, attr##_show, attr##_store)
-
static struct device_attribute bdi_dev_attrs[] = {
__ATTR_RW(read_ahead_kb),
__ATTR_RW(min_ratio),
--
1.8.3.rc0.20.gb99dd2e
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
include/linux/sysfs.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9cd20c8..f62ff01 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -94,6 +94,15 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
+#define ATTRIBUTE_GROUPS(name) \
+static const struct attribute_group name##_group = { \
+ .attrs = name##_attrs, \
+}; \
+static const struct attribute_group *name##_groups[] = { \
+ &name##_group, \
+ NULL, \
+}
+
#define attr_name(_attr) (_attr).attr.name
struct file;
--
1.8.3.rc0.20.gb99dd2e
From: Guenter Roeck <[email protected]>
device_create_groups lets callers create devices as well as associated
sysfs attributes with a single call. This avoids race conditions seen
if sysfs attributes on new devices are created later.
[fixed up comment block placement and add checks for printk buffer
formats - gregkh]
Signed-off-by: Guenter Roeck <[email protected]>
Cc: Jean Delvare <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/base/core.c | 111 ++++++++++++++++++++++++++++++++++++-------------
include/linux/device.h | 5 +++
2 files changed, 88 insertions(+), 28 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index dc3ea23..a8aae18 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1667,34 +1667,11 @@ static void device_create_release(struct device *dev)
kfree(dev);
}
-/**
- * device_create_vargs - creates a device and registers it with sysfs
- * @class: pointer to the struct class that this device should be registered to
- * @parent: pointer to the parent struct device of this new device, if any
- * @devt: the dev_t for the char device to be added
- * @drvdata: the data to be added to the device for callbacks
- * @fmt: string for the device's name
- * @args: va_list for the device's name
- *
- * This function can be used by char device classes. A struct device
- * will be created in sysfs, registered to the specified class.
- *
- * A "dev" file will be created, showing the dev_t for the device, if
- * the dev_t is not 0,0.
- * If a pointer to a parent struct device is passed in, the newly created
- * struct device will be a child of that device in sysfs.
- * The pointer to the struct device will be returned from the call.
- * Any further sysfs files that might be required can be created using this
- * pointer.
- *
- * Returns &struct device pointer on success, or ERR_PTR() on error.
- *
- * Note: the struct class passed to this function must have previously
- * been created with a call to class_create().
- */
-struct device *device_create_vargs(struct class *class, struct device *parent,
- dev_t devt, void *drvdata, const char *fmt,
- va_list args)
+static struct device *
+device_create_groups_vargs(struct class *class, struct device *parent,
+ dev_t devt, void *drvdata,
+ const struct attribute_group **groups,
+ const char *fmt, va_list args)
{
struct device *dev = NULL;
int retval = -ENODEV;
@@ -1711,6 +1688,7 @@ struct device *device_create_vargs(struct class *class, struct device *parent,
dev->devt = devt;
dev->class = class;
dev->parent = parent;
+ dev->groups = groups;
dev->release = device_create_release;
dev_set_drvdata(dev, drvdata);
@@ -1728,6 +1706,39 @@ error:
put_device(dev);
return ERR_PTR(retval);
}
+
+/**
+ * device_create_vargs - creates a device and registers it with sysfs
+ * @class: pointer to the struct class that this device should be registered to
+ * @parent: pointer to the parent struct device of this new device, if any
+ * @devt: the dev_t for the char device to be added
+ * @drvdata: the data to be added to the device for callbacks
+ * @fmt: string for the device's name
+ * @args: va_list for the device's name
+ *
+ * This function can be used by char device classes. A struct device
+ * will be created in sysfs, registered to the specified class.
+ *
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct device is passed in, the newly created
+ * struct device will be a child of that device in sysfs.
+ * The pointer to the struct device will be returned from the call.
+ * Any further sysfs files that might be required can be created using this
+ * pointer.
+ *
+ * Returns &struct device pointer on success, or ERR_PTR() on error.
+ *
+ * Note: the struct class passed to this function must have previously
+ * been created with a call to class_create().
+ */
+struct device *device_create_vargs(struct class *class, struct device *parent,
+ dev_t devt, void *drvdata, const char *fmt,
+ va_list args)
+{
+ return device_create_groups_vargs(class, parent, devt, drvdata, NULL,
+ fmt, args);
+}
EXPORT_SYMBOL_GPL(device_create_vargs);
/**
@@ -1767,6 +1778,50 @@ struct device *device_create(struct class *class, struct device *parent,
}
EXPORT_SYMBOL_GPL(device_create);
+/**
+ * device_create_with_groups - creates a device and registers it with sysfs
+ * @class: pointer to the struct class that this device should be registered to
+ * @parent: pointer to the parent struct device of this new device, if any
+ * @devt: the dev_t for the char device to be added
+ * @drvdata: the data to be added to the device for callbacks
+ * @groups: NULL-terminated list of attribute groups to be created
+ * @fmt: string for the device's name
+ *
+ * This function can be used by char device classes. A struct device
+ * will be created in sysfs, registered to the specified class.
+ * Additional attributes specified in the groups parameter will also
+ * be created automatically.
+ *
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct device is passed in, the newly created
+ * struct device will be a child of that device in sysfs.
+ * The pointer to the struct device will be returned from the call.
+ * Any further sysfs files that might be required can be created using this
+ * pointer.
+ *
+ * Returns &struct device pointer on success, or ERR_PTR() on error.
+ *
+ * Note: the struct class passed to this function must have previously
+ * been created with a call to class_create().
+ */
+struct device *device_create_with_groups(struct class *class,
+ struct device *parent, dev_t devt,
+ void *drvdata,
+ const struct attribute_group **groups,
+ const char *fmt, ...)
+{
+ va_list vargs;
+ struct device *dev;
+
+ va_start(vargs, fmt);
+ dev = device_create_groups_vargs(class, parent, devt, drvdata, groups,
+ fmt, vargs);
+ va_end(vargs);
+ return dev;
+}
+EXPORT_SYMBOL_GPL(device_create_with_groups);
+
static int __match_devt(struct device *dev, const void *data)
{
const dev_t *devt = data;
diff --git a/include/linux/device.h b/include/linux/device.h
index f207a8f..bd5931e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -938,6 +938,11 @@ extern __printf(5, 6)
struct device *device_create(struct class *cls, struct device *parent,
dev_t devt, void *drvdata,
const char *fmt, ...);
+extern __printf(6, 7)
+struct device *device_create_with_groups(struct class *cls,
+ struct device *parent, dev_t devt, void *drvdata,
+ const struct attribute_group **groups,
+ const char *fmt, ...);
extern void device_destroy(struct class *cls, dev_t devt);
/*
--
1.8.3.rc0.20.gb99dd2e
groups should be able to support binary attributes, just like it
supports "normal" attributes. This lets us only handle one type of
structure, groups, throughout the driver core and subsystems, making
binary attributes a "full fledged" part of the driver model, and not
something just "tacked on".
Reported-by: Oliver Schinagl <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
fs/sysfs/group.c | 66 +++++++++++++++++++++++++++++++++++----------------
include/linux/sysfs.h | 4 ++--
2 files changed, 48 insertions(+), 22 deletions(-)
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aec3d5c..e5719c6 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -20,38 +20,64 @@ static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
- int i;
+ struct bin_attribute *const* bin_attr;
- for (i = 0, attr = grp->attrs; *attr; i++, attr++)
- sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+ if (grp->attrs)
+ for (attr = grp->attrs; *attr; attr++)
+ sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+ if (grp->bin_attrs)
+ for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
+ sysfs_remove_bin_file(kobj, *bin_attr);
}
static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp, int update)
{
struct attribute *const* attr;
+ struct bin_attribute *const* bin_attr;
int error = 0, i;
- for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
- umode_t mode = 0;
+ if (grp->attrs) {
+ for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
+ umode_t mode = 0;
+
+ /*
+ * In update mode, we're changing the permissions or
+ * visibility. Do this by first removing then
+ * re-adding (if required) the file.
+ */
+ if (update)
+ sysfs_hash_and_remove(dir_sd, NULL,
+ (*attr)->name);
+ if (grp->is_visible) {
+ mode = grp->is_visible(kobj, *attr, i);
+ if (!mode)
+ continue;
+ }
+ error = sysfs_add_file_mode(dir_sd, *attr,
+ SYSFS_KOBJ_ATTR,
+ (*attr)->mode | mode);
+ if (unlikely(error))
+ break;
+ }
+ if (error) {
+ remove_files(dir_sd, kobj, grp);
+ goto exit;
+ }
+ }
- /* in update mode, we're changing the permissions or
- * visibility. Do this by first removing then
- * re-adding (if required) the file */
- if (update)
- sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
- if (grp->is_visible) {
- mode = grp->is_visible(kobj, *attr, i);
- if (!mode)
- continue;
+ if (grp->bin_attrs) {
+ for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
+ if (update)
+ sysfs_remove_bin_file(kobj, *bin_attr);
+ error = sysfs_create_bin_file(kobj, *bin_attr);
+ if (error)
+ break;
}
- error = sysfs_add_file_mode(dir_sd, *attr, SYSFS_KOBJ_ATTR,
- (*attr)->mode | mode);
- if (unlikely(error))
- break;
+ if (error)
+ remove_files(dir_sd, kobj, grp);
}
- if (error)
- remove_files(dir_sd, kobj, grp);
+exit:
return error;
}
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index d50a96b..2c3b6a3 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -21,6 +21,7 @@
struct kobject;
struct module;
+struct bin_attribute;
enum kobj_ns_type;
struct attribute {
@@ -59,10 +60,9 @@ struct attribute_group {
umode_t (*is_visible)(struct kobject *,
struct attribute *, int);
struct attribute **attrs;
+ struct bin_attribute **bin_attrs;
};
-
-
/**
* Use these macros to make defining attributes easier. See include/linux/device.h
* for examples..
--
1.8.3.rc0.20.gb99dd2e
We should be using groups, not attribute lists, for classes to allow
subdirectories, and soon, binary files. Groups are just more flexible
overall, so add them.
The dev_attrs list will go away after all in-kernel users are converted
to use dev_groups.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/base/core.c | 9 ++++++++-
include/linux/device.h | 4 +++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index a8aae18..8856d74 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -528,9 +528,12 @@ static int device_add_attrs(struct device *dev)
int error;
if (class) {
- error = device_add_attributes(dev, class->dev_attrs);
+ error = device_add_groups(dev, class->dev_groups);
if (error)
return error;
+ error = device_add_attributes(dev, class->dev_attrs);
+ if (error)
+ goto err_remove_class_groups;
error = device_add_bin_attributes(dev, class->dev_bin_attrs);
if (error)
goto err_remove_class_attrs;
@@ -563,6 +566,9 @@ static int device_add_attrs(struct device *dev)
err_remove_class_attrs:
if (class)
device_remove_attributes(dev, class->dev_attrs);
+ err_remove_class_groups:
+ if (class)
+ device_remove_groups(dev, class->dev_groups);
return error;
}
@@ -581,6 +587,7 @@ static void device_remove_attrs(struct device *dev)
if (class) {
device_remove_attributes(dev, class->dev_attrs);
device_remove_bin_attributes(dev, class->dev_bin_attrs);
+ device_remove_groups(dev, class->dev_groups);
}
}
diff --git a/include/linux/device.h b/include/linux/device.h
index bd5931e..22b546a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -320,6 +320,7 @@ int subsys_virtual_register(struct bus_type *subsys,
* @name: Name of the class.
* @owner: The module owner.
* @class_attrs: Default attributes of this class.
+ * @dev_groups: Default attributes of the devices that belong to the class.
* @dev_attrs: Default attributes of the devices belong to the class.
* @dev_bin_attrs: Default binary attributes of the devices belong to the class.
* @dev_kobj: The kobject that represents this class and links it into the hierarchy.
@@ -349,7 +350,8 @@ struct class {
struct module *owner;
struct class_attribute *class_attrs;
- struct device_attribute *dev_attrs;
+ struct device_attribute *dev_attrs; /* use dev_groups instead */
+ const struct attribute_group **dev_groups;
struct bin_attribute *dev_bin_attrs;
struct kobject *dev_kobj;
--
1.8.3.rc0.20.gb99dd2e
Make it easier to create attributes without having to always audit the
mode settings.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
include/linux/device.h | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/include/linux/device.h b/include/linux/device.h
index bcf8c0d..f207a8f 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -47,7 +47,11 @@ struct bus_attribute {
};
#define BUS_ATTR(_name, _mode, _show, _store) \
-struct bus_attribute bus_attr_##_name = __ATTR(_name, _mode, _show, _store)
+ struct bus_attribute bus_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define BUS_ATTR_RW(_name) \
+ struct bus_attribute bus_attr_##_name = __ATTR_RW(_name)
+#define BUS_ATTR_RO(_name) \
+ struct bus_attribute bus_attr_##_name = __ATTR_RO(_name)
extern int __must_check bus_create_file(struct bus_type *,
struct bus_attribute *);
@@ -261,9 +265,12 @@ struct driver_attribute {
size_t count);
};
-#define DRIVER_ATTR(_name, _mode, _show, _store) \
-struct driver_attribute driver_attr_##_name = \
- __ATTR(_name, _mode, _show, _store)
+#define DRIVER_ATTR(_name, _mode, _show, _store) \
+ struct driver_attribute driver_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define DRIVER_ATTR_RW(_name) \
+ struct driver_attribute driver_attr_##_name = __ATTR_RW(_name)
+#define DRIVER_ATTR_RO(_name) \
+ struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
extern int __must_check driver_create_file(struct device_driver *driver,
const struct driver_attribute *attr);
@@ -414,8 +421,12 @@ struct class_attribute {
const struct class_attribute *attr);
};
-#define CLASS_ATTR(_name, _mode, _show, _store) \
-struct class_attribute class_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define CLASS_ATTR(_name, _mode, _show, _store) \
+ struct class_attribute class_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define CLASS_ATTR_RW(_name) \
+ struct class_attribute class_attr_##_name = __ATTR_RW(_name)
+#define CLASS_ATTR_RO(_name) \
+ struct class_attribute class_attr_##_name = __ATTR_RO(_name)
extern int __must_check class_create_file(struct class *class,
const struct class_attribute *attr);
@@ -423,7 +434,6 @@ extern void class_remove_file(struct class *class,
const struct class_attribute *attr);
/* Simple class attribute that is just a static string */
-
struct class_attribute_string {
struct class_attribute attr;
char *str;
@@ -512,6 +522,10 @@ ssize_t device_store_bool(struct device *dev, struct device_attribute *attr,
#define DEVICE_ATTR(_name, _mode, _show, _store) \
struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define DEVICE_ATTR_RW(_name) \
+ struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
+#define DEVICE_ATTR_RO(_name) \
+ struct device_attribute dev_attr_##_name = __ATTR_RO(_name)
#define DEVICE_ULONG_ATTR(_name, _mode, _var) \
struct dev_ext_attribute dev_attr_##_name = \
{ __ATTR(_name, _mode, device_show_ulong, device_store_ulong), &(_var) }
--
1.8.3.rc0.20.gb99dd2e
This makes it easier to create static binary attributes, which is needed
in a number of drivers, instead of "open coding" them.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
include/linux/sysfs.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index f62ff01..d50a96b 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -132,6 +132,15 @@ struct bin_attribute {
*/
#define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&(bin_attr)->attr)
+/* macro to create static binary attributes easier */
+#define BIN_ATTR(_name, _mode, _read, _write, _size) \
+struct bin_attribute bin_attr_##_name = { \
+ .attr = {.name = __stringify(_name), .mode = _mode }, \
+ .read = _read, \
+ .write = _write, \
+ .size = _size, \
+}
+
struct sysfs_ops {
ssize_t (*show)(struct kobject *, struct attribute *,char *);
ssize_t (*store)(struct kobject *,struct attribute *,const char *, size_t);
--
1.8.3.rc0.20.gb99dd2e
On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> Hi all,
>
> Here's the second iteration of the patchset to add better attribute
> group support to the driver core and sysfs.
>
> I've tested these (shocker!) and everything works fine with them (I'm
> sending this from Linus's latest kernel with these 7 on top of it.)
>
> I'd like to send these to Linus for 3.11 unless someone objects.
>
> Oliver, please use this series instead of the last one, it has fixes
> that Guenter pointed out that would have crashed your box at boot.
>
> changes from v2:
> - actually boots
> - 7th patch added properly
> - added BUS_ATTR, CLASS_ATTR, and DRIVER_ATTR RW and RO macros
> to help with converting code to use attributes properly.
>
Looks good this time. And, yes, it does boot and work, including patch #7.
Feel free to add
Reviewed-by: Guenter Roeck <[email protected]>
and if you like
Tested-by: Guenter Roeck <[email protected]>
to the series.
Thanks,
Guenter
On Wed, Jul 10, 2013 at 10:23:57PM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> > Hi all,
> >
> > Here's the second iteration of the patchset to add better attribute
> > group support to the driver core and sysfs.
> >
> > I've tested these (shocker!) and everything works fine with them (I'm
> > sending this from Linus's latest kernel with these 7 on top of it.)
> >
> > I'd like to send these to Linus for 3.11 unless someone objects.
> >
> > Oliver, please use this series instead of the last one, it has fixes
> > that Guenter pointed out that would have crashed your box at boot.
> >
> > changes from v2:
> > - actually boots
> > - 7th patch added properly
> > - added BUS_ATTR, CLASS_ATTR, and DRIVER_ATTR RW and RO macros
> > to help with converting code to use attributes properly.
> >
> Looks good this time. And, yes, it does boot and work, including patch #7.
> Feel free to add
>
> Reviewed-by: Guenter Roeck <[email protected]>
>
> and if you like
>
> Tested-by: Guenter Roeck <[email protected]>
>
> to the series.
Thanks for the testing and review, I'll add this when I commit them
sometime next week.
greg k-h
On 11-07-13 08:28, Greg Kroah-Hartman wrote:
> On Wed, Jul 10, 2013 at 10:23:57PM -0700, Guenter Roeck wrote:
>> On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
>>> Hi all,
>>>
>>> Here's the second iteration of the patchset to add better attribute
>>> group support to the driver core and sysfs.
>>>
>>> I've tested these (shocker!) and everything works fine with them (I'm
>>> sending this from Linus's latest kernel with these 7 on top of it.)
>>>
>>> I'd like to send these to Linus for 3.11 unless someone objects.
>>>
>>> Oliver, please use this series instead of the last one, it has fixes
>>> that Guenter pointed out that would have crashed your box at boot.
It still crashes, but haven't read back all. I do have some extra helper
macro's patch incoming once i make sure it boots.
(it boots but segfaults) give me a few hours to report back.
oliver
>>>
>>> changes from v2:
>>> - actually boots
>>> - 7th patch added properly
>>> - added BUS_ATTR, CLASS_ATTR, and DRIVER_ATTR RW and RO macros
>>> to help with converting code to use attributes properly.
>>>
>> Looks good this time. And, yes, it does boot and work, including patch #7.
>> Feel free to add
>>
>> Reviewed-by: Guenter Roeck <[email protected]>
>>
>> and if you like
>>
>> Tested-by: Guenter Roeck <[email protected]>
>>
>> to the series.
>
> Thanks for the testing and review, I'll add this when I commit them
> sometime next week.
>
> greg k-h
>
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> groups should be able to support binary attributes, just like it
> supports "normal" attributes. This lets us only handle one type of
> structure, groups, throughout the driver core and subsystems, making
> binary attributes a "full fledged" part of the driver model, and not
> something just "tacked on".
However when only using binary attributes it warns and doesn't create
anything. The attached patch fixes that.
>
> Reported-by: Oliver Schinagl <[email protected]>
If I may be so bold and ask to change my e-mail address to
<[email protected]> that would be kind. I use the +list delimiter to
put all the mailing list mails in a separate mailbox.
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> fs/sysfs/group.c | 66 +++++++++++++++++++++++++++++++++++----------------
> include/linux/sysfs.h | 4 ++--
> 2 files changed, 48 insertions(+), 22 deletions(-)
>
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index aec3d5c..e5719c6 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -20,38 +20,64 @@ static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> const struct attribute_group *grp)
> {
> struct attribute *const* attr;
> - int i;
> + struct bin_attribute *const* bin_attr;
>
> - for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> - sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
> + if (grp->attrs)
> + for (attr = grp->attrs; *attr; attr++)
> + sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
> + if (grp->bin_attrs)
> + for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
> + sysfs_remove_bin_file(kobj, *bin_attr);
> }
>
> static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> const struct attribute_group *grp, int update)
> {
> struct attribute *const* attr;
> + struct bin_attribute *const* bin_attr;
> int error = 0, i;
>
> - for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> - umode_t mode = 0;
> + if (grp->attrs) {
> + for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> + umode_t mode = 0;
> +
> + /*
> + * In update mode, we're changing the permissions or
> + * visibility. Do this by first removing then
> + * re-adding (if required) the file.
> + */
> + if (update)
> + sysfs_hash_and_remove(dir_sd, NULL,
> + (*attr)->name);
> + if (grp->is_visible) {
> + mode = grp->is_visible(kobj, *attr, i);
> + if (!mode)
> + continue;
> + }
> + error = sysfs_add_file_mode(dir_sd, *attr,
> + SYSFS_KOBJ_ATTR,
> + (*attr)->mode | mode);
> + if (unlikely(error))
> + break;
> + }
> + if (error) {
> + remove_files(dir_sd, kobj, grp);
> + goto exit;
> + }
> + }
>
> - /* in update mode, we're changing the permissions or
> - * visibility. Do this by first removing then
> - * re-adding (if required) the file */
> - if (update)
> - sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
> - if (grp->is_visible) {
> - mode = grp->is_visible(kobj, *attr, i);
> - if (!mode)
> - continue;
> + if (grp->bin_attrs) {
> + for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> + if (update)
> + sysfs_remove_bin_file(kobj, *bin_attr);
> + error = sysfs_create_bin_file(kobj, *bin_attr);
> + if (error)
> + break;
> }
> - error = sysfs_add_file_mode(dir_sd, *attr, SYSFS_KOBJ_ATTR,
> - (*attr)->mode | mode);
> - if (unlikely(error))
> - break;
> + if (error)
> + remove_files(dir_sd, kobj, grp);
> }
> - if (error)
> - remove_files(dir_sd, kobj, grp);
> +exit:
> return error;
> }
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index d50a96b..2c3b6a3 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -21,6 +21,7 @@
>
> struct kobject;
> struct module;
> +struct bin_attribute;
> enum kobj_ns_type;
>
> struct attribute {
> @@ -59,10 +60,9 @@ struct attribute_group {
> umode_t (*is_visible)(struct kobject *,
> struct attribute *, int);
> struct attribute **attrs;
> + struct bin_attribute **bin_attrs;
> };
>
> -
> -
> /**
> * Use these macros to make defining attributes easier. See include/linux/device.h
> * for examples..
>
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> To make it easier for driver subsystems to work with attribute groups,
> create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
> typing for the most common use for attribute groups.
But binary groups are discriminated against :(
The attached patch should help here.
I suppose one more additional helper wouldn't be bad to have:
#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)
but wasn't sure if that wasn't a little overkill. I gladly add it in an
additional patch.
>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> include/linux/sysfs.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 9cd20c8..f62ff01 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -94,6 +94,15 @@ struct attribute_group {
> #define __ATTR_IGNORE_LOCKDEP __ATTR
> #endif
>
> +#define ATTRIBUTE_GROUPS(name) \
> +static const struct attribute_group name##_group = { \
> + .attrs = name##_attrs, \
> +}; \
> +static const struct attribute_group *name##_groups[] = { \
> + &name##_group, \
> + NULL, \
> +}
> +
> #define attr_name(_attr) (_attr).attr.name
>
> struct file;
>
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> To make it easier for driver subsystems to work with attribute groups,
> create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
> typing for the most common use for attribute groups.
Forgot to add this trivial little cleanup patch with my previous mail, I
know you don't like patch dependancies, but this one depends on the
previous one as that included stat.h, if you decide my previous patch
isn't good, i can redo this one with stat.h included (and move atomic.h
to the top to keep includes alphabetically ordered).
Oliver
>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> include/linux/sysfs.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 9cd20c8..f62ff01 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -94,6 +94,15 @@ struct attribute_group {
> #define __ATTR_IGNORE_LOCKDEP __ATTR
> #endif
>
> +#define ATTRIBUTE_GROUPS(name) \
> +static const struct attribute_group name##_group = { \
> + .attrs = name##_attrs, \
> +}; \
> +static const struct attribute_group *name##_groups[] = { \
> + &name##_group, \
> + NULL, \
> +}
> +
> #define attr_name(_attr) (_attr).attr.name
>
> struct file;
>
On Thu, Jul 11, 2013 at 01:45:53PM +0200, Oliver Schinagl wrote:
> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> >groups should be able to support binary attributes, just like it
> >supports "normal" attributes. This lets us only handle one type of
> >structure, groups, throughout the driver core and subsystems, making
> >binary attributes a "full fledged" part of the driver model, and not
> >something just "tacked on".
> However when only using binary attributes it warns and doesn't
> create anything. The attached patch fixes that.
Ah, missed that, I didn't have a binary-only attribute group to test
that out, thanks.
> >Reported-by: Oliver Schinagl <[email protected]>
> If I may be so bold and ask to change my e-mail address to
> <[email protected]> that would be kind. I use the +list delimiter
> to put all the mailing list mails in a separate mailbox.
Sure, no problem at all, will do so. And I'll queue up your attached
patch as well, it looks great.
greg k-h
On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> >To make it easier for driver subsystems to work with attribute groups,
> >create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
> >typing for the most common use for attribute groups.
> But binary groups are discriminated against :(
Yes, as they are "rarer" by far, as they should be. binary sysfs files
should almost never be used, as they are only "pass-through" files to
the hardware, so I want to see you do more work in order to use them, as
they should not be created lightly.
> The attached patch should help here.
Can you give me an example of using these macros? I seem to be lost in
them somehow, or maybe my morning coffee just hasn't kicked in...
> I suppose one more additional helper wouldn't be bad to have:
>
> #define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
> ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
> ATTRIBUTE_(BIN_)GROUPS(_name)
Would that ever be needed?
> >From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
> From: Oliver Schinagl <[email protected]>
> Date: Thu, 11 Jul 2013 13:48:18 +0200
> Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
>
> With the recent changes to sysfs there's various helper macro's.
> However there's no RW, RO BIN_ helper macro's. This patch adds them.
>
> Additionally there are no BIN_ group helpers so there's that aswell
> Moreso, if both bin and normal attribute groups are used, there's a
> simple helper for that, though the naming code be better. _TXT_ for the
> show/store ones and neither TXT or BIN for both, but that would change
> things to extensivly.
>
> Finally there's also helpers for ATTRIBUTE_ATTRS.
>
> After this patch, create default attributes can be as easy as:
>
> ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
> ATTRIBUTE_BIN_GROUPS(name);
>
> Signed-off-by: Oliver Schinagl <[email protected]>
> ---
> include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 84 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 2c3b6a3..0ebed11 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -17,6 +17,7 @@
> #include <linux/list.h>
> #include <linux/lockdep.h>
> #include <linux/kobject_ns.h>
> +#include <linux/stat.h>
> #include <linux/atomic.h>
>
> struct kobject;
> @@ -94,15 +95,32 @@ struct attribute_group {
> #define __ATTR_IGNORE_LOCKDEP __ATTR
> #endif
>
> -#define ATTRIBUTE_GROUPS(name) \
> -static const struct attribute_group name##_group = { \
> - .attrs = name##_attrs, \
> +#define __ATTRIBUTE_GROUPS(_name) \
> +static const struct attribute_group *_name##_groups[] = { \
> + &_name##_group, \
> + NULL, \
> +}
> +
> +#define ATTRIBUTE_GROUPS(_name) \
> +static const struct attribute_group _name##_group = { \
> + .attrs = _name##_attrs, \
> }; \
> -static const struct attribute_group *name##_groups[] = { \
> - &name##_group, \
> +__ATTRIBUTE_GROUPS(_name)
> +
> +#define __ATTRIBUTE_ATTRS(_name) \
> +struct bin_attribute *_name##_attrs[] = { \
> + &_name##_attr, \
> NULL, \
> }
>
> +#define ATTRIBUTE_ATTR_RO(_name, _size) \
> +struct attribute _name##_attr = __ATTR_RO(_name, _size); \
> +__ATTRIBUTE_ATTRS(_name)
> +
> +#define ATTRIBUTE_ATTR_RW(_name, _size) \
> +struct attribute _name##_attr = __ATTR_RW(_name, _size); \
> +__ATTRIBUTE_ATTRS(_name)
What do these two help out with? "attribute attribute read-write" seems
a bit "clunky", don't you think? :)
> +
> #define attr_name(_attr) (_attr).attr.name
>
> struct file;
> @@ -132,15 +150,69 @@ struct bin_attribute {
> */
> #define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&(bin_attr)->attr)
>
> -/* macro to create static binary attributes easier */
> -#define BIN_ATTR(_name, _mode, _read, _write, _size) \
> -struct bin_attribute bin_attr_##_name = { \
> - .attr = {.name = __stringify(_name), .mode = _mode }, \
> - .read = _read, \
> - .write = _write, \
> - .size = _size, \
> +/* macros to create static binary attributes easier */
> +#define __BIN_ATTR(_name, _mode, _read, _write, _size) { \
> + .attr = { .name = __stringify(_name), .mode = _mode }, \
> + .read = _read, \
> + .write = _write, \
> + .size = _size, \
> +}
> +
> +#define __BIN_ATTR_RO(_name, _size) { \
> + .attr = { .name = __stringify(_name), .mode = S_IRUGO }, \
> + .read = _name##_read, \
> + .size = _size, \
> +}
> +
> +#define __BIN_ATTR_RW(_name, _size) __BIN_ATTR(_name, \
> + (S_IWUSR | S_IRUGO), _name##_read, \
> + _name##_write)
> +
> +#define __BIN_ATTR_NULL __ATTR_NULL
> +
> +#define BIN_ATTR(_name, _mode, _read, _write, _size) \
> +struct bin_attribute bin_attr_##_name = __BIN_ATTR(_name, _mode, _read, \
> + _write, _size)
> +
> +#define BIN_RO_ATTR(_name, _size) \
> +struct bin_attribute bin_attr_##_name = __BIN_ATTR_RO(_name, _size)
> +
> +#define BIN_RW_ATTR(_name, _size) \
> +struct bin_attribute bin_attr_##_name = __BIN_ATTR_RW(_name, _size)
To be consistent, these shoudl be BIN_ATTR_RO and BIN_ATTR_RW, right?
These all look fine to me, thanks.
It's these that I'm confused about:
> +
> +#define __ATTRIBUTE_BIN_GROUPS(_name) \
> +static const struct attribute_group *_name##_bin_groups[] = { \
> + &_name##_bin_group, \
> + NULL, \
> }
>
> +#define ATTRIBUTE_BIN_GROUPS(_name) \
> +static const struct attribute_group _name##_bin_group = { \
> + .bin_attrs = _name##_bin_attrs, \
> +}; \
> +__ATTRIBUTE_BIN_GROUPS(_name)
> +
> +#define ATTRIBUTE_FULL_GROUPS(_name) \
> +static const struct attribute_group _name##_full_group = { \
> + .attrs = _name##_attrs, \
> + .bin_attrs = _name##_bin_attrs, \
> +}; \
> +__ATTRIBUTE_GROUPS(_name); __ATTRIBUTE_BIN_GROUPS(_name)
> +
> +#define __ATTRIBUTE_BIN_ATTRS(_name) \
> +struct bin_attribute *_name##_bin_attrs[] = { \
> + &_name##_bin_attr, \
> + NULL, \
> +}
> +
> +#define ATTRIBUTE_BIN_ATTR_RO(_name, _size) \
> +struct bin_attribute _name##_bin_attr = __BIN_ATTR_RO(_name, _size); \
> +__ATTRIBUTE_BIN_ATTRS(_name)
> +
> +#define ATTRIBUTE_BIN_ATTR_RW(_name, _size) \
> +struct bin_attribute _name##_bin_attr = __BIN_ATTR_RW(_name, _size); \
> +__ATTRIBUTE_BIN_ATTRS(_name)
Can you show me how these would be used in a real-world example?
thanks,
greg k-h
On Thu, Jul 11, 2013 at 02:03:34PM +0200, Oliver Schinagl wrote:
> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> >To make it easier for driver subsystems to work with attribute groups,
> >create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
> >typing for the most common use for attribute groups.
> Forgot to add this trivial little cleanup patch with my previous
> mail, I know you don't like patch dependancies, but this one depends
> on the previous one as that included stat.h, if you decide my
> previous patch isn't good, i can redo this one with stat.h included
> (and move atomic.h to the top to keep includes alphabetically
> ordered).
No problem, I can merge this with your other one when I can figure out
what that one is really doing :)
thanks,
greg k-h
On 07/11/13 19:06, Greg Kroah-Hartman wrote:
> On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
>> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
>>> To make it easier for driver subsystems to work with attribute groups,
>>> create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
>>> typing for the most common use for attribute groups.
>> But binary groups are discriminated against :(
>
> Yes, as they are "rarer" by far, as they should be. binary sysfs files
> should almost never be used, as they are only "pass-through" files to
> the hardware, so I want to see you do more work in order to use them, as
> they should not be created lightly.
I guess I can see a valid reason here, but they are only helper macro's
making life easier for people who do need to use these and are on par
with the non-binary versions. And we already have quite some binary
attributes, probably far less then normal ones :)
Anyway, wouldn't all users be reviewed anyway? But I guess it's a small
safety net to make it not TOO easy.
>
>> The attached patch should help here.
>
> Can you give me an example of using these macros? I seem to be lost in
> them somehow, or maybe my morning coffee just hasn't kicked in...
Yeah, I kinda added the whole shebang there :) I was trying being helpful :(
>
>> I suppose one more additional helper wouldn't be bad to have:
>>
>> #define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
>> ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
>> ATTRIBUTE_(BIN_)GROUPS(_name)
>
> Would that ever be needed?
Of ourse, by the lazy :)
I think now you create an attribute in a group as this (with this patch):
ATTRIBUTE_ATTR_RO(name, SIZE);
ATTRIBUTE_GROUPS(name);
.group = name;
After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);
.group = name;
saves a whole line :)
>
>> >From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
>> From: Oliver Schinagl <[email protected]>
>> Date: Thu, 11 Jul 2013 13:48:18 +0200
>> Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
>>
>> With the recent changes to sysfs there's various helper macro's.
>> However there's no RW, RO BIN_ helper macro's. This patch adds them.
>>
>> Additionally there are no BIN_ group helpers so there's that aswell
>> Moreso, if both bin and normal attribute groups are used, there's a
>> simple helper for that, though the naming code be better. _TXT_ for the
>> show/store ones and neither TXT or BIN for both, but that would change
>> things to extensivly.
>>
>> Finally there's also helpers for ATTRIBUTE_ATTRS.
>>
>> After this patch, create default attributes can be as easy as:
>>
>> ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
>> ATTRIBUTE_BIN_GROUPS(name);
>>
>> Signed-off-by: Oliver Schinagl <[email protected]>
>> ---
>> include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 84 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>> index 2c3b6a3..0ebed11 100644
>> --- a/include/linux/sysfs.h
>> +++ b/include/linux/sysfs.h
>> @@ -17,6 +17,7 @@
>> #include <linux/list.h>
>> #include <linux/lockdep.h>
>> #include <linux/kobject_ns.h>
>> +#include <linux/stat.h>
>> #include <linux/atomic.h>
>>
>> struct kobject;
>> @@ -94,15 +95,32 @@ struct attribute_group {
>> #define __ATTR_IGNORE_LOCKDEP __ATTR
>> #endif
>>
>> -#define ATTRIBUTE_GROUPS(name) \
>> -static const struct attribute_group name##_group = { \
>> - .attrs = name##_attrs, \
>> +#define __ATTRIBUTE_GROUPS(_name) \
>> +static const struct attribute_group *_name##_groups[] = { \
>> + &_name##_group, \
>> + NULL, \
>> +}
>> +
>> +#define ATTRIBUTE_GROUPS(_name) \
>> +static const struct attribute_group _name##_group = { \
>> + .attrs = _name##_attrs, \
>> }; \
>> -static const struct attribute_group *name##_groups[] = { \
>> - &name##_group, \
>> +__ATTRIBUTE_GROUPS(_name)
>> +
>> +#define __ATTRIBUTE_ATTRS(_name) \
>> +struct bin_attribute *_name##_attrs[] = { \
typo here, scrap bin_ copy paste fail!
>> + &_name##_attr, \
>> NULL, \
>> }
>>
>> +#define ATTRIBUTE_ATTR_RO(_name, _size) \
>> +struct attribute _name##_attr = __ATTR_RO(_name, _size); \
>> +__ATTRIBUTE_ATTRS(_name)
>> +
>> +#define ATTRIBUTE_ATTR_RW(_name, _size) \
>> +struct attribute _name##_attr = __ATTR_RW(_name, _size); \
>> +__ATTRIBUTE_ATTRS(_name)
>
> What do these two help out with? "attribute attribute read-write" seems
> a bit "clunky", don't you think? :)
I aggree, but I tried to stick with the ATTRIBUTE_GROUP naming and
that's the best I could come up with.
Unless I completely misunderstood (which isn't all that unlikely) the
following is needed to create a group using a .group.
So you pass group an array of attribute_group pointers. The
ATTRIBUTE_GROUPS helps there, right? It saves the typing of creating the
array of groups and adding groups to that.
So a group consists of an array of attributes if I understood right and
that array needs to be filled with pointers attributes? well those
ATTRIBUTE_ATTR's do just that. Granted, maybe the naming is poor, but
just ATTRS() felt to short.
>
>> +
>> #define attr_name(_attr) (_attr).attr.name
>>
>> struct file;
>> @@ -132,15 +150,69 @@ struct bin_attribute {
>> */
>> #define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&(bin_attr)->attr)
>>
>> -/* macro to create static binary attributes easier */
>> -#define BIN_ATTR(_name, _mode, _read, _write, _size) \
>> -struct bin_attribute bin_attr_##_name = { \
>> - .attr = {.name = __stringify(_name), .mode = _mode }, \
>> - .read = _read, \
>> - .write = _write, \
>> - .size = _size, \
>> +/* macros to create static binary attributes easier */
>> +#define __BIN_ATTR(_name, _mode, _read, _write, _size) { \
>> + .attr = { .name = __stringify(_name), .mode = _mode }, \
>> + .read = _read, \
>> + .write = _write, \
>> + .size = _size, \
>> +}
>> +
>> +#define __BIN_ATTR_RO(_name, _size) { \
>> + .attr = { .name = __stringify(_name), .mode = S_IRUGO }, \
>> + .read = _name##_read, \
>> + .size = _size, \
>> +}
>> +
>> +#define __BIN_ATTR_RW(_name, _size) __BIN_ATTR(_name, \
>> + (S_IWUSR | S_IRUGO), _name##_read, \
>> + _name##_write)
>> +
>> +#define __BIN_ATTR_NULL __ATTR_NULL
>> +
>> +#define BIN_ATTR(_name, _mode, _read, _write, _size) \
>> +struct bin_attribute bin_attr_##_name = __BIN_ATTR(_name, _mode, _read, \
>> + _write, _size)
>> +
>> +#define BIN_RO_ATTR(_name, _size) \
>> +struct bin_attribute bin_attr_##_name = __BIN_ATTR_RO(_name, _size)
>> +
>> +#define BIN_RW_ATTR(_name, _size) \
>> +struct bin_attribute bin_attr_##_name = __BIN_ATTR_RW(_name, _size)
>
> To be consistent, these shoudl be BIN_ATTR_RO and BIN_ATTR_RW, right?
Yes, I too did this before morning coffee, and I don't drink coffee!
>
> These all look fine to me, thanks.
>
> It's these that I'm confused about:
>
>> +
>> +#define __ATTRIBUTE_BIN_GROUPS(_name) \
>> +static const struct attribute_group *_name##_bin_groups[] = { \
>> + &_name##_bin_group, \
>> + NULL, \
>> }
This is just a helper for the ones below.
>>
>> +#define ATTRIBUTE_BIN_GROUPS(_name) \
>> +static const struct attribute_group _name##_bin_group = { \
>> + .bin_attrs = _name##_bin_attrs, \
>> +}; \
>> +__ATTRIBUTE_BIN_GROUPS(_name)
This is the equiv. of ATTRIBUTE_GROUPS(_name) which creates an attribute
group, with only a binary attribute instead.
>> +
>> +#define ATTRIBUTE_FULL_GROUPS(_name) \
>> +static const struct attribute_group _name##_full_group = { \
>> + .attrs = _name##_attrs, \
>> + .bin_attrs = _name##_bin_attrs, \
>> +}; \
>> +__ATTRIBUTE_GROUPS(_name); __ATTRIBUTE_BIN_GROUPS(_name)
This one probably should go, it defines both, and since binaries should
be used cautiously, it's not really needed I guess.
>> +
>> +#define __ATTRIBUTE_BIN_ATTRS(_name) \
>> +struct bin_attribute *_name##_bin_attrs[] = { \
>> + &_name##_bin_attr, \
>> + NULL, \
>> +}
Helper macro again for below
>> +
>> +#define ATTRIBUTE_BIN_ATTR_RO(_name, _size) \
>> +struct bin_attribute _name##_bin_attr = __BIN_ATTR_RO(_name, _size); \
>> +__ATTRIBUTE_BIN_ATTRS(_name)
>> +
>> +#define ATTRIBUTE_BIN_ATTR_RW(_name, _size) \
>> +struct bin_attribute _name##_bin_attr = __BIN_ATTR_RW(_name, _size); \
>> +__ATTRIBUTE_BIN_ATTRS(_name)
These I guess are the equivialent what ATTRIBUTE_GROUP is for groups,
but now for the attributes that go in groups?
>
> Can you show me how these would be used in a real-world example?
Well my real world is currently limited by my own driver. If I may copy
paste from there:
ATTRIBUTE_BIN_ATTR_RO(sunxi_sid, SID_SIZE);
ATTRIBUTE_BIN_GROUPS(sunxi_sid);
static struct platform_driver sunxi_sid_driver = {
.probe = sunxi_sid_probe,
.remove = sunxi_sid_remove,
.driver = {
.name = DRV_NAME,
.owner = THIS_MODULE,
.of_match_table = sunxi_sid_of_match,
.groups = sunxi_sid_bin_groups,
},
};
module_platform_driver(sunxi_sid_driver);
But if you say, you want to be a little less complete, we can drop
ATTRIBUTE_BIN_ATTR_R[OW]() forcing you to do this instead:
struct bin_attribute sunxi_sid_bin_attr = __BIN_ATTR_RO(eeprom, SID_SIZE);
struct bin_attribute *sunxi_sid_bin_attrs[] = {
&sunxi_sid_bin_attr,
NULL,
};
Which requires some manual labor yet still has the __BIN_ATTR_R[OW]
macro's to help with some of the more tedious work and allowing you to
name the binary attributes nicer?
>
> thanks,
Sorry if I sound a little confusing, it made a lot of sense this morning :(
Oliver
>
> greg k-h
>
On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
> On 07/11/13 19:06, Greg Kroah-Hartman wrote:
> >On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
> >>On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> >>>To make it easier for driver subsystems to work with attribute groups,
> >>>create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
> >>>typing for the most common use for attribute groups.
> >>But binary groups are discriminated against :(
> >
> >Yes, as they are "rarer" by far, as they should be. binary sysfs files
> >should almost never be used, as they are only "pass-through" files to
> >the hardware, so I want to see you do more work in order to use them, as
> >they should not be created lightly.
> I guess I can see a valid reason here, but they are only helper
> macro's making life easier for people who do need to use these and
> are on par with the non-binary versions. And we already have quite
> some binary attributes, probably far less then normal ones :)
I only count about 100 valid binary files in the tree at the moment,
that's not really all that many to handle.
> Anyway, wouldn't all users be reviewed anyway? But I guess it's a
> small safety net to make it not TOO easy.
exactly :)
> >>The attached patch should help here.
> >
> >Can you give me an example of using these macros? I seem to be lost in
> >them somehow, or maybe my morning coffee just hasn't kicked in...
> Yeah, I kinda added the whole shebang there :) I was trying being helpful :(
>
> >
> >>I suppose one more additional helper wouldn't be bad to have:
> >>
> >>#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
> >>ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
> >>ATTRIBUTE_(BIN_)GROUPS(_name)
> >
> >Would that ever be needed?
> Of ourse, by the lazy :)
>
> I think now you create an attribute in a group as this (with this patch):
>
> ATTRIBUTE_ATTR_RO(name, SIZE);
but "raw" attributes are rare, you really want a "device", "class", or
"bus" attribute, right?
> ATTRIBUTE_GROUPS(name);
>
> .group = name;
>
> After that last addition, you'd simply do:
> ATTRIBUTE_GROUPS_RO(name);
>
> .group = name;
>
> saves a whole line :)
Not worth it :)
> >>>From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
> >>From: Oliver Schinagl <[email protected]>
> >>Date: Thu, 11 Jul 2013 13:48:18 +0200
> >>Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
> >>
> >>With the recent changes to sysfs there's various helper macro's.
> >>However there's no RW, RO BIN_ helper macro's. This patch adds them.
> >>
> >>Additionally there are no BIN_ group helpers so there's that aswell
> >>Moreso, if both bin and normal attribute groups are used, there's a
> >>simple helper for that, though the naming code be better. _TXT_ for the
> >>show/store ones and neither TXT or BIN for both, but that would change
> >>things to extensivly.
> >>
> >>Finally there's also helpers for ATTRIBUTE_ATTRS.
> >>
> >>After this patch, create default attributes can be as easy as:
> >>
> >>ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
> >>ATTRIBUTE_BIN_GROUPS(name);
> >>
> >>Signed-off-by: Oliver Schinagl <[email protected]>
> >>---
> >> include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
> >> 1 file changed, 84 insertions(+), 12 deletions(-)
> >>
> >>diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> >>index 2c3b6a3..0ebed11 100644
> >>--- a/include/linux/sysfs.h
> >>+++ b/include/linux/sysfs.h
> >>@@ -17,6 +17,7 @@
> >> #include <linux/list.h>
> >> #include <linux/lockdep.h>
> >> #include <linux/kobject_ns.h>
> >>+#include <linux/stat.h>
> >> #include <linux/atomic.h>
> >>
> >> struct kobject;
> >>@@ -94,15 +95,32 @@ struct attribute_group {
> >> #define __ATTR_IGNORE_LOCKDEP __ATTR
> >> #endif
> >>
> >>-#define ATTRIBUTE_GROUPS(name) \
> >>-static const struct attribute_group name##_group = { \
> >>- .attrs = name##_attrs, \
> >>+#define __ATTRIBUTE_GROUPS(_name) \
> >>+static const struct attribute_group *_name##_groups[] = { \
> >>+ &_name##_group, \
> >>+ NULL, \
> >>+}
> >>+
> >>+#define ATTRIBUTE_GROUPS(_name) \
> >>+static const struct attribute_group _name##_group = { \
> >>+ .attrs = _name##_attrs, \
> >> }; \
> >>-static const struct attribute_group *name##_groups[] = { \
> >>- &name##_group, \
> >>+__ATTRIBUTE_GROUPS(_name)
> >>+
> >>+#define __ATTRIBUTE_ATTRS(_name) \
> >>+struct bin_attribute *_name##_attrs[] = { \
> typo here, scrap bin_ copy paste fail!
>
> >>+ &_name##_attr, \
> >> NULL, \
> >> }
> >>
> >>+#define ATTRIBUTE_ATTR_RO(_name, _size) \
> >>+struct attribute _name##_attr = __ATTR_RO(_name, _size); \
> >>+__ATTRIBUTE_ATTRS(_name)
> >>+
> >>+#define ATTRIBUTE_ATTR_RW(_name, _size) \
> >>+struct attribute _name##_attr = __ATTR_RW(_name, _size); \
> >>+__ATTRIBUTE_ATTRS(_name)
> >
> >What do these two help out with? "attribute attribute read-write" seems
> >a bit "clunky", don't you think? :)
> I aggree, but I tried to stick with the ATTRIBUTE_GROUP naming and
> that's the best I could come up with.
>
> Unless I completely misunderstood (which isn't all that unlikely)
> the following is needed to create a group using a .group.
>
> So you pass group an array of attribute_group pointers. The
> ATTRIBUTE_GROUPS helps there, right? It saves the typing of creating
> the array of groups and adding groups to that.
>
> So a group consists of an array of attributes if I understood right
> and that array needs to be filled with pointers attributes? well
> those ATTRIBUTE_ATTR's do just that. Granted, maybe the naming is
> poor, but just ATTRS() felt to short.
Here's an example of a file I converted to use the ATTRIBUTE_GROUP()
macro attached below (net/wireless/sysfs). As is, it's an increase of
only 2 lines to the file overall, which is about normal for the
conversion. As you can see, you still need a list of attributes (which
someone has already said I need another macro for, to stop typing
"&dev_attr*.attr" all the time).
With your macros, how would a file be converted to use them? Perhaps
that will help explain things to me better.
> >>+#define ATTRIBUTE_BIN_GROUPS(_name) \
> >>+static const struct attribute_group _name##_bin_group = { \
> >>+ .bin_attrs = _name##_bin_attrs, \
> >>+}; \
> >>+__ATTRIBUTE_BIN_GROUPS(_name)
> This is the equiv. of ATTRIBUTE_GROUPS(_name) which creates an
> attribute group, with only a binary attribute instead.
Again, binary files are rare, and should be rare, don't make it too easy
to create them :)
> >>+#define ATTRIBUTE_BIN_ATTR_RW(_name, _size) \
> >>+struct bin_attribute _name##_bin_attr = __BIN_ATTR_RW(_name, _size); \
> >>+__ATTRIBUTE_BIN_ATTRS(_name)
> These I guess are the equivialent what ATTRIBUTE_GROUP is for
> groups, but now for the attributes that go in groups?
>
> >
> >Can you show me how these would be used in a real-world example?
> Well my real world is currently limited by my own driver. If I may
> copy paste from there:
>
> ATTRIBUTE_BIN_ATTR_RO(sunxi_sid, SID_SIZE);
> ATTRIBUTE_BIN_GROUPS(sunxi_sid);
>
> static struct platform_driver sunxi_sid_driver = {
> .probe = sunxi_sid_probe,
> .remove = sunxi_sid_remove,
> .driver = {
> .name = DRV_NAME,
> .owner = THIS_MODULE,
> .of_match_table = sunxi_sid_of_match,
> .groups = sunxi_sid_bin_groups,
> },
> };
> module_platform_driver(sunxi_sid_driver);
>
> But if you say, you want to be a little less complete, we can drop
> ATTRIBUTE_BIN_ATTR_R[OW]() forcing you to do this instead:
>
> struct bin_attribute sunxi_sid_bin_attr = __BIN_ATTR_RO(eeprom, SID_SIZE);
>
> struct bin_attribute *sunxi_sid_bin_attrs[] = {
> &sunxi_sid_bin_attr,
> NULL,
> };
>
> Which requires some manual labor yet still has the __BIN_ATTR_R[OW]
> macro's to help with some of the more tedious work and allowing you
> to name the binary attributes nicer?
BIN_ATTR_RO() is fine, I'd stick with that for now, it's getting
confusing enough as-is :)
thanks,
greg k-h
On 07/11/13 22:26, Greg Kroah-Hartman wrote:
> On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
>> On 07/11/13 19:06, Greg Kroah-Hartman wrote:
>>> On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
>>>> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
>>>>> To make it easier for driver subsystems to work with attribute groups,
>>>>> create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
>>>>> typing for the most common use for attribute groups.
>>>> But binary groups are discriminated against :(
>>>
>>> Yes, as they are "rarer" by far, as they should be. binary sysfs files
>>> should almost never be used, as they are only "pass-through" files to
>>> the hardware, so I want to see you do more work in order to use them, as
>>> they should not be created lightly.
>> I guess I can see a valid reason here, but they are only helper
>> macro's making life easier for people who do need to use these and
>> are on par with the non-binary versions. And we already have quite
>> some binary attributes, probably far less then normal ones :)
>
> I only count about 100 valid binary files in the tree at the moment,
> that's not really all that many to handle.
100 is quite a few :) But point taken.
>
>> Anyway, wouldn't all users be reviewed anyway? But I guess it's a
>> small safety net to make it not TOO easy.
>
> exactly :)
I aggree and this is a v2 that strips all the additional bits.
A few comments left below.
>
>>>> The attached patch should help here.
>>>
>>> Can you give me an example of using these macros? I seem to be lost in
>>> them somehow, or maybe my morning coffee just hasn't kicked in...
>> Yeah, I kinda added the whole shebang there :) I was trying being helpful :(
>>
>>>
>>>> I suppose one more additional helper wouldn't be bad to have:
>>>>
>>>> #define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
>>>> ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
>>>> ATTRIBUTE_(BIN_)GROUPS(_name)
>>>
>>> Would that ever be needed?
>> Of ourse, by the lazy :)
>>
>> I think now you create an attribute in a group as this (with this patch):
>>
>> ATTRIBUTE_ATTR_RO(name, SIZE);
>
> but "raw" attributes are rare, you really want a "device", "class", or
> "bus" attribute, right?
I suppose so, But I got stuck in the rare case some how initially. I
registered my driver with module_platform_driver(); and in that struct i
had the "device_driver" which had a group attribute so I used that. I
already learned from maxime that that is the wrong way :) and hopefully
I'll figure out what the right way will be soon ;)
>
>> ATTRIBUTE_GROUPS(name);
>>
>> .group = name;
>>
>> After that last addition, you'd simply do:
>> ATTRIBUTE_GROUPS_RO(name);
>>
>> .group = name;
>>
>> saves a whole line :)
>
> Not worth it :)
>
>>>> >From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
>>>> From: Oliver Schinagl <[email protected]>
>>>> Date: Thu, 11 Jul 2013 13:48:18 +0200
>>>> Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
>>>>
>>>> With the recent changes to sysfs there's various helper macro's.
>>>> However there's no RW, RO BIN_ helper macro's. This patch adds them.
>>>>
>>>> Additionally there are no BIN_ group helpers so there's that aswell
>>>> Moreso, if both bin and normal attribute groups are used, there's a
>>>> simple helper for that, though the naming code be better. _TXT_ for the
>>>> show/store ones and neither TXT or BIN for both, but that would change
>>>> things to extensivly.
>>>>
>>>> Finally there's also helpers for ATTRIBUTE_ATTRS.
>>>>
>>>> After this patch, create default attributes can be as easy as:
>>>>
>>>> ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
>>>> ATTRIBUTE_BIN_GROUPS(name);
>>>>
>>>> Signed-off-by: Oliver Schinagl <[email protected]>
>>>> ---
>>>> include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
>>>> 1 file changed, 84 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>>>> index 2c3b6a3..0ebed11 100644
>>>> --- a/include/linux/sysfs.h
>>>> +++ b/include/linux/sysfs.h
>>>> @@ -17,6 +17,7 @@
>>>> #include <linux/list.h>
>>>> #include <linux/lockdep.h>
>>>> #include <linux/kobject_ns.h>
>>>> +#include <linux/stat.h>
>>>> #include <linux/atomic.h>
>>>>
>>>> struct kobject;
>>>> @@ -94,15 +95,32 @@ struct attribute_group {
>>>> #define __ATTR_IGNORE_LOCKDEP __ATTR
>>>> #endif
>>>>
>>>> -#define ATTRIBUTE_GROUPS(name) \
>>>> -static const struct attribute_group name##_group = { \
>>>> - .attrs = name##_attrs, \
>>>> +#define __ATTRIBUTE_GROUPS(_name) \
>>>> +static const struct attribute_group *_name##_groups[] = { \
>>>> + &_name##_group, \
>>>> + NULL, \
>>>> +}
>>>> +
>>>> +#define ATTRIBUTE_GROUPS(_name) \
>>>> +static const struct attribute_group _name##_group = { \
>>>> + .attrs = _name##_attrs, \
>>>> }; \
>>>> -static const struct attribute_group *name##_groups[] = { \
>>>> - &name##_group, \
>>>> +__ATTRIBUTE_GROUPS(_name)
>>>> +
>>>> +#define __ATTRIBUTE_ATTRS(_name) \
>>>> +struct bin_attribute *_name##_attrs[] = { \
>> typo here, scrap bin_ copy paste fail!
>>
>>>> + &_name##_attr, \
>>>> NULL, \
>>>> }
>>>>
>>>> +#define ATTRIBUTE_ATTR_RO(_name, _size) \
>>>> +struct attribute _name##_attr = __ATTR_RO(_name, _size); \
>>>> +__ATTRIBUTE_ATTRS(_name)
>>>> +
>>>> +#define ATTRIBUTE_ATTR_RW(_name, _size) \
>>>> +struct attribute _name##_attr = __ATTR_RW(_name, _size); \
>>>> +__ATTRIBUTE_ATTRS(_name)
>>>
>>> What do these two help out with? "attribute attribute read-write" seems
>>> a bit "clunky", don't you think? :)
>> I aggree, but I tried to stick with the ATTRIBUTE_GROUP naming and
>> that's the best I could come up with.
>>
>> Unless I completely misunderstood (which isn't all that unlikely)
>> the following is needed to create a group using a .group.
>>
>> So you pass group an array of attribute_group pointers. The
>> ATTRIBUTE_GROUPS helps there, right? It saves the typing of creating
>> the array of groups and adding groups to that.
>>
>> So a group consists of an array of attributes if I understood right
>> and that array needs to be filled with pointers attributes? well
>> those ATTRIBUTE_ATTR's do just that. Granted, maybe the naming is
>> poor, but just ATTRS() felt to short.
>
> Here's an example of a file I converted to use the ATTRIBUTE_GROUP()
> macro attached below (net/wireless/sysfs). As is, it's an increase of
> only 2 lines to the file overall, which is about normal for the
> conversion. As you can see, you still need a list of attributes (which
> someone has already said I need another macro for, to stop typing
> "&dev_attr*.attr" all the time).
>
> With your macros, how would a file be converted to use them? Perhaps
> that will help explain things to me better.
Heh, they can't I don't think.
>
>
>>>> +#define ATTRIBUTE_BIN_GROUPS(_name) \
>>>> +static const struct attribute_group _name##_bin_group = { \
>>>> + .bin_attrs = _name##_bin_attrs, \
>>>> +}; \
>>>> +__ATTRIBUTE_BIN_GROUPS(_name)
>> This is the equiv. of ATTRIBUTE_GROUPS(_name) which creates an
>> attribute group, with only a binary attribute instead.
>
> Again, binary files are rare, and should be rare, don't make it too easy
> to create them :)
>
>>>> +#define ATTRIBUTE_BIN_ATTR_RW(_name, _size) \
>>>> +struct bin_attribute _name##_bin_attr = __BIN_ATTR_RW(_name, _size); \
>>>> +__ATTRIBUTE_BIN_ATTRS(_name)
>> These I guess are the equivialent what ATTRIBUTE_GROUP is for
>> groups, but now for the attributes that go in groups?
>>
>>>
>>> Can you show me how these would be used in a real-world example?
>> Well my real world is currently limited by my own driver. If I may
>> copy paste from there:
>>
>> ATTRIBUTE_BIN_ATTR_RO(sunxi_sid, SID_SIZE);
>> ATTRIBUTE_BIN_GROUPS(sunxi_sid);
>>
>> static struct platform_driver sunxi_sid_driver = {
>> .probe = sunxi_sid_probe,
>> .remove = sunxi_sid_remove,
>> .driver = {
>> .name = DRV_NAME,
>> .owner = THIS_MODULE,
>> .of_match_table = sunxi_sid_of_match,
>> .groups = sunxi_sid_bin_groups,
>> },
>> };
>> module_platform_driver(sunxi_sid_driver);
>>
>> But if you say, you want to be a little less complete, we can drop
>> ATTRIBUTE_BIN_ATTR_R[OW]() forcing you to do this instead:
>>
>> struct bin_attribute sunxi_sid_bin_attr = __BIN_ATTR_RO(eeprom, SID_SIZE);
>>
>> struct bin_attribute *sunxi_sid_bin_attrs[] = {
>> &sunxi_sid_bin_attr,
>> NULL,
>> };
>>
>> Which requires some manual labor yet still has the __BIN_ATTR_R[OW]
>> macro's to help with some of the more tedious work and allowing you
>> to name the binary attributes nicer?
>
> BIN_ATTR_RO() is fine, I'd stick with that for now, it's getting
> confusing enough as-is :)
Agreed and attached.
>
> thanks,
>
> greg k-h
>
On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> Hi all,
>
> Here's the second iteration of the patchset to add better attribute
> group support to the driver core and sysfs.
>
> I've tested these (shocker!) and everything works fine with them (I'm
> sending this from Linus's latest kernel with these 7 on top of it.)
>
> I'd like to send these to Linus for 3.11 unless someone objects.
>
Hi Greg,
I have not seen the series in the upstream kernel. Do you have an updated
plan to get it upstream ?
If needed I can carry the patches I need in my branch for now, so it doesn't
matter too much if the code misses 3.11. Still, it would help to know the
plan going forward.
Thanks,
Guenter
On Sun, Jul 14, 2013 at 10:27:03AM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> > Hi all,
> >
> > Here's the second iteration of the patchset to add better attribute
> > group support to the driver core and sysfs.
> >
> > I've tested these (shocker!) and everything works fine with them (I'm
> > sending this from Linus's latest kernel with these 7 on top of it.)
> >
> > I'd like to send these to Linus for 3.11 unless someone objects.
> >
> Hi Greg,
>
> I have not seen the series in the upstream kernel. Do you have an updated
> plan to get it upstream ?
>
> If needed I can carry the patches I need in my branch for now, so it doesn't
> matter too much if the code misses 3.11. Still, it would help to know the
> plan going forward.
I was planning on doing one more respin of the patches on Monday, and
then getting them into 3.11-rc2 if possible so that everyone can start
taking advantage of them.
Got sucked into the "discussion" of the stable kernels on Friday and
didn't do the update like I was wanting to do so :)
thanks,
greg k-h
On Sun, Jul 14, 2013 at 10:32:58AM -0700, Greg Kroah-Hartman wrote:
> On Sun, Jul 14, 2013 at 10:27:03AM -0700, Guenter Roeck wrote:
> > On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> > > Hi all,
> > >
> > > Here's the second iteration of the patchset to add better attribute
> > > group support to the driver core and sysfs.
> > >
> > > I've tested these (shocker!) and everything works fine with them (I'm
> > > sending this from Linus's latest kernel with these 7 on top of it.)
> > >
> > > I'd like to send these to Linus for 3.11 unless someone objects.
> > >
> > Hi Greg,
> >
> > I have not seen the series in the upstream kernel. Do you have an updated
> > plan to get it upstream ?
> >
> > If needed I can carry the patches I need in my branch for now, so it doesn't
> > matter too much if the code misses 3.11. Still, it would help to know the
> > plan going forward.
>
> I was planning on doing one more respin of the patches on Monday, and
> then getting them into 3.11-rc2 if possible so that everyone can start
> taking advantage of them.
>
Sounds good.
> Got sucked into the "discussion" of the stable kernels on Friday and
> didn't do the update like I was wanting to do so :)
>
In this context ... you are doing an amazing job with -stable.
I have no idea how you manage to do that, especially with all your other
responsibilities. If anything, it is unfair to talk about an allegedly
"broken" process if so much of it depends on a single person.
stable_kernel_rules talks about a review committee. Unless I am missing
something, that committee consists of one person. Maybe it would help
to add a couple more. Good candidates might be the people who are
complaining about a broken process ;).
Guenter
Greg,
Haven't heard anything about this v2 patch, I know you are very busy but
with the merge window closing today/yesterday just wondering about the
status of it all.
Oliver
On 11-07-13 22:55, Oliver Schinagl wrote:
> On 07/11/13 22:26, Greg Kroah-Hartman wrote:
>> On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
>>> On 07/11/13 19:06, Greg Kroah-Hartman wrote:
>>>> On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
>>>>> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
>>>>>> To make it easier for driver subsystems to work with attribute
>>>>>> groups,
>>>>>> create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
>>>>>> typing for the most common use for attribute groups.
>>>>> But binary groups are discriminated against :(
>>>>
>>>> Yes, as they are "rarer" by far, as they should be. binary sysfs
>>>> files
>>>> should almost never be used, as they are only "pass-through" files to
>>>> the hardware, so I want to see you do more work in order to use
>>>> them, as
>>>> they should not be created lightly.
>>> I guess I can see a valid reason here, but they are only helper
>>> macro's making life easier for people who do need to use these and
>>> are on par with the non-binary versions. And we already have quite
>>> some binary attributes, probably far less then normal ones :)
>>
>> I only count about 100 valid binary files in the tree at the moment,
>> that's not really all that many to handle.
> 100 is quite a few :) But point taken.
>>
>>> Anyway, wouldn't all users be reviewed anyway? But I guess it's a
>>> small safety net to make it not TOO easy.
>>
>> exactly :)
> I aggree and this is a v2 that strips all the additional bits.
>
> A few comments left below.
>
>>
>>>>> The attached patch should help here.
>>>>
>>>> Can you give me an example of using these macros? I seem to be
>>>> lost in
>>>> them somehow, or maybe my morning coffee just hasn't kicked in...
>>> Yeah, I kinda added the whole shebang there :) I was trying being
>>> helpful :(
>>>
>>>>
>>>>> I suppose one more additional helper wouldn't be bad to have:
>>>>>
>>>>> #define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
>>>>> ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
>>>>> ATTRIBUTE_(BIN_)GROUPS(_name)
>>>>
>>>> Would that ever be needed?
>>> Of ourse, by the lazy :)
>>>
>>> I think now you create an attribute in a group as this (with this
>>> patch):
>>>
>>> ATTRIBUTE_ATTR_RO(name, SIZE);
>>
>> but "raw" attributes are rare, you really want a "device", "class", or
>> "bus" attribute, right?
> I suppose so, But I got stuck in the rare case some how initially. I
> registered my driver with module_platform_driver(); and in that struct
> i had the "device_driver" which had a group attribute so I used that.
> I already learned from maxime that that is the wrong way :) and
> hopefully I'll figure out what the right way will be soon ;)
>
>>
>>> ATTRIBUTE_GROUPS(name);
>>>
>>> .group = name;
>>>
>>> After that last addition, you'd simply do:
>>> ATTRIBUTE_GROUPS_RO(name);
>>>
>>> .group = name;
>>>
>>> saves a whole line :)
>>
>> Not worth it :)
>>
>>>>> >From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00
>>>>> 2001
>>>>> From: Oliver Schinagl <[email protected]>
>>>>> Date: Thu, 11 Jul 2013 13:48:18 +0200
>>>>> Subject: [PATCH] sysfs: add more helper macro's for
>>>>> (bin_)attribute(_groups)
>>>>>
>>>>> With the recent changes to sysfs there's various helper macro's.
>>>>> However there's no RW, RO BIN_ helper macro's. This patch adds them.
>>>>>
>>>>> Additionally there are no BIN_ group helpers so there's that aswell
>>>>> Moreso, if both bin and normal attribute groups are used, there's a
>>>>> simple helper for that, though the naming code be better. _TXT_
>>>>> for the
>>>>> show/store ones and neither TXT or BIN for both, but that would
>>>>> change
>>>>> things to extensivly.
>>>>>
>>>>> Finally there's also helpers for ATTRIBUTE_ATTRS.
>>>>>
>>>>> After this patch, create default attributes can be as easy as:
>>>>>
>>>>> ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
>>>>> ATTRIBUTE_BIN_GROUPS(name);
>>>>>
>>>>> Signed-off-by: Oliver Schinagl <[email protected]>
>>>>> ---
>>>>> include/linux/sysfs.h | 96
>>>>> ++++++++++++++++++++++++++++++++++++++++++++-------
>>>>> 1 file changed, 84 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>>>>> index 2c3b6a3..0ebed11 100644
>>>>> --- a/include/linux/sysfs.h
>>>>> +++ b/include/linux/sysfs.h
>>>>> @@ -17,6 +17,7 @@
>>>>> #include <linux/list.h>
>>>>> #include <linux/lockdep.h>
>>>>> #include <linux/kobject_ns.h>
>>>>> +#include <linux/stat.h>
>>>>> #include <linux/atomic.h>
>>>>>
>>>>> struct kobject;
>>>>> @@ -94,15 +95,32 @@ struct attribute_group {
>>>>> #define __ATTR_IGNORE_LOCKDEP __ATTR
>>>>> #endif
>>>>>
>>>>> -#define ATTRIBUTE_GROUPS(name) \
>>>>> -static const struct attribute_group name##_group = { \
>>>>> - .attrs = name##_attrs, \
>>>>> +#define __ATTRIBUTE_GROUPS(_name) \
>>>>> +static const struct attribute_group *_name##_groups[] = { \
>>>>> + &_name##_group, \
>>>>> + NULL, \
>>>>> +}
>>>>> +
>>>>> +#define ATTRIBUTE_GROUPS(_name) \
>>>>> +static const struct attribute_group _name##_group = { \
>>>>> + .attrs = _name##_attrs, \
>>>>> }; \
>>>>> -static const struct attribute_group *name##_groups[] = { \
>>>>> - &name##_group, \
>>>>> +__ATTRIBUTE_GROUPS(_name)
>>>>> +
>>>>> +#define __ATTRIBUTE_ATTRS(_name) \
>>>>> +struct bin_attribute *_name##_attrs[] = { \
>>> typo here, scrap bin_ copy paste fail!
>>>
>>>>> + &_name##_attr, \
>>>>> NULL, \
>>>>> }
>>>>>
>>>>> +#define ATTRIBUTE_ATTR_RO(_name, _size) \
>>>>> +struct attribute _name##_attr = __ATTR_RO(_name, _size); \
>>>>> +__ATTRIBUTE_ATTRS(_name)
>>>>> +
>>>>> +#define ATTRIBUTE_ATTR_RW(_name, _size) \
>>>>> +struct attribute _name##_attr = __ATTR_RW(_name, _size); \
>>>>> +__ATTRIBUTE_ATTRS(_name)
>>>>
>>>> What do these two help out with? "attribute attribute read-write"
>>>> seems
>>>> a bit "clunky", don't you think? :)
>>> I aggree, but I tried to stick with the ATTRIBUTE_GROUP naming and
>>> that's the best I could come up with.
>>>
>>> Unless I completely misunderstood (which isn't all that unlikely)
>>> the following is needed to create a group using a .group.
>>>
>>> So you pass group an array of attribute_group pointers. The
>>> ATTRIBUTE_GROUPS helps there, right? It saves the typing of creating
>>> the array of groups and adding groups to that.
>>>
>>> So a group consists of an array of attributes if I understood right
>>> and that array needs to be filled with pointers attributes? well
>>> those ATTRIBUTE_ATTR's do just that. Granted, maybe the naming is
>>> poor, but just ATTRS() felt to short.
>>
>> Here's an example of a file I converted to use the ATTRIBUTE_GROUP()
>> macro attached below (net/wireless/sysfs). As is, it's an increase of
>> only 2 lines to the file overall, which is about normal for the
>> conversion. As you can see, you still need a list of attributes (which
>> someone has already said I need another macro for, to stop typing
>> "&dev_attr*.attr" all the time).
>>
>> With your macros, how would a file be converted to use them? Perhaps
>> that will help explain things to me better.
> Heh, they can't I don't think.
>
>>
>>
>>>>> +#define ATTRIBUTE_BIN_GROUPS(_name) \
>>>>> +static const struct attribute_group _name##_bin_group = { \
>>>>> + .bin_attrs = _name##_bin_attrs, \
>>>>> +}; \
>>>>> +__ATTRIBUTE_BIN_GROUPS(_name)
>>> This is the equiv. of ATTRIBUTE_GROUPS(_name) which creates an
>>> attribute group, with only a binary attribute instead.
>>
>> Again, binary files are rare, and should be rare, don't make it too easy
>> to create them :)
>>
>>>>> +#define ATTRIBUTE_BIN_ATTR_RW(_name, _size) \
>>>>> +struct bin_attribute _name##_bin_attr = __BIN_ATTR_RW(_name,
>>>>> _size); \
>>>>> +__ATTRIBUTE_BIN_ATTRS(_name)
>>> These I guess are the equivialent what ATTRIBUTE_GROUP is for
>>> groups, but now for the attributes that go in groups?
>>>
>>>>
>>>> Can you show me how these would be used in a real-world example?
>>> Well my real world is currently limited by my own driver. If I may
>>> copy paste from there:
>>>
>>> ATTRIBUTE_BIN_ATTR_RO(sunxi_sid, SID_SIZE);
>>> ATTRIBUTE_BIN_GROUPS(sunxi_sid);
>>>
>>> static struct platform_driver sunxi_sid_driver = {
>>> .probe = sunxi_sid_probe,
>>> .remove = sunxi_sid_remove,
>>> .driver = {
>>> .name = DRV_NAME,
>>> .owner = THIS_MODULE,
>>> .of_match_table = sunxi_sid_of_match,
>>> .groups = sunxi_sid_bin_groups,
>>> },
>>> };
>>> module_platform_driver(sunxi_sid_driver);
>>>
>>> But if you say, you want to be a little less complete, we can drop
>>> ATTRIBUTE_BIN_ATTR_R[OW]() forcing you to do this instead:
>>>
>>> struct bin_attribute sunxi_sid_bin_attr = __BIN_ATTR_RO(eeprom,
>>> SID_SIZE);
>>>
>>> struct bin_attribute *sunxi_sid_bin_attrs[] = {
>>> &sunxi_sid_bin_attr,
>>> NULL,
>>> };
>>>
>>> Which requires some manual labor yet still has the __BIN_ATTR_R[OW]
>>> macro's to help with some of the more tedious work and allowing you
>>> to name the binary attributes nicer?
>>
>> BIN_ATTR_RO() is fine, I'd stick with that for now, it's getting
>> confusing enough as-is :)
> Agreed and attached.
>
>>
>> thanks,
>>
>> greg k-h
>>
>
On Mon, Jul 15, 2013 at 12:55:22AM +0200, Oliver Schinagl wrote:
> Greg,
>
> Haven't heard anything about this v2 patch, I know you are very busy
> but with the merge window closing today/yesterday just wondering
> about the status of it all.
Just refreshed them against 3.11-rc1 and sent them out a mere seconds
ago :)
thanks,
greg k-h