On 19/11/2022 08:58, Yang Yingliang wrote:
Personally I prefer 1. However did you develop a prototype patch for
how 2. would look? And how many changes are still required for 1.?
For 1, in total, there are 8 places need be checked
in drivers/scsi/scsi_transport_sas.c, 2 places
in drivers/scsi/scsi_sysfs.c, 3 places
in drivers/scsi/scsi_transport_fc.c, 2 places
in drivers/scsi/scsi_transport_srp.c, 1 place
and in linux-next there are 4x places which do already check the return
code...
Not sure what's best to do. I'll leave it to James' wisdom.
However, we do seem to have a common pattern:
error = device_add(dev);
if (error)
return error;
transport_add_device(dev);
transport_configure_device(dev);
Could we make an "add" (which does as above pattern) and "remove"
helper? It might simplify things such that we not only fixing the
possible crash but also reducing code.
Thanks,
John
For 2, I think we can use device_is_registered() to check if add
operation is successful, may be like this (not test yet):
diff --git a/drivers/base/transport_class.c
b/drivers/base/transport_class.c
index ccc86206e508..ac41be7b724e 100644
--- a/drivers/base/transport_class.c
+++ b/drivers/base/transport_class.c
@@ -227,9 +227,11 @@ static int transport_remove_classdev(struct
attribute_container *cont,
tclass->remove(tcont, dev, classdev);
if (tclass->remove != anon_transport_dummy_function) {
- if (tcont->statistics)
- sysfs_remove_group(&classdev->kobj, tcont->statistics);
- attribute_container_class_device_del(classdev);
+ if (device_is_registered(classdev)) {
+ if (tcont->statistics)
+ sysfs_remove_group(&classdev->kobj, tcont->statistics);
+ attribute_container_class_device_del(classdev);
+ }
}
return 0;
