[PATCH v2] scsi: bsg: fix errno when scsi_bsg_register_queue fails

[PATCH v2] scsi: bsg: fix errno when scsi_bsg_register_queue fails

[Jackie Liu]

From: Jackie Liu <liuyun01@kylinos.cn>

When the value of error is printed, it will always be 0. Here, we should be
print the correct error code when scsi_bsg_register_queue fails.

Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@lst.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
---
 v1->v2:
 resend to linux-scsi mail list.

 drivers/scsi/scsi_sysfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 86793259e541..d8789f6cda62 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1379,6 +1379,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 			 * We're treating error on bsg register as non-fatal, so
 			 * pretend nothing went wrong.
 			 */
+			error = PTR_ERR(sdev->bsg_dev);
 			sdev_printk(KERN_INFO, sdev,
 				    "Failed to register bsg queue, errno=%d\n",
 				    error);
-- 
2.25.1

Re: [PATCH v2] scsi: bsg: fix errno when scsi_bsg_register_queue fails

[Guenter Roeck]

On Fri, Oct 22, 2021 at 09:02:01AM +0800, Jackie Liu wrote:
> From: Jackie Liu <liuyun01@kylinos.cn>
> 
> When the value of error is printed, it will always be 0. Here, we should be
> print the correct error code when scsi_bsg_register_queue fails.
> 

The comment above the changed code says:

"
We're treating error on bsg register as non-fatal, so pretend nothing went wrong.
"

With this patch in place, "error" is returned to the caller, and the code
no longer pretends that nothing is wrong. Also, the message is a dev_info
message, not dev_err, suggesting that ignoring the error was indeed on
purpose. Assuming the comment is correct, this patch is plain wrong;
the message should have printed PTR_ERR(sdev->bsg_dev) instead and not set
the 'error' variable.

Guenter

> Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
> ---
>  v1->v2:
>  resend to linux-scsi mail list.
> 
>  drivers/scsi/scsi_sysfs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 86793259e541..d8789f6cda62 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1379,6 +1379,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>  			 * We're treating error on bsg register as non-fatal, so
>  			 * pretend nothing went wrong.
>  			 */
> +			error = PTR_ERR(sdev->bsg_dev);
>  			sdev_printk(KERN_INFO, sdev,
>  				    "Failed to register bsg queue, errno=%d\n",
>  				    error);
> -- 
> 2.25.1
> 

Re: [PATCH v2] scsi: bsg: fix errno when scsi_bsg_register_queue fails

[Jackie Liu]

Hi Guenter.

