在 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@xxxxxxxxxx>
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@xxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Signed-off-by: Jackie Liu <liuyun01@xxxxxxxxxx>
---
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
