drivers/base/bus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
From: Zijun Hu <quic_zijuhu@quicinc.com>
Return -EIO instead of 0 when show/store invalid bus attribute as
class/device/driver/kobject attribute.
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
drivers/base/bus.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index ffea0728b8b2..e84522a90102 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -152,7 +152,7 @@ static ssize_t bus_attr_show(struct kobject *kobj, struct attribute *attr,
{
struct bus_attribute *bus_attr = to_bus_attr(attr);
struct subsys_private *subsys_priv = to_subsys_private(kobj);
- ssize_t ret = 0;
+ ssize_t ret = -EIO;
if (bus_attr->show)
ret = bus_attr->show(subsys_priv->bus, buf);
@@ -164,7 +164,7 @@ static ssize_t bus_attr_store(struct kobject *kobj, struct attribute *attr,
{
struct bus_attribute *bus_attr = to_bus_attr(attr);
struct subsys_private *subsys_priv = to_subsys_private(kobj);
- ssize_t ret = 0;
+ ssize_t ret = -EIO;
if (bus_attr->store)
ret = bus_attr->store(subsys_priv->bus, buf, count);
---
base-commit: b57d5ffc3ab507d0e19fc8b90b19c76af43fb790
change-id: 20240723-bus_fix-1940d8e79e92
Best regards,
--
Zijun Hu <quic_zijuhu@quicinc.com>
On Tue, Jul 23, 2024 at 10:55:43PM +0800, Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > Return -EIO instead of 0 when show/store invalid bus attribute as > class/device/driver/kobject attribute. Why? What is this now going to break? You are changing a user-visable api that has been this way for 20+ years, how was this tested? > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > drivers/base/bus.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c > index ffea0728b8b2..e84522a90102 100644 > --- a/drivers/base/bus.c > +++ b/drivers/base/bus.c > @@ -152,7 +152,7 @@ static ssize_t bus_attr_show(struct kobject *kobj, struct attribute *attr, > { > struct bus_attribute *bus_attr = to_bus_attr(attr); > struct subsys_private *subsys_priv = to_subsys_private(kobj); > - ssize_t ret = 0; > + ssize_t ret = -EIO; > > if (bus_attr->show) > ret = bus_attr->show(subsys_priv->bus, buf); > @@ -164,7 +164,7 @@ static ssize_t bus_attr_store(struct kobject *kobj, struct attribute *attr, > { > struct bus_attribute *bus_attr = to_bus_attr(attr); > struct subsys_private *subsys_priv = to_subsys_private(kobj); > - ssize_t ret = 0; > + ssize_t ret = -EIO; Why -EIO? What is wrong with returning an empty string? I think I know why you picked this but you better document the heck out of it AND test it very very well first. thanks, greg k-h
On 7/24/2024 1:31 AM, Greg Kroah-Hartman wrote: > On Tue, Jul 23, 2024 at 10:55:43PM +0800, Zijun Hu wrote: >> From: Zijun Hu <quic_zijuhu@quicinc.com> >> >> Return -EIO instead of 0 when show/store invalid bus attribute as >> class/device/driver/kobject attribute. > > Why? What is this now going to break? You are changing a user-visable > api that has been this way for 20+ years, how was this tested? > this change should break nothing. tested by wc a writing only bus attribute, for example root@kvm-Q35:/sys/bus/gpio# ls -l --w------- 1 root root 4096 7月 24 20:20 drivers_probe root@kvm-Q35:/sys/bus/gpio# chmod u+r drivers_probe root@kvm-Q35:/sys/bus/gpio# wc -c drivers_probe 0 drivers_probe // for current design root@zijun-kvm-Q35:/sys/bus/gpio# wc -c drivers_probe wc: drivers_probe: Input/output error // for this change >> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> drivers/base/bus.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/base/bus.c b/drivers/base/bus.c >> index ffea0728b8b2..e84522a90102 100644 >> --- a/drivers/base/bus.c >> +++ b/drivers/base/bus.c >> @@ -152,7 +152,7 @@ static ssize_t bus_attr_show(struct kobject *kobj, struct attribute *attr, >> { >> struct bus_attribute *bus_attr = to_bus_attr(attr); >> struct subsys_private *subsys_priv = to_subsys_private(kobj); >> - ssize_t ret = 0; >> + ssize_t ret = -EIO; >> >> if (bus_attr->show) >> ret = bus_attr->show(subsys_priv->bus, buf); >> @@ -164,7 +164,7 @@ static ssize_t bus_attr_store(struct kobject *kobj, struct attribute *attr, >> { >> struct bus_attribute *bus_attr = to_bus_attr(attr); >> struct subsys_private *subsys_priv = to_subsys_private(kobj); >> - ssize_t ret = 0; >> + ssize_t ret = -EIO; > > Why -EIO? What is wrong with returning an empty string? > for this error case, all class/device/driver/kobject attributes return -EIO, i just follow them for bus attribute. for empty string, the value returned by attribute store() is returned. > I think I know why you picked this but you better document the heck out > of it AND test it very very well first. > sure. > thanks, > > greg k-h
On Wed, Jul 24, 2024 at 08:56:18PM +0800, quic_zijuhu wrote: > On 7/24/2024 1:31 AM, Greg Kroah-Hartman wrote: > > On Tue, Jul 23, 2024 at 10:55:43PM +0800, Zijun Hu wrote: > >> From: Zijun Hu <quic_zijuhu@quicinc.com> > >> > >> Return -EIO instead of 0 when show/store invalid bus attribute as > >> class/device/driver/kobject attribute. > > > > Why? What is this now going to break? You are changing a user-visable > > api that has been this way for 20+ years, how was this tested? > > > this change should break nothing. Have you tested all tools that access these files? Please document what was done for testing please. > tested by wc a writing only bus attribute, for example > > root@kvm-Q35:/sys/bus/gpio# ls -l > --w------- 1 root root 4096 7月 24 20:20 drivers_probe > root@kvm-Q35:/sys/bus/gpio# chmod u+r drivers_probe > root@kvm-Q35:/sys/bus/gpio# wc -c drivers_probe > 0 drivers_probe // for current design > > root@zijun-kvm-Q35:/sys/bus/gpio# wc -c drivers_probe > wc: drivers_probe: Input/output error // for this change That's just using a shell, I am referring to actual tools that read these files and rely on the contents and error values that they provide. thanks, greg k-h
On 7/24/2024 9:29 PM, Greg Kroah-Hartman wrote: > On Wed, Jul 24, 2024 at 08:56:18PM +0800, quic_zijuhu wrote: >> On 7/24/2024 1:31 AM, Greg Kroah-Hartman wrote: >>> On Tue, Jul 23, 2024 at 10:55:43PM +0800, Zijun Hu wrote: >>>> From: Zijun Hu <quic_zijuhu@quicinc.com> >>>> >>>> Return -EIO instead of 0 when show/store invalid bus attribute as >>>> class/device/driver/kobject attribute. >>> >>> Why? What is this now going to break? You are changing a user-visable >>> api that has been this way for 20+ years, how was this tested? >>> >> this change should break nothing. > > Have you tested all tools that access these files? Please document what > was done for testing please. > not yet. let me do more investigation then give reply. sorry to send v2 without noticing this reply. let us still discuss with this mail thread. >> tested by wc a writing only bus attribute, for example >> >> root@kvm-Q35:/sys/bus/gpio# ls -l >> --w------- 1 root root 4096 7月 24 20:20 drivers_probe >> root@kvm-Q35:/sys/bus/gpio# chmod u+r drivers_probe >> root@kvm-Q35:/sys/bus/gpio# wc -c drivers_probe >> 0 drivers_probe // for current design >> >> root@zijun-kvm-Q35:/sys/bus/gpio# wc -c drivers_probe >> wc: drivers_probe: Input/output error // for this change > > That's just using a shell, I am referring to actual tools that read > these files and rely on the contents and error values that they provide. > make sense. > thanks, > > greg k-h
© 2016 - 2024 Red Hat, Inc.