Before commit ead09dd3aed5c ("scsi: bsg: Simplify device registration", 
the errno need return to the caller, I restore the old logic, and print
this errno.

--
Jackie Liu

在 2021/12/23 上午12:54, Guenter Roeck 写道:
> On Fri, Oct 22, 2021 at 09:02:01AM +0800, Jackie Liu wrote:
>> From: Jackie Liu <liuyun01@kylinos.cn>
>>
>> When the value of error is printed, it will always be 0. Here, we should be
>> print the correct error code when scsi_bsg_register_queue fails.
>>
> 
> The comment above the changed code says:
> 
> "
> We're treating error on bsg register as non-fatal, so pretend nothing went wrong.
> "
> 
> With this patch in place, "error" is returned to the caller, and the code
> no longer pretends that nothing is wrong. Also, the message is a dev_info
> message, not dev_err, suggesting that ignoring the error was indeed on
> purpose. Assuming the comment is correct, this patch is plain wrong;
> the message should have printed PTR_ERR(sdev->bsg_dev) instead and not set
> the 'error' variable.
> 
> Guenter
> 
>> Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
>> Cc: Jens Axboe <axboe@kernel.dk>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>> ---
>>   v1->v2:
>>   resend to linux-scsi mail list.
>>
>>   drivers/scsi/scsi_sysfs.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index 86793259e541..d8789f6cda62 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -1379,6 +1379,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>>   			 * We're treating error on bsg register as non-fatal, so
>>   			 * pretend nothing went wrong.
>>   			 */
>> +			error = PTR_ERR(sdev->bsg_dev);
>>   			sdev_printk(KERN_INFO, sdev,
>>   				    "Failed to register bsg queue, errno=%d\n",
>>   				    error);
>> -- 
>> 2.25.1
>>

Re: [PATCH v2] scsi: bsg: fix errno when scsi_bsg_register_queue fails

[Guenter Roeck]

On 12/22/21 5:07 PM, Jackie Liu wrote:
> Hi Guenter.
> 
> Before commit ead09dd3aed5c ("scsi: bsg: Simplify device registration", the errno need return to the caller, I restore the old logic, and print
> this errno.
> 

The comment associated with the code says "We're treating error on bsg
register as non-fatal, so pretend nothing went wrong." Your commit does
not explain why that is wrong, and why the error should be returned
to the caller. In the current code, the comment is still there,
but the error is not ignored, and it is printed as informational message,
not as error message. At the very least that is misleading, and the code
no longer matches the comment. Also, the description in your commit does
not match the change made: It sounds like a change with no functional
impact ("Here, we should be print the correct error code when
scsi_bsg_register_queue fails") when in reality it does introduce
a functional change (the error is not only printed but also returned
to the caller).

Guenter

> -- 
> Jackie Liu
> 
> 在 2021/12/23 上午12:54, Guenter Roeck 写道:
>> On Fri, Oct 22, 2021 at 09:02:01AM +0800, Jackie Liu wrote:
>>> From: Jackie Liu <liuyun01@kylinos.cn>
>>>
>>> When the value of error is printed, it will always be 0. Here, we should be
>>> print the correct error code when scsi_bsg_register_queue fails.
>>>
>>
>> The comment above the changed code says:
>>
>> "
>> We're treating error on bsg register as non-fatal, so pretend nothing went wrong.
>> "
>>
>> With this patch in place, "error" is returned to the caller, and the code
>> no longer pretends that nothing is wrong. Also, the message is a dev_info
>> message, not dev_err, suggesting that ignoring the error was indeed on
>> purpose. Assuming the comment is correct, this patch is plain wrong;
>> the message should have printed PTR_ERR(sdev->bsg_dev) instead and not set
>> the 'error' variable.
>>
>> Guenter
>>
>>> Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
>>> Cc: Jens Axboe <axboe@kernel.dk>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>> ---
>>>   v1->v2:
>>>   resend to linux-scsi mail list.
>>>
>>>   drivers/scsi/scsi_sysfs.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>> index 86793259e541..d8789f6cda62 100644
>>> --- a/drivers/scsi/scsi_sysfs.c
>>> +++ b/drivers/scsi/scsi_sysfs.c
>>> @@ -1379,6 +1379,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>>>                * We're treating error on bsg register as non-fatal, so
>>>                * pretend nothing went wrong.
>>>                */
>>> +            error = PTR_ERR(sdev->bsg_dev);
>>>               sdev_printk(KERN_INFO, sdev,
>>>                       "Failed to register bsg queue, errno=%d\n",
>>>                       error);
>>> -- 
>>> 2.25.1
>>>

Re: [PATCH v2] scsi: bsg: fix errno when scsi_bsg_register_queue fails

[Jackie Liu]

在 2021/12/23 上午9:35, Guenter Roeck 写道:
> On 12/22/21 5:07 PM, Jackie Liu wrote:
>> Hi Guenter.
>>
>> Before commit ead09dd3aed5c ("scsi: bsg: Simplify device 
>> registration", the errno need return to the caller, I restore the old 
>> logic, and print
>> this errno.
>>
> 
> The comment associated with the code says "We're treating error on bsg
> register as non-fatal, so pretend nothing went wrong." Your commit does
> not explain why that is wrong, and why the error should be returned
> to the caller. In the current code, the comment is still there,
> but the error is not ignored, and it is printed as informational message,
> not as error message. At the very least that is misleading, and the code
> no longer matches the comment. Also, the description in your commit does
> not match the change made: It sounds like a change with no functional
> impact ("Here, we should be print the correct error code when
> scsi_bsg_register_queue fails") when in reality it does introduce
> a functional change (the error is not only printed but also returned
> to the caller).
> 
> Guenter

I see, Thanks for point out, after commit ee37e09d81a4 ("[SCSI] fix
duplicate removal on error path in scsi_sysfs_add_sdev"), Before this
errno will be forced to return 0.

After:

[1] error = device_create_file(&sdev->sdev_gendev,  	
                            sdev->host->hostt->sdev_attrs[i]);

Then:

with 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier")
delete code [1], so we force return errno.

I don’t know if I should restore the original logic or delete
this comment information. Guenter and Christoph, What do you think? I
can send another patch based on this.

--
Jackie Liu

> 
>> -- 
>> Jackie Liu
>>
>> 在 2021/12/23 上午12:54, Guenter Roeck 写道:
>>> On Fri, Oct 22, 2021 at 09:02:01AM +0800, Jackie Liu wrote:
>>>> From: Jackie Liu <liuyun01@kylinos.cn>
>>>>
>>>> When the value of error is printed, it will always be 0. Here, we 
>>>> should be
>>>> print the correct error code when scsi_bsg_register_queue fails.
>>>>
>>>
>>> The comment above the changed code says:
>>>
>>> "
>>> We're treating error on bsg register as non-fatal, so pretend nothing 
>>> went wrong.
>>> "
>>>
>>> With this patch in place, "error" is returned to the caller, and the 
>>> code
>>> no longer pretends that nothing is wrong. Also, the message is a 
>>> dev_info
>>> message, not dev_err, suggesting that ignoring the error was indeed on
>>> purpose. Assuming the comment is correct, this patch is plain wrong;
>>> the message should have printed PTR_ERR(sdev->bsg_dev) instead and 
>>> not set
>>> the 'error' variable.
>>>
>>> Guenter
>>>
>>>> Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
>>>> Cc: Jens Axboe <axboe@kernel.dk>
>>>> Cc: Christoph Hellwig <hch@lst.de>
>>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>> ---
>>>>   v1->v2:
>>>>   resend to linux-scsi mail list.
>>>>
>>>>   drivers/scsi/scsi_sysfs.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>>> index 86793259e541..d8789f6cda62 100644
>>>> --- a/drivers/scsi/scsi_sysfs.c
>>>> +++ b/drivers/scsi/scsi_sysfs.c
>>>> @@ -1379,6 +1379,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>>>>                * We're treating error on bsg register as non-fatal, so
>>>>                * pretend nothing went wrong.
>>>>                */
>>>> +            error = PTR_ERR(sdev->bsg_dev);
>>>>               sdev_printk(KERN_INFO, sdev,
>>>>                       "Failed to register bsg queue, errno=%d\n",
>>>>                       error);
>>>> -- 
>>>> 2.25.1
>>>>
> 

Re: [PATCH v2] scsi: bsg: fix errno when scsi_bsg_register_queue fails

[Jackie Liu]

在 2021/12/23 上午10:42, Jackie Liu 写道:
> 
> 
> 在 2021/12/23 上午9:35, Guenter Roeck 写道:
>> On 12/22/21 5:07 PM, Jackie Liu wrote:
>>> Hi Guenter.
>>>
>>> Before commit ead09dd3aed5c ("scsi: bsg: Simplify device 
>>> registration", the errno need return to the caller, I restore the old 
>>> logic, and print
>>> this errno.
>>>
>>
>> The comment associated with the code says "We're treating error on bsg
>> register as non-fatal, so pretend nothing went wrong." Your commit does
>> not explain why that is wrong, and why the error should be returned
>> to the caller. In the current code, the comment is still there,
>> but the error is not ignored, and it is printed as informational message,
>> not as error message. At the very least that is misleading, and the code
>> no longer matches the comment. Also, the description in your commit does
>> not match the change made: It sounds like a change with no functional
>> impact ("Here, we should be print the correct error code when
>> scsi_bsg_register_queue fails") when in reality it does introduce
>> a functional change (the error is not only printed but also returned
>> to the caller).
>>
>> Guenter
> 
> I see, Thanks for point out, after commit ee37e09d81a4 ("[SCSI] fix
> duplicate removal on error path in scsi_sysfs_add_sdev"), Before this
> errno will be forced to return 0.
> 
> After:
> 
> [1] error = device_create_file(&sdev->sdev_gendev,
>                             sdev->host->hostt->sdev_attrs[i]);
> 
> Then:
> 
> with 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier")
> delete code [1], so we force return errno.
> 
> I don’t know if I should restore the original logic or delete
> this comment information. Guenter and Christoph, What do you think? I
> can send another patch based on this.

Hi, Guenter

I saw that you have already posted a fix patch, I suggest that the
analysis I mentioned can be added to the description. I think the Fixes 
tag should be 92c4b58b15c5 instead of my patch.

In any case, if you are interested in re-sending a patch, you can add my
review tag.

Reviewed-by: Jackie Liu <liuyun01@kylinos.cn>


My broken mailbox couldn’t receive the mail from the mailing list, so I
sent it from here.

--
Jackie Liu

> 
> -- 
> Jackie Liu
> 
>>
>>> -- 
>>> Jackie Liu
>>>
>>> 在 2021/12/23 上午12:54, Guenter Roeck 写道:
>>>> On Fri, Oct 22, 2021 at 09:02:01AM +0800, Jackie Liu wrote:
>>>>> From: Jackie Liu <liuyun01@kylinos.cn>
>>>>>
>>>>> When the value of error is printed, it will always be 0. Here, we 
>>>>> should be
>>>>> print the correct error code when scsi_bsg_register_queue fails.
>>>>>
>>>>
>>>> The comment above the changed code says:
>>>>
>>>> "
>>>> We're treating error on bsg register as non-fatal, so pretend 
>>>> nothing went wrong.
>>>> "
>>>>
>>>> With this patch in place, "error" is returned to the caller, and the 
>>>> code
>>>> no longer pretends that nothing is wrong. Also, the message is a 
>>>> dev_info
>>>> message, not dev_err, suggesting that ignoring the error was indeed on
>>>> purpose. Assuming the comment is correct, this patch is plain wrong;
>>>> the message should have printed PTR_ERR(sdev->bsg_dev) instead and 
>>>> not set
>>>> the 'error' variable.
>>>>
>>>> Guenter
>>>>
>>>>> Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
>>>>> Cc: Jens Axboe <axboe@kernel.dk>
>>>>> Cc: Christoph Hellwig <hch@lst.de>
>>>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>> ---
>>>>>   v1->v2:
>>>>>   resend to linux-scsi mail list.
>>>>>
>>>>>   drivers/scsi/scsi_sysfs.c | 1 +
>>>>>   1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>>>> index 86793259e541..d8789f6cda62 100644
>>>>> --- a/drivers/scsi/scsi_sysfs.c
>>>>> +++ b/drivers/scsi/scsi_sysfs.c
>>>>> @@ -1379,6 +1379,7 @@ int scsi_sysfs_add_sdev(struct scsi_device 
>>>>> *sdev)
>>>>>                * We're treating error on bsg register as non-fatal, so
>>>>>                * pretend nothing went wrong.
>>>>>                */
>>>>> +            error = PTR_ERR(sdev->bsg_dev);
>>>>>               sdev_printk(KERN_INFO, sdev,
>>>>>                       "Failed to register bsg queue, errno=%d\n",
>>>>>                       error);
>>>>> -- 
>>>>> 2.25.1
>>>>>
>>

Re: [PATCH v2] scsi: bsg: fix errno when scsi_bsg_register_queue fails

[Christoph Hellwig]

On Thu, Dec 23, 2021 at 10:42:30AM +0800, Jackie Liu wrote:
> I see, Thanks for point out, after commit ee37e09d81a4 ("[SCSI] fix
> duplicate removal on error path in scsi_sysfs_add_sdev"), Before this
> errno will be forced to return 0.
>
> After:
>
> [1] error = device_create_file(&sdev->sdev_gendev,  	
>                            sdev->host->hostt->sdev_attrs[i]);
>
> Then:
>
> with 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier")
> delete code [1], so we force return errno.
>
> I don’t know if I should restore the original logic or delete
> this comment information. Guenter and Christoph, What do you think? I
> can send another patch based on this.

I think we should just handle the error properly and remove the comment.
There's no good reason to ignore bsg registration errors.