staging: comedi: register sysfs device attributes with driver core

PATCH In review H Hartley Sweeten 4 participants 5 messages May 11, 2012 – May 14, 2012 Staging

Share & tools

H Hartley Sweeten

author

2012-05-11 01:05 UTC #0 lore raw

PATCH 1/1 staging: comedi: register sysfs device attributes with driver core

Currently the sysfs device attributes are created by the comedi core after each comedi device is created. This can lead to a race condition where userspace gets an add event before the files are created.

Register the device attributes with the comedi class so that the driver core handles creating them and we avoid the race.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Ian Abbott <[email protected]>

Cc Mori Hess <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---

drivers/staging/comedi/comedi_fops.c +12 ~~-40~~

index 44ca1fe..7677657 100644
@@ -227,9 +227,6 @@ static ssize_t store_max_read_buffer_kb(struct device *dev,
 	return count;
 }
 
-static DEVICE_ATTR(max_read_buffer_kb, S_IRUGO | S_IWUSR,
-		show_max_read_buffer_kb, store_max_read_buffer_kb);
-
 static ssize_t show_read_buffer_kb(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {

@@ -287,9 +284,6 @@ static ssize_t store_read_buffer_kb(struct device *dev,
 	return count;
 }
 
-static DEVICE_ATTR(read_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
-		show_read_buffer_kb, store_read_buffer_kb);
-
 static ssize_t show_max_write_buffer_kb(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)

@@ -344,9 +338,6 @@ static ssize_t store_max_write_buffer_kb(struct device *dev,
 	return count;
 }
 
-static DEVICE_ATTR(max_write_buffer_kb, S_IRUGO | S_IWUSR,
-		show_max_write_buffer_kb, store_max_write_buffer_kb);
-
 static ssize_t show_write_buffer_kb(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {

@@ -404,19 +395,16 @@ static ssize_t store_write_buffer_kb(struct device *dev,
 	return count;
 }
 
-static DEVICE_ATTR(write_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
-		show_write_buffer_kb, store_write_buffer_kb);
-
-static struct attribute *comedi_attrs[] = {
-	&dev_attr_max_read_buffer_kb.attr,
-	&dev_attr_read_buffer_kb.attr,
-	&dev_attr_max_write_buffer_kb.attr,
-	&dev_attr_write_buffer_kb.attr,
-	NULL
-};
-
-static const struct attribute_group comedi_sysfs_files = {
-	.attrs	= comedi_attrs,
+static struct device_attribute comedi_dev_attrs[] = {
+	__ATTR(max_read_buffer_kb, S_IRUGO | S_IWUSR,
+		show_max_read_buffer_kb, store_max_read_buffer_kb),
+	__ATTR(read_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
+		show_read_buffer_kb, store_read_buffer_kb),
+	__ATTR(max_write_buffer_kb, S_IRUGO | S_IWUSR,
+		show_max_write_buffer_kb, store_max_write_buffer_kb),
+	__ATTR(write_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
+		show_write_buffer_kb, store_write_buffer_kb),
+	__ATTR_NULL
 };
 
 static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd,

@@ -2355,6 +2343,8 @@ static int __init comedi_init(void)
 		return PTR_ERR(comedi_class);
 	}
 
+	comedi_class->dev_attrs = comedi_dev_attrs;
+
 	/* XXX requires /proc interface */
 	comedi_proc_init();

@@ -2496,7 +2486,6 @@ int comedi_alloc_board_minor(struct device *hardware_device)
 	struct comedi_device_file_info *info;
 	struct device *csdev;
 	unsigned i;
-	int retval;
 
 	info = kzalloc(sizeof(struct comedi_device_file_info), GFP_KERNEL);
 	if (info == NULL)

@@ -2532,14 +2521,6 @@ int comedi_alloc_board_minor(struct device *hardware_device)
 		info->device->class_dev = csdev;
 	dev_set_drvdata(csdev, info);
 
-	retval = sysfs_create_group(&csdev->kobj, &comedi_sysfs_files);
-	if (retval) {
-		printk(KERN_ERR
-		       "comedi: failed to create sysfs attribute files\n");
-		comedi_free_board_minor(i);
-		return retval;
-	}
-
 	return i;
 }

@@ -2590,7 +2571,6 @@ int comedi_alloc_subdevice_minor(struct comedi_device *dev,
 	struct comedi_device_file_info *info;
 	struct device *csdev;
 	unsigned i;
-	int retval;
 
 	info = kmalloc(sizeof(struct comedi_device_file_info), GFP_KERNEL);
 	if (info == NULL)

@@ -2621,14 +2601,6 @@ int comedi_alloc_subdevice_minor(struct comedi_device *dev,
 		s->class_dev = csdev;
 	dev_set_drvdata(csdev, info);
 
-	retval = sysfs_create_group(&csdev->kobj, &comedi_sysfs_files);
-	if (retval) {
-		printk(KERN_ERR
-		       "comedi: failed to create sysfs attribute files\n");
-		comedi_free_subdevice_minor(s);
-		return retval;
-	}
-
 	return i;
 }

Dan Carpenter

2012-05-11 06:13 UTC #1 lore raw

drivers/staging/comedi/comedi_fops.c static ssize_t store_write_buffer_kb(struct device *dev, · line 407

Quoted 12 lines · H Hartley Sweeten

drivers/staging/comedi/comedi_fops.c +10 -2

-static const struct attribute_group comedi_sysfs_files = {
-	.attrs	= comedi_attrs,
+static struct device_attribute comedi_dev_attrs[] = {
+	__ATTR(max_read_buffer_kb, S_IRUGO | S_IWUSR,
+		show_max_read_buffer_kb, store_max_read_buffer_kb),
+	__ATTR(read_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
+		show_read_buffer_kb, store_read_buffer_kb),
+	__ATTR(max_write_buffer_kb, S_IRUGO | S_IWUSR,
+		show_max_write_buffer_kb, store_max_write_buffer_kb),
+	__ATTR(write_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
+		show_write_buffer_kb, store_write_buffer_kb),
+	__ATTR_NULL

Some of these are group writable and some are only user writable so it's not consistent. Probably just make them user writeable.

I guess this was in the original code too, but it's just more obvious now that they're grouped together.

regards,
dan carpenter

Ian Abbott

2012-05-11 13:14 UTC #2 lore raw

Quoted 17 lines · Dan Carpenter

drivers/staging/comedi/comedi_fops.c static ssize_t store_write_buffer_kb(struct device *dev, · line 407
Quoted 12 lines · H Hartley Sweeten
drivers/staging/comedi/comedi_fops.c +10 -2
-static const struct attribute_group comedi_sysfs_files = {
-	.attrs	= comedi_attrs,
+static struct device_attribute comedi_dev_attrs[] = {
+	__ATTR(max_read_buffer_kb, S_IRUGO | S_IWUSR,
+		show_max_read_buffer_kb, store_max_read_buffer_kb),
+	__ATTR(read_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
+		show_read_buffer_kb, store_read_buffer_kb),
+	__ATTR(max_write_buffer_kb, S_IRUGO | S_IWUSR,
+		show_max_write_buffer_kb, store_max_write_buffer_kb),
+	__ATTR(write_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
+		show_write_buffer_kb, store_write_buffer_kb),
+	__ATTR_NULL
Some of these are group writable and some are only user writable so it's not consistent. Probably just make them user writeable.

Quoted 2 lines

I guess this was in the original code too, but it's just more obvious now that they're grouped together.

Is it possible to change (group) ownership of these attribute files using udev rules? Ideally, users in the group that has group ownership on the /dev/comedi* files should be able to modify the read_buffer_kb and write_buffer_kb attribute files as well, but only super-users should be able to modify the max_read_buffer_kb and max_write_buffer_kb files.

Signature · 2 lines

-=( Ian Abbott @ MEV Ltd.    E-mail: <[email protected]>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Dan Carpenter

2012-05-11 18:15 UTC #3 lore raw

Quoted 21 lines · Ian Abbott

Quoted 17 lines · Dan Carpenter
drivers/staging/comedi/comedi_fops.c static ssize_t store_write_buffer_kb(struct device *dev, · line 407
Quoted 12 lines · H Hartley Sweeten
drivers/staging/comedi/comedi_fops.c +10 -2
-static const struct attribute_group comedi_sysfs_files = {
-	.attrs	= comedi_attrs,
+static struct device_attribute comedi_dev_attrs[] = {
+	__ATTR(max_read_buffer_kb, S_IRUGO | S_IWUSR,
+		show_max_read_buffer_kb, store_max_read_buffer_kb),
+	__ATTR(read_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
+		show_read_buffer_kb, store_read_buffer_kb),
+	__ATTR(max_write_buffer_kb, S_IRUGO | S_IWUSR,
+		show_max_write_buffer_kb, store_max_write_buffer_kb),
+	__ATTR(write_buffer_kb, S_IRUGO | S_IWUSR | S_IWGRP,
+		show_write_buffer_kb, store_write_buffer_kb),
+	__ATTR_NULL
Some of these are group writable and some are only user writable so it's not consistent. Probably just make them user writeable.
Quoted 2 lines
I guess this was in the original code too, but it's just more obvious now that they're grouped together.

Quoted 7 lines

Is it possible to change (group) ownership of these attribute files using udev rules? Ideally, users in the group that has group ownership on the /dev/comedi* files should be able to modify the read_buffer_kb and write_buffer_kb attribute files as well, but only super-users should be able to modify the max_read_buffer_kb and max_write_buffer_kb files.

Oh. Ok that's fine then. I didn't understand what was going on.

regards,
dan carpenter

Greg KH

maintainer

2012-05-14 20:00 UTC #4 lore raw

In reply to H Hartley Sweeten

Quoted 7 lines · H Hartley Sweeten

Currently the sysfs device attributes are created by the comedi core after each comedi device is created. This can lead to a race condition where userspace gets an add event before the files are created.
Register the device attributes with the comedi class so that the driver core handles creating them and we avoid the race.

Nice job, thanks for making this change.

greg k-h