Skip to contentSkip to conversation

read/write on attribute w/o show/store should return -ENOSYS

PATCH In review Dmitry Torokhov 6 participants 18 messages Apr 28, 2005 – Apr 29, 2005

Subsystems (9)
PowerPC Block layer ACPI PCI +5 more subsystems
lore
Share & tools
lore

Dmitry Torokhov

author
#0 lore raw

cover read/write on attribute w/o show/store should return -ENOSYS

Hi,

Jean Delvare has noticed that if a driver happens to declare its attribute as RW but doesn't provide store() method attempt to write into such attribute will cause spinning process as most of the attribute implementations return 0 in case of missing store causing endless retries. In some cases missing show/store will return -EPERM, -EACCESS or -EINVAL.

I think we should unify implementations and have them all return -ENOSYS (function not implemented) when corresponding method (show/store) is missing.

Signature · 1 line
Dmitry
author
#1 lore raw

PATCH 1/5 sysfs: if show/store is missing return -ENOSYS

sysfs: if attribute does not implement show or store method
read/write should return -ENOSYS instead of 0, EACCESS or EINVAL.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 bin.c  |    4 ++--
 file.c |   19 ++++++++++++-------
 2 files changed, 14 insertions(+), 9 deletions(-)
fs/sysfs/file.c +12 -7
Index: dtor/fs/sysfs/file.c
===================================================================
@@ -23,7 +23,7 @@ subsys_attr_show(struct kobject * kobj, 
 {
 	struct subsystem * s = to_subsys(kobj);
 	struct subsys_attribute * sattr = to_sattr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (sattr->show)
 		ret = sattr->show(s,page);

@@ -36,7 +36,7 @@ subsys_attr_store(struct kobject * kobj,
 {
 	struct subsystem * s = to_subsys(kobj);
 	struct subsys_attribute * sattr = to_sattr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (sattr->store)
 		ret = sattr->store(s,page,count);

@@ -274,17 +274,17 @@ static int check_perm(struct inode * ino
 	 * or the subsystem have no operations.
 	 */
 	if (!ops)
-		goto Eaccess;
+		goto Enosys;
 
 	/* File needs write support.
 	 * The inode's perms must say it's ok, 
 	 * and we must have a store method.
 	 */
 	if (file->f_mode & FMODE_WRITE) {
-
-		if (!(inode->i_mode & S_IWUGO) || !ops->store)
+		if (!(inode->i_mode & S_IWUGO))
 			goto Eaccess;
-
+		if (!ops->store)
+			goto Enosys;
 	}
 
 	/* File needs read support.

@@ -292,8 +292,10 @@ static int check_perm(struct inode * ino
 	 * must be a show method for it.
 	 */
 	if (file->f_mode & FMODE_READ) {
-		if (!(inode->i_mode & S_IRUGO) || !ops->show)
+		if (!(inode->i_mode & S_IRUGO))
 			goto Eaccess;
+		if (!ops->show)
+			goto Enosys;
 	}
 
 	/* No error? Great, allocate a buffer for the file, and store it

@@ -313,6 +315,9 @@ static int check_perm(struct inode * ino
  Einval:
 	error = -EINVAL;
 	goto Done;
+ Enosys:
+	error = -ENOSYS;
+	goto Done;
  Eaccess:
 	error = -EACCES;
 	module_put(attr->owner);
fs/sysfs/bin.c +2 -2
Index: dtor/fs/sysfs/bin.c
===================================================================
@@ -25,7 +25,7 @@ fill_read(struct dentry *dentry, char *b
 	struct kobject * kobj = to_kobj(dentry->d_parent);
 
 	if (!attr->read)
-		return -EINVAL;
+		return -ENOSYS;
 
 	return attr->read(kobj, buffer, off, count);
 }

@@ -71,7 +71,7 @@ flush_write(struct dentry *dentry, char 
 	struct kobject *kobj = to_kobj(dentry->d_parent);
 
 	if (!attr->write)
-		return -EINVAL;
+		return -ENOSYS;
 
 	return attr->write(kobj, buffer, offset, count);
 }
author
#2 lore raw

PATCH 2/5 sysfs: (driver/base) if show/store is missing return -ENOSYS

In reply to Dmitry Torokhov

sysfs: fix drivers/base so if an attribute doesn't implement
show or store method read/write will return -ENOSYS instead of 0.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 bus.c   |    4 ++--
 class.c |    4 ++--
 core.c  |    4 ++--
 sys.c   |    4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)
drivers/base/class.c +2 -2
Index: dtor/drivers/base/class.c
===================================================================
@@ -26,7 +26,7 @@ class_attr_show(struct kobject * kobj, s
 {
 	struct class_attribute * class_attr = to_class_attr(attr);
 	struct class * dc = to_class(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (class_attr->show)
 		ret = class_attr->show(dc, buf);

@@ -39,7 +39,7 @@ class_attr_store(struct kobject * kobj, 
 {
 	struct class_attribute * class_attr = to_class_attr(attr);
 	struct class * dc = to_class(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (class_attr->store)
 		ret = class_attr->store(dc, buf, count);
drivers/base/sys.c +2 -2
Index: dtor/drivers/base/sys.c
===================================================================
@@ -37,7 +37,7 @@ sysdev_show(struct kobject * kobj, struc
 
 	if (sysdev_attr->show)
 		return sysdev_attr->show(sysdev, buffer);
-	return 0;
+	return -ENOSYS;
 }
 
 

@@ -50,7 +50,7 @@ sysdev_store(struct kobject * kobj, stru
 
 	if (sysdev_attr->store)
 		return sysdev_attr->store(sysdev, buffer, count);
-	return 0;
+	return -ENOSYS;
 }
 
 static struct sysfs_ops sysfs_ops = {
drivers/base/bus.c +2 -2
Index: dtor/drivers/base/bus.c
===================================================================
@@ -36,7 +36,7 @@ drv_attr_show(struct kobject * kobj, str
 {
 	struct driver_attribute * drv_attr = to_drv_attr(attr);
 	struct device_driver * drv = to_driver(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (drv_attr->show)
 		ret = drv_attr->show(drv, buf);

@@ -49,7 +49,7 @@ drv_attr_store(struct kobject * kobj, st
 {
 	struct driver_attribute * drv_attr = to_drv_attr(attr);
 	struct device_driver * drv = to_driver(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (drv_attr->store)
 		ret = drv_attr->store(drv, buf, count);
drivers/base/core.c +2 -2
Index: dtor/drivers/base/core.c
===================================================================
@@ -38,7 +38,7 @@ dev_attr_show(struct kobject * kobj, str
 {
 	struct device_attribute * dev_attr = to_dev_attr(attr);
 	struct device * dev = to_dev(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (dev_attr->show)
 		ret = dev_attr->show(dev, buf);

@@ -51,7 +51,7 @@ dev_attr_store(struct kobject * kobj, st
 {
 	struct device_attribute * dev_attr = to_dev_attr(attr);
 	struct device * dev = to_dev(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (dev_attr->store)
 		ret = dev_attr->store(dev, buf, count);
author
#3 lore raw

PATCH 3/5 sysfs: (driver/pci) if show/store is missing return -ENOSYS

In reply to Dmitry Torokhov

sysfs: fix drivers/pci so if an attribute does not implement
show or store method read/write will return -ENOSYS instead of 0.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 hotplug/pci_hotplug_core.c |    4 ++--
 hotplug/rpadlpar_sysfs.c   |    2 +-
 pci-driver.c               |   26 ++++++++++++++------------
 3 files changed, 17 insertions(+), 15 deletions(-)
drivers/pci/hotplug/rpadlpar_sysfs.c +1 -1
Index: dtor/drivers/pci/hotplug/rpadlpar_sysfs.c
===================================================================
@@ -48,7 +48,7 @@ dlpar_attr_store(struct kobject * kobj, 
 	struct dlpar_io_attr *dlpar_attr = container_of(attr,
 						struct dlpar_io_attr, attr);
 	return dlpar_attr->store ?
-		dlpar_attr->store(dlpar_attr, buf, nbytes) : 0;
+		dlpar_attr->store(dlpar_attr, buf, nbytes) : -ENOSYS;
 }
 
 static struct sysfs_ops dlpar_attr_sysfs_ops = {
drivers/pci/pci-driver.c +14 -12
Index: dtor/drivers/pci/pci-driver.c
===================================================================
@@ -327,13 +327,14 @@ pci_driver_attr_show(struct kobject * ko
 {
 	struct device_driver *driver = kobj_to_pci_driver(kobj);
 	struct driver_attribute *dattr = attr_to_driver_attribute(attr);
-	ssize_t ret = 0;
+	ssize_t ret;
 
-	if (get_driver(driver)) {
-		if (dattr->show)
-			ret = dattr->show(driver, buf);
-		put_driver(driver);
-	}
+	if (!get_driver(driver))
+		return -ENODEV;
+
+	ret = dattr->show ? dattr->show(driver, buf) : -ENOSYS;
+
+	put_driver(driver);
 	return ret;
 }
 

@@ -343,13 +344,14 @@ pci_driver_attr_store(struct kobject * k
 {
 	struct device_driver *driver = kobj_to_pci_driver(kobj);
 	struct driver_attribute *dattr = attr_to_driver_attribute(attr);
-	ssize_t ret = 0;
+	ssize_t ret;
 
-	if (get_driver(driver)) {
-		if (dattr->store)
-			ret = dattr->store(driver, buf, count);
-		put_driver(driver);
-	}
+	if (!get_driver(driver))
+		return -ENODEV;
+
+	ret = dattr->store ? dattr->store(driver, buf, count) : -ENOSYS;
+
+	put_driver(driver);
 	return ret;
 }
drivers/pci/hotplug/pci_hotplug_core.c +2 -2
Index: dtor/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================
@@ -73,7 +73,7 @@ static ssize_t hotplug_slot_attr_show(st
 {
 	struct hotplug_slot *slot = to_hotplug_slot(kobj);
 	struct hotplug_slot_attribute *attribute = to_hotplug_attr(attr);
-	return attribute->show ? attribute->show(slot, buf) : 0;
+	return attribute->show ? attribute->show(slot, buf) : -ENOSYS;
 }
 
 static ssize_t hotplug_slot_attr_store(struct kobject *kobj,

@@ -81,7 +81,7 @@ static ssize_t hotplug_slot_attr_store(s
 {
 	struct hotplug_slot *slot = to_hotplug_slot(kobj);
 	struct hotplug_slot_attribute *attribute = to_hotplug_attr(attr);
-	return attribute->store ? attribute->store(slot, buf, len) : 0;
+	return attribute->store ? attribute->store(slot, buf, len) : -ENOSYS;
 }
 
 static struct sysfs_ops hotplug_slot_sysfs_ops = {
author
#4 lore raw

PATCH 4/5 sysfs: (driver/block) if show/store is missing return -ENOSYS

In reply to Dmitry Torokhov

sysfs: fix drivers/block so if an attribute doesn't implement
show or store method read/write will return -ENOSYS instead of 0 or -EINVAL.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 as-iosched.c       |    4 ++--
 cfq-iosched.c      |    4 ++--
 deadline-iosched.c |    4 ++--
 genhd.c            |    2 +-
 ll_rw_blk.c        |    4 ++--
 5 files changed, 9 insertions(+), 9 deletions(-)
drivers/block/as-iosched.c +2 -2
Index: dtor/drivers/block/as-iosched.c
===================================================================
@@ -2044,7 +2044,7 @@ as_attr_show(struct kobject *kobj, struc
 	struct as_fs_entry *entry = to_as(attr);
 
 	if (!entry->show)
-		return 0;
+		return -ENOSYS;
 
 	return entry->show(e->elevator_data, page);
 }

@@ -2057,7 +2057,7 @@ as_attr_store(struct kobject *kobj, stru
 	struct as_fs_entry *entry = to_as(attr);
 
 	if (!entry->store)
-		return -EINVAL;
+		return -ENOSYS;
 
 	return entry->store(e->elevator_data, page, length);
 }
drivers/block/cfq-iosched.c +2 -2
Index: dtor/drivers/block/cfq-iosched.c
===================================================================
@@ -1772,7 +1772,7 @@ cfq_attr_show(struct kobject *kobj, stru
 	struct cfq_fs_entry *entry = to_cfq(attr);
 
 	if (!entry->show)
-		return 0;
+		return -ENOSYS;
 
 	return entry->show(e->elevator_data, page);
 }

@@ -1785,7 +1785,7 @@ cfq_attr_store(struct kobject *kobj, str
 	struct cfq_fs_entry *entry = to_cfq(attr);
 
 	if (!entry->store)
-		return -EINVAL;
+		return -ENOSYS;
 
 	return entry->store(e->elevator_data, page, length);
 }
drivers/block/genhd.c +1 -1
Index: dtor/drivers/block/genhd.c
===================================================================
@@ -321,7 +321,7 @@ static ssize_t disk_attr_show(struct kob
 	struct gendisk *disk = to_disk(kobj);
 	struct disk_attribute *disk_attr =
 		container_of(attr,struct disk_attribute,attr);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (disk_attr->show)
 		ret = disk_attr->show(disk,page);
drivers/block/ll_rw_blk.c +2 -2
Index: dtor/drivers/block/ll_rw_blk.c
===================================================================
@@ -3582,7 +3582,7 @@ queue_attr_show(struct kobject *kobj, st
 
 	q = container_of(kobj, struct request_queue, kobj);
 	if (!entry->show)
-		return 0;
+		return -ENOSYS;
 
 	return entry->show(q, page);
 }

@@ -3596,7 +3596,7 @@ queue_attr_store(struct kobject *kobj, s
 
 	q = container_of(kobj, struct request_queue, kobj);
 	if (!entry->store)
-		return -EINVAL;
+		return -ENOSYS;
 
 	return entry->store(q, page, length);
 }
drivers/block/deadline-iosched.c +2 -2
Index: dtor/drivers/block/deadline-iosched.c
===================================================================
@@ -886,7 +886,7 @@ deadline_attr_show(struct kobject *kobj,
 	struct deadline_fs_entry *entry = to_deadline(attr);
 
 	if (!entry->show)
-		return 0;
+		return -ENOSYS;
 
 	return entry->show(e->elevator_data, page);
 }

@@ -899,7 +899,7 @@ deadline_attr_store(struct kobject *kobj
 	struct deadline_fs_entry *entry = to_deadline(attr);
 
 	if (!entry->store)
-		return -EINVAL;
+		return -ENOSYS;
 
 	return entry->store(e->elevator_data, page, length);
 }
author
#5 lore raw

PATCH 5/5 sysfs: (rest) if show/store is missing return -ENOSYS

In reply to Dmitry Torokhov

sysfs: fix the rest of the kernel so if an attribute doesn't
implement show or store method read/write will return -ENOSYS instead of 0 or -EINVAL or -EPERM.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/acpi/scan.c             |    4 ++--
 drivers/cpufreq/cpufreq.c       |    4 ++--
 drivers/firmware/edd.c          |    2 +-
 drivers/firmware/efivars.c      |    4 ++--
 drivers/infiniband/core/sysfs.c |    2 +-
 kernel/params.c                 |    4 ++--
 security/seclvl.c               |    4 ++--
 7 files changed, 12 insertions(+), 12 deletions(-)
drivers/cpufreq/cpufreq.c +2 -2
Index: dtor/drivers/cpufreq/cpufreq.c
===================================================================
@@ -521,7 +521,7 @@ static ssize_t show(struct kobject * kob
 	policy = cpufreq_cpu_get(policy->cpu);
 	if (!policy)
 		return -EINVAL;
-	ret = fattr->show ? fattr->show(policy,buf) : 0;
+	ret = fattr->show ? fattr->show(policy,buf) : -ENOSYS;
 	cpufreq_cpu_put(policy);
 	return ret;
 }

@@ -535,7 +535,7 @@ static ssize_t store(struct kobject * ko
 	policy = cpufreq_cpu_get(policy->cpu);
 	if (!policy)
 		return -EINVAL;
-	ret = fattr->store ? fattr->store(policy,buf,count) : 0;
+	ret = fattr->store ? fattr->store(policy,buf,count) : -ENOSYS;
 	cpufreq_cpu_put(policy);
 	return ret;
 }
drivers/firmware/efivars.c +2 -2
Index: dtor/drivers/firmware/efivars.c
===================================================================
@@ -352,7 +352,7 @@ static ssize_t efivar_attr_show(struct k
 {
 	struct efivar_entry *var = to_efivar_entry(kobj);
 	struct efivar_attribute *efivar_attr = to_efivar_attr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;

@@ -368,7 +368,7 @@ static ssize_t efivar_attr_store(struct 
 {
 	struct efivar_entry *var = to_efivar_entry(kobj);
 	struct efivar_attribute *efivar_attr = to_efivar_attr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
kernel/params.c +2 -2
Index: dtor/kernel/params.c
===================================================================
@@ -629,7 +629,7 @@ static ssize_t module_attr_show(struct k
 	mk = to_module_kobject(kobj);
 
 	if (!attribute->show)
-		return -EPERM;
+		return -ENOSYS;
 
 	if (!try_module_get(mk->mod))
 		return -ENODEV;

@@ -653,7 +653,7 @@ static ssize_t module_attr_store(struct 
 	mk = to_module_kobject(kobj);
 
 	if (!attribute->store)
-		return -EPERM;
+		return -ENOSYS;
 
 	if (!try_module_get(mk->mod))
 		return -ENODEV;
drivers/infiniband/core/sysfs.c +1 -1
Index: dtor/drivers/infiniband/core/sysfs.c
===================================================================
@@ -71,7 +71,7 @@ static ssize_t port_attr_show(struct kob
 	struct ib_port *p = container_of(kobj, struct ib_port, kobj);
 
 	if (!port_attr->show)
-		return 0;
+		return -ENOSYS;
 
 	return port_attr->show(p, port_attr, buf);
 }
security/seclvl.c +2 -2
Index: dtor/security/seclvl.c
===================================================================
@@ -155,7 +155,7 @@ seclvl_attr_store(struct kobject *kobj,
 	struct seclvl_obj *obj = container_of(kobj, struct seclvl_obj, kobj);
 	struct seclvl_attribute *attribute =
 	    container_of(attr, struct seclvl_attribute, attr);
-	return (attribute->store ? attribute->store(obj, buf, len) : 0);
+	return attribute->store ? attribute->store(obj, buf, len) : -ENOSYS;
 }
 
 static ssize_t

@@ -164,7 +164,7 @@ seclvl_attr_show(struct kobject *kobj, s
 	struct seclvl_obj *obj = container_of(kobj, struct seclvl_obj, kobj);
 	struct seclvl_attribute *attribute =
 	    container_of(attr, struct seclvl_attribute, attr);
-	return (attribute->show ? attribute->show(obj, buf) : 0);
+	return attribute->show ? attribute->show(obj, buf) : -ENOSYS;
 }
 
 /**
drivers/acpi/scan.c +2 -2
Index: dtor/drivers/acpi/scan.c
===================================================================
@@ -65,14 +65,14 @@ static ssize_t acpi_device_attr_show(str
 {
 	struct acpi_device *device = to_acpi_device(kobj);
 	struct acpi_device_attribute *attribute = to_handle_attr(attr);
-	return attribute->show ? attribute->show(device, buf) : 0;
+	return attribute->show ? attribute->show(device, buf) : -ENOSYS;
 }
 static ssize_t acpi_device_attr_store(struct kobject *kobj,
 		struct attribute *attr, const char *buf, size_t len)
 {
 	struct acpi_device *device = to_acpi_device(kobj);
 	struct acpi_device_attribute *attribute = to_handle_attr(attr);
-	return attribute->store ? attribute->store(device, buf, len) : len;
+	return attribute->store ? attribute->store(device, buf, len) : -ENOSYS;
 }
 
 static struct sysfs_ops acpi_device_sysfs_ops = {
drivers/firmware/edd.c +1 -1
Index: dtor/drivers/firmware/edd.c
===================================================================
@@ -115,7 +115,7 @@ edd_attr_show(struct kobject * kobj, str
 {
 	struct edd_device *dev = to_edd_device(kobj);
 	struct edd_attribute *edd_attr = to_edd_attr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -ENOSYS;
 
 	if (edd_attr->show)
 		ret = edd_attr->show(dev, buf);
#6 lore raw
In reply to Dmitry Torokhov
Quoted 9 lines · Dmitry Torokhov

Hi,

Jean Delvare has noticed that if a driver happens to declare its attribute as RW but doesn't provide store() method attempt to write into such attribute will cause spinning process as most of the attribute implementations return 0 in case of missing store causing endless retries. In some cases missing show/store will return -EPERM, -EACCESS or -EINVAL.

Quoted 3 lines

I think we should unify implementations and have them all return -ENOSYS (function not implemented) when corresponding method (show/store) is missing.

What is the POSIX standard for this? ENOSYS or EACCESS?

Or anyone have a link that I can look this up at?

thanks,

greg k-h

#7 lore raw
Quoted 10 lines · * Greg KH ([email protected]) wrote:
Quoted 9 lines · Dmitry Torokhov

Hi,

Jean Delvare has noticed that if a driver happens to declare its attribute as RW but doesn't provide store() method attempt to write into such attribute will cause spinning process as most of the attribute implementations return 0 in case of missing store causing endless retries. In some cases missing show/store will return -EPERM, -EACCESS or -EINVAL.

Quoted 5 lines
Quoted 3 lines

I think we should unify implementations and have them all return -ENOSYS (function not implemented) when corresponding method (show/store) is missing.

What is the POSIX standard for this? ENOSYS or EACCESS?

SuSv3 suggests EBADF, however we already do EINVAL at VFS for no write op. Although, returning 0 (i.e. wrote zero bytes) is still meaningful too.

Quoted 1 line

Or anyone have a link that I can look this up at?

http://www.opengroup.org/onlinepubs/000095399/functions/write.html

thanks,
-chris

#8 lore raw
Quoted 3 lines · Chris Wright

SuSv3 suggests EBADF, however we already do EINVAL at VFS for no write op. Although, returning 0 (i.e. wrote zero bytes) is still meaningful too.

I think EBADF implies that you opened the thing not for writing, and now you are trying to write to it.

But my understanding of this problem is that the open is succeeding for writes, but when the actual write is performed, the store fails (or does not exist).

EIO makes sense for that. EACCESS less so, but still some.

Robert Love

#9 lore raw
In reply to Chris Wright
Quoted 15 lines · Chris Wright
Quoted 14 lines · * Greg KH ([email protected]) wrote:
Quoted 12 lines · Dmitry Torokhov

Hi,

Jean Delvare has noticed that if a driver happens to declare its attribute as RW but doesn't provide store() method attempt to write into such attribute will cause spinning process as most of the attribute implementations return 0 in case of missing store causing endless retries. In some cases missing show/store will return -EPERM, -EACCESS or -EINVAL.

I think we should unify implementations and have them all return -ENOSYS (function not implemented) when corresponding method (show/store) is missing.

Quoted 6 lines
Quoted 1 line

What is the POSIX standard for this? ENOSYS or EACCESS?

SuSv3 suggests EBADF, however we already do EINVAL at VFS for no write op. Although, returning 0 (i.e. wrote zero bytes) is still meaningful too.

Returning 0 causes caller to immediately repeat operation causing the loop and 100% CPU utiluization as was reported. If ENOSYS is not acceptable (after all the problem is sysfs-specific, other fs-es either support write or they don't for entire volume) I'd say (looking at the descripptions) we should use ENXIO instead of EBADF:

[ENXIO]
A request was made of a nonexistent device, or the request was outside the capabilities of the device.

[EBADF]
The fildes argument is not a valid file descriptor open for writing.

We have a valid FD and it was opened for wrinting, so EBADF is not really applicable. But then I may be talking complete gibberish...

Signature · 1 line
Dmitry
#10 lore raw
Quoted 33 lines · Dmitry Torokhov
Quoted 15 lines · Chris Wright
Quoted 14 lines · * Greg KH ([email protected]) wrote:
Quoted 12 lines · Dmitry Torokhov

Hi,

Jean Delvare has noticed that if a driver happens to declare its attribute as RW but doesn't provide store() method attempt to write into such attribute will cause spinning process as most of the attribute implementations return 0 in case of missing store causing endless retries. In some cases missing show/store will return -EPERM, -EACCESS or -EINVAL.

I think we should unify implementations and have them all return -ENOSYS (function not implemented) when corresponding method (show/store) is missing.

Quoted 6 lines
Quoted 1 line

What is the POSIX standard for this? ENOSYS or EACCESS?

SuSv3 suggests EBADF, however we already do EINVAL at VFS for no write op. Although, returning 0 (i.e. wrote zero bytes) is still meaningful too.

Returning 0 causes caller to immediately repeat operation causing the loop and 100% CPU utiluization as was reported. If ENOSYS is not acceptable (after all the problem is sysfs-specific, other fs-es either support write or they don't for entire volume) I'd say (looking at the descripptions) we should use ENXIO instead of EBADF:

[ENXIO]
A request was made of a nonexistent device, or the request was outside the capabilities of the device.

Quoted 9 lines

[EBADF]
The fildes argument is not a valid file descriptor open for writing.

We have a valid FD and it was opened for wrinting, so EBADF is not really applicable. But then I may be talking complete gibberish...

Signature · 2 lines
Dmitry
-

Perhaps...
ENOSPC, i.e., the device is full. This is the usual return value for a CD/ROM and df always shows it's full even if it has one small file.

The only thing that really makes sense is EPERM but it is usually associated with permissions rather than if an operation is permitted.

Cheers,
Dick Johnson
Penguin : Linux version 2.6.11 on an i686 machine (5537.79 BogoMips).
Notice : All mail here is now cached for review by Dictator Bush.
98.36% of all statistics are fiction.

#11 lore raw
Quoted 4 lines · Richard B. Johnson

Perhaps...
ENOSPC, i.e., the device is full. This is the usual return value for a CD/ROM and df always shows it's full even if it has one small file.

But there is space on the backing device. Given two files on the same file system, one should not be returning ENOSPC on write and the other succeeding.

Quoted 2 lines

The only thing that really makes sense is EPERM but it is usually associated with permissions rather than if an operation is permitted.

EPERM isn't even a valid SUSv3 return for write(2). Permissions don't apply to writes, only opens (and, thus, this is a valid return for open(2)).

The answer has already been suggested: EACCESS or EIO.

Robert Love

#12 lore raw
In reply to Greg KH
Quoted 3 lines · Greg KH

What is the POSIX standard for this? ENOSYS or EACCESS?

Or anyone have a link that I can look this up at?

ENOSYS isn't defined by SUSv3 (and thus not POSIX) for write(2).

EACCESS is defined only for socket errors, but you could of course hijack it. I think it is silly, though, since the open(2) succeeded. Ideally, the open for writing should fail with EACCESS.

EIO actually means that an internal or physical I/O error occurred, PLUS it is reserved for implementation-defined errors. So that makes sense.

The main thing is to _not_ return zero. That would cause stdio to resubmit indefinitely.

Robert Love

author
#13 lore raw

PATCH 1/5 sysfs: if show/store is missing return -EIO

sysfs: if attribute does not implement show or store method
read/write should return -EIO instead of 0 or -EINVAL.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 bin.c  |    4 ++--
 file.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
fs/sysfs/file.c +2 -2
Index: dtor/fs/sysfs/file.c
===================================================================
@@ -23,7 +23,7 @@ subsys_attr_show(struct kobject * kobj, 
 {
 	struct subsystem * s = to_subsys(kobj);
 	struct subsys_attribute * sattr = to_sattr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (sattr->show)
 		ret = sattr->show(s,page);

@@ -36,7 +36,7 @@ subsys_attr_store(struct kobject * kobj,
 {
 	struct subsystem * s = to_subsys(kobj);
 	struct subsys_attribute * sattr = to_sattr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (sattr->store)
 		ret = sattr->store(s,page,count);
fs/sysfs/bin.c +2 -2
Index: dtor/fs/sysfs/bin.c
===================================================================
@@ -25,7 +25,7 @@ fill_read(struct dentry *dentry, char *b
 	struct kobject * kobj = to_kobj(dentry->d_parent);
 
 	if (!attr->read)
-		return -EINVAL;
+		return -EIO;
 
 	return attr->read(kobj, buffer, off, count);
 }

@@ -71,7 +71,7 @@ flush_write(struct dentry *dentry, char 
 	struct kobject *kobj = to_kobj(dentry->d_parent);
 
 	if (!attr->write)
-		return -EINVAL;
+		return -EIO;
 
 	return attr->write(kobj, buffer, offset, count);
 }
author
#14 lore raw

PATCH 2/5 sysfs: (driver/base) if show/store is missing return -EIO

sysfs: fix drivers/base so if an attribute doesn't implement
show or store method read/write will return -EIO instead of 0.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 bus.c   |    4 ++--
 class.c |    4 ++--
 core.c  |    4 ++--
 sys.c   |    4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)
drivers/base/class.c +2 -2
Index: dtor/drivers/base/class.c
===================================================================
@@ -26,7 +26,7 @@ class_attr_show(struct kobject * kobj, s
 {
 	struct class_attribute * class_attr = to_class_attr(attr);
 	struct class * dc = to_class(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (class_attr->show)
 		ret = class_attr->show(dc, buf);

@@ -39,7 +39,7 @@ class_attr_store(struct kobject * kobj, 
 {
 	struct class_attribute * class_attr = to_class_attr(attr);
 	struct class * dc = to_class(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (class_attr->store)
 		ret = class_attr->store(dc, buf, count);
drivers/base/sys.c +2 -2
Index: dtor/drivers/base/sys.c
===================================================================
@@ -37,7 +37,7 @@ sysdev_show(struct kobject * kobj, struc
 
 	if (sysdev_attr->show)
 		return sysdev_attr->show(sysdev, buffer);
-	return 0;
+	return -EIO;
 }
 
 

@@ -50,7 +50,7 @@ sysdev_store(struct kobject * kobj, stru
 
 	if (sysdev_attr->store)
 		return sysdev_attr->store(sysdev, buffer, count);
-	return 0;
+	return -EIO;
 }
 
 static struct sysfs_ops sysfs_ops = {
drivers/base/bus.c +2 -2
Index: dtor/drivers/base/bus.c
===================================================================
@@ -36,7 +36,7 @@ drv_attr_show(struct kobject * kobj, str
 {
 	struct driver_attribute * drv_attr = to_drv_attr(attr);
 	struct device_driver * drv = to_driver(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (drv_attr->show)
 		ret = drv_attr->show(drv, buf);

@@ -49,7 +49,7 @@ drv_attr_store(struct kobject * kobj, st
 {
 	struct driver_attribute * drv_attr = to_drv_attr(attr);
 	struct device_driver * drv = to_driver(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (drv_attr->store)
 		ret = drv_attr->store(drv, buf, count);
drivers/base/core.c +2 -2
Index: dtor/drivers/base/core.c
===================================================================
@@ -38,7 +38,7 @@ dev_attr_show(struct kobject * kobj, str
 {
 	struct device_attribute * dev_attr = to_dev_attr(attr);
 	struct device * dev = to_dev(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (dev_attr->show)
 		ret = dev_attr->show(dev, buf);

@@ -51,7 +51,7 @@ dev_attr_store(struct kobject * kobj, st
 {
 	struct device_attribute * dev_attr = to_dev_attr(attr);
 	struct device * dev = to_dev(kobj);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (dev_attr->store)
 		ret = dev_attr->store(dev, buf, count);
author
#15 lore raw

PATCH 3/5 sysfs: (driver/pci) if show/store is missing return -EIO

In reply to Dmitry Torokhov

sysfs: fix drivers/pci so if an attribute does not implement
show or store method read/write will return -EIO instead of 0.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 hotplug/pci_hotplug_core.c |    4 ++--
 hotplug/rpadlpar_sysfs.c   |    2 +-
 pci-driver.c               |   26 ++++++++++++++------------
 3 files changed, 17 insertions(+), 15 deletions(-)
drivers/pci/hotplug/rpadlpar_sysfs.c +1 -1
Index: dtor/drivers/pci/hotplug/rpadlpar_sysfs.c
===================================================================
@@ -48,7 +48,7 @@ dlpar_attr_store(struct kobject * kobj, 
 	struct dlpar_io_attr *dlpar_attr = container_of(attr,
 						struct dlpar_io_attr, attr);
 	return dlpar_attr->store ?
-		dlpar_attr->store(dlpar_attr, buf, nbytes) : 0;
+		dlpar_attr->store(dlpar_attr, buf, nbytes) : -EIO;
 }
 
 static struct sysfs_ops dlpar_attr_sysfs_ops = {
drivers/pci/pci-driver.c +14 -12
Index: dtor/drivers/pci/pci-driver.c
===================================================================
@@ -327,13 +327,14 @@ pci_driver_attr_show(struct kobject * ko
 {
 	struct device_driver *driver = kobj_to_pci_driver(kobj);
 	struct driver_attribute *dattr = attr_to_driver_attribute(attr);
-	ssize_t ret = 0;
+	ssize_t ret;
 
-	if (get_driver(driver)) {
-		if (dattr->show)
-			ret = dattr->show(driver, buf);
-		put_driver(driver);
-	}
+	if (!get_driver(driver))
+		return -ENODEV;
+
+	ret = dattr->show ? dattr->show(driver, buf) : -EIO;
+
+	put_driver(driver);
 	return ret;
 }
 

@@ -343,13 +344,14 @@ pci_driver_attr_store(struct kobject * k
 {
 	struct device_driver *driver = kobj_to_pci_driver(kobj);
 	struct driver_attribute *dattr = attr_to_driver_attribute(attr);
-	ssize_t ret = 0;
+	ssize_t ret;
 
-	if (get_driver(driver)) {
-		if (dattr->store)
-			ret = dattr->store(driver, buf, count);
-		put_driver(driver);
-	}
+	if (!get_driver(driver))
+		return -ENODEV;
+
+	ret = dattr->store ? dattr->store(driver, buf, count) : -EIO;
+
+	put_driver(driver);
 	return ret;
 }
drivers/pci/hotplug/pci_hotplug_core.c +2 -2
Index: dtor/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================
@@ -73,7 +73,7 @@ static ssize_t hotplug_slot_attr_show(st
 {
 	struct hotplug_slot *slot = to_hotplug_slot(kobj);
 	struct hotplug_slot_attribute *attribute = to_hotplug_attr(attr);
-	return attribute->show ? attribute->show(slot, buf) : 0;
+	return attribute->show ? attribute->show(slot, buf) : -EIO;
 }
 
 static ssize_t hotplug_slot_attr_store(struct kobject *kobj,

@@ -81,7 +81,7 @@ static ssize_t hotplug_slot_attr_store(s
 {
 	struct hotplug_slot *slot = to_hotplug_slot(kobj);
 	struct hotplug_slot_attribute *attribute = to_hotplug_attr(attr);
-	return attribute->store ? attribute->store(slot, buf, len) : 0;
+	return attribute->store ? attribute->store(slot, buf, len) : -EIO;
 }
 
 static struct sysfs_ops hotplug_slot_sysfs_ops = {
author
#16 lore raw

PATCH 4/5 sysfs: (driver/block) if show/store is missing return -EIO

In reply to Dmitry Torokhov

sysfs: fix drivers/block so if an attribute doesn't implement
show or store method read/write will return -EIO instead of 0 or -EINVAL.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 as-iosched.c       |    4 ++--
 cfq-iosched.c      |    4 ++--
 deadline-iosched.c |    4 ++--
 genhd.c            |    2 +-
 ll_rw_blk.c        |    4 ++--
 5 files changed, 9 insertions(+), 9 deletions(-)
drivers/block/as-iosched.c +2 -2
Index: dtor/drivers/block/as-iosched.c
===================================================================
@@ -2044,7 +2044,7 @@ as_attr_show(struct kobject *kobj, struc
 	struct as_fs_entry *entry = to_as(attr);
 
 	if (!entry->show)
-		return 0;
+		return -EIO;
 
 	return entry->show(e->elevator_data, page);
 }

@@ -2057,7 +2057,7 @@ as_attr_store(struct kobject *kobj, stru
 	struct as_fs_entry *entry = to_as(attr);
 
 	if (!entry->store)
-		return -EINVAL;
+		return -EIO;
 
 	return entry->store(e->elevator_data, page, length);
 }
drivers/block/cfq-iosched.c +2 -2
Index: dtor/drivers/block/cfq-iosched.c
===================================================================
@@ -1772,7 +1772,7 @@ cfq_attr_show(struct kobject *kobj, stru
 	struct cfq_fs_entry *entry = to_cfq(attr);
 
 	if (!entry->show)
-		return 0;
+		return -EIO;
 
 	return entry->show(e->elevator_data, page);
 }

@@ -1785,7 +1785,7 @@ cfq_attr_store(struct kobject *kobj, str
 	struct cfq_fs_entry *entry = to_cfq(attr);
 
 	if (!entry->store)
-		return -EINVAL;
+		return -EIO;
 
 	return entry->store(e->elevator_data, page, length);
 }
drivers/block/genhd.c +1 -1
Index: dtor/drivers/block/genhd.c
===================================================================
@@ -321,7 +321,7 @@ static ssize_t disk_attr_show(struct kob
 	struct gendisk *disk = to_disk(kobj);
 	struct disk_attribute *disk_attr =
 		container_of(attr,struct disk_attribute,attr);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (disk_attr->show)
 		ret = disk_attr->show(disk,page);
drivers/block/ll_rw_blk.c +2 -2
Index: dtor/drivers/block/ll_rw_blk.c
===================================================================
@@ -3582,7 +3582,7 @@ queue_attr_show(struct kobject *kobj, st
 
 	q = container_of(kobj, struct request_queue, kobj);
 	if (!entry->show)
-		return 0;
+		return -EIO;
 
 	return entry->show(q, page);
 }

@@ -3596,7 +3596,7 @@ queue_attr_store(struct kobject *kobj, s
 
 	q = container_of(kobj, struct request_queue, kobj);
 	if (!entry->store)
-		return -EINVAL;
+		return -EIO;
 
 	return entry->store(q, page, length);
 }
drivers/block/deadline-iosched.c +2 -2
Index: dtor/drivers/block/deadline-iosched.c
===================================================================
@@ -886,7 +886,7 @@ deadline_attr_show(struct kobject *kobj,
 	struct deadline_fs_entry *entry = to_deadline(attr);
 
 	if (!entry->show)
-		return 0;
+		return -EIO;
 
 	return entry->show(e->elevator_data, page);
 }

@@ -899,7 +899,7 @@ deadline_attr_store(struct kobject *kobj
 	struct deadline_fs_entry *entry = to_deadline(attr);
 
 	if (!entry->store)
-		return -EINVAL;
+		return -EIO;
 
 	return entry->store(e->elevator_data, page, length);
 }
author
#17 lore raw

PATCH 5/5 sysfs: (rest) if show/store is missing return -EIO

In reply to Dmitry Torokhov

sysfs: fix the rest of the kernel so if an attribute doesn't
implement show or store method read/write will return -EIO instead of 0 or -EINVAL or -EPERM.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/acpi/scan.c             |    4 ++--
 drivers/cpufreq/cpufreq.c       |    4 ++--
 drivers/firmware/edd.c          |    2 +-
 drivers/firmware/efivars.c      |    4 ++--
 drivers/infiniband/core/sysfs.c |    2 +-
 kernel/params.c                 |    4 ++--
 security/seclvl.c               |    4 ++--
 7 files changed, 12 insertions(+), 12 deletions(-)
drivers/cpufreq/cpufreq.c +2 -2
Index: dtor/drivers/cpufreq/cpufreq.c
===================================================================
@@ -521,7 +521,7 @@ static ssize_t show(struct kobject * kob
 	policy = cpufreq_cpu_get(policy->cpu);
 	if (!policy)
 		return -EINVAL;
-	ret = fattr->show ? fattr->show(policy,buf) : 0;
+	ret = fattr->show ? fattr->show(policy,buf) : -EIO;
 	cpufreq_cpu_put(policy);
 	return ret;
 }

@@ -535,7 +535,7 @@ static ssize_t store(struct kobject * ko
 	policy = cpufreq_cpu_get(policy->cpu);
 	if (!policy)
 		return -EINVAL;
-	ret = fattr->store ? fattr->store(policy,buf,count) : 0;
+	ret = fattr->store ? fattr->store(policy,buf,count) : -EIO;
 	cpufreq_cpu_put(policy);
 	return ret;
 }
drivers/firmware/efivars.c +2 -2
Index: dtor/drivers/firmware/efivars.c
===================================================================
@@ -352,7 +352,7 @@ static ssize_t efivar_attr_show(struct k
 {
 	struct efivar_entry *var = to_efivar_entry(kobj);
 	struct efivar_attribute *efivar_attr = to_efivar_attr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;

@@ -368,7 +368,7 @@ static ssize_t efivar_attr_store(struct 
 {
 	struct efivar_entry *var = to_efivar_entry(kobj);
 	struct efivar_attribute *efivar_attr = to_efivar_attr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
kernel/params.c +2 -2
Index: dtor/kernel/params.c
===================================================================
@@ -629,7 +629,7 @@ static ssize_t module_attr_show(struct k
 	mk = to_module_kobject(kobj);
 
 	if (!attribute->show)
-		return -EPERM;
+		return -EIO;
 
 	if (!try_module_get(mk->mod))
 		return -ENODEV;

@@ -653,7 +653,7 @@ static ssize_t module_attr_store(struct 
 	mk = to_module_kobject(kobj);
 
 	if (!attribute->store)
-		return -EPERM;
+		return -EIO;
 
 	if (!try_module_get(mk->mod))
 		return -ENODEV;
drivers/infiniband/core/sysfs.c +1 -1
Index: dtor/drivers/infiniband/core/sysfs.c
===================================================================
@@ -71,7 +71,7 @@ static ssize_t port_attr_show(struct kob
 	struct ib_port *p = container_of(kobj, struct ib_port, kobj);
 
 	if (!port_attr->show)
-		return 0;
+		return -EIO;
 
 	return port_attr->show(p, port_attr, buf);
 }
security/seclvl.c +2 -2
Index: dtor/security/seclvl.c
===================================================================
@@ -155,7 +155,7 @@ seclvl_attr_store(struct kobject *kobj,
 	struct seclvl_obj *obj = container_of(kobj, struct seclvl_obj, kobj);
 	struct seclvl_attribute *attribute =
 	    container_of(attr, struct seclvl_attribute, attr);
-	return (attribute->store ? attribute->store(obj, buf, len) : 0);
+	return attribute->store ? attribute->store(obj, buf, len) : -EIO;
 }
 
 static ssize_t

@@ -164,7 +164,7 @@ seclvl_attr_show(struct kobject *kobj, s
 	struct seclvl_obj *obj = container_of(kobj, struct seclvl_obj, kobj);
 	struct seclvl_attribute *attribute =
 	    container_of(attr, struct seclvl_attribute, attr);
-	return (attribute->show ? attribute->show(obj, buf) : 0);
+	return attribute->show ? attribute->show(obj, buf) : -EIO;
 }
 
 /**
drivers/acpi/scan.c +2 -2
Index: dtor/drivers/acpi/scan.c
===================================================================
@@ -65,14 +65,14 @@ static ssize_t acpi_device_attr_show(str
 {
 	struct acpi_device *device = to_acpi_device(kobj);
 	struct acpi_device_attribute *attribute = to_handle_attr(attr);
-	return attribute->show ? attribute->show(device, buf) : 0;
+	return attribute->show ? attribute->show(device, buf) : -EIO;
 }
 static ssize_t acpi_device_attr_store(struct kobject *kobj,
 		struct attribute *attr, const char *buf, size_t len)
 {
 	struct acpi_device *device = to_acpi_device(kobj);
 	struct acpi_device_attribute *attribute = to_handle_attr(attr);
-	return attribute->store ? attribute->store(device, buf, len) : len;
+	return attribute->store ? attribute->store(device, buf, len) : -EIO;
 }
 
 static struct sysfs_ops acpi_device_sysfs_ops = {
drivers/firmware/edd.c +1 -1
Index: dtor/drivers/firmware/edd.c
===================================================================
@@ -115,7 +115,7 @@ edd_attr_show(struct kobject * kobj, str
 {
 	struct edd_device *dev = to_edd_device(kobj);
 	struct edd_attribute *edd_attr = to_edd_attr(attr);
-	ssize_t ret = 0;
+	ssize_t ret = -EIO;
 
 	if (edd_attr->show)
 		ret = edd_attr->show(dev, buf);