[v1] driver core: bus: Return -EIO instead of 0 when show/store invalid bus attribute

[PATCH] driver core: bus: Return -EIO instead of 0 when show/store invalid bus attribute

Posted by Zijun Hu 1 year, 6 months ago

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>

Re: [PATCH] driver core: bus: Return -EIO instead of 0 when show/store invalid bus attribute

Posted by Greg Kroah-Hartman 1 year, 6 months ago

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

Re: [PATCH] driver core: bus: Return -EIO instead of 0 when show/store invalid bus attribute

Posted by quic_zijuhu 1 year, 6 months ago

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

Re: [PATCH] driver core: bus: Return -EIO instead of 0 when show/store invalid bus attribute

Posted by Greg Kroah-Hartman 1 year, 6 months ago

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

Re: [PATCH] driver core: bus: Return -EIO instead of 0 when show/store invalid bus attribute

Posted by quic_zijuhu 1 year, 6 months ago

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