references on sdev_class & shost_class
Hi Mike,
what's the rationale behind all this class_get's on the scsi host and device classes? I stumbled over that code when merging up the final bits for proper scsi_device reference counting and it looks strange.
If we every don't get a reference to the class in scsi_device_put or scsi_remove_device we leak references to scsi_devices which would be very bad.
But how could we actually fail to acquire a reference? This means scsi_mod would be unloading which seems impossible as all scsi hosts and devices are allocated by modules that use symbols from scsi_mod.
Should we just rip this stuff out like below?
drivers/scsi/hosts.c +8 -11
@@ -323,23 +323,20 @@**/ struct Scsi_Host *scsi_host_lookup (unsigned short hostnum) { -struct class *class =class_get (&shost_class); +struct class *class = &shost_class;struct class_device *cdev;struct Scsi_Host *shost =ERR_PTR (-ENXIO ), *p; -if (class) { -down_read (&class->subsys.rwsem); -list_for_each_entry (cdev, &class->children, node) { - p =class_to_shost (cdev); -if (p->host_no == hostnum) { - shost =scsi_host_get (p); -break ; - } +down_read (&class->subsys.rwsem); +list_for_each_entry (cdev, &class->children, node) { + p =class_to_shost (cdev); +if (p->host_no == hostnum) { + shost =scsi_host_get (p); +break ; } -up_read (&class->subsys.rwsem); } +up_read (&class->subsys.rwsem); -class_put (&shost_class);return shost; }
drivers/scsi/scsi.c +0 -12
@@ -885,10 +885,6 @@int scsi_device_get (struct scsi_device *sdev) { -struct class *class =class_get (&sdev_class); - -if (!class) - goto out;if (test_bit (SDEV_DEL , &sdev->sdev_state)) goto out;if (!try_module_get (sdev->host->hostt->module))
@@ -896,29 +892,21 @@if (!get_device (&sdev->sdev_gendev)) goto out_put_module;atomic_inc (&sdev->access_count); -class_put (&sdev_class);return 0 ; out_put_module:module_put (sdev->host->hostt->module); out: -class_put (&sdev_class);return -ENXIO ; }void scsi_device_put (struct scsi_device *sdev) { -struct class *class =class_get (&sdev_class); - -if (!class) -return ; -module_put (sdev->host->hostt->module);if (atomic_dec_and_test (&sdev->access_count))if (test_bit (SDEV_DEL , &sdev->sdev_state))device_del (&sdev->sdev_gendev);put_device (&sdev->sdev_gendev); -class_put (&sdev_class); }int scsi_device_cancel_cb (struct device *dev,void *data)
===== drivers/scsi/scsi_sysfs.c 1.33 vs edited =====
drivers/scsi/scsi_sysfs.c +5 -12
@@ -403,22 +403,15 @@**/ void scsi_remove_device (struct scsi_device *sdev) { -struct class *class =class_get (&sdev_class); -class_device_unregister (&sdev->sdev_classdev); -if (class) { -down_write (&class->subsys.rwsem); -set_bit (SDEV_DEL , &sdev->sdev_state); -if (sdev->host->hostt->slave_destroy) - sdev->host->hostt->slave_destroy (sdev); -if (atomic_read (&sdev->access_count)) -device_del (&sdev->sdev_gendev); -up_write (&class->subsys.rwsem); - } +set_bit (SDEV_DEL , &sdev->sdev_state); +if (sdev->host->hostt->slave_destroy) + sdev->host->hostt->slave_destroy (sdev); +if (atomic_read (&sdev->access_count)) +device_del (&sdev->sdev_gendev);put_device (&sdev->sdev_gendev); -class_put (&sdev_class); }
Yes, It probably can be taken as you stated. I was just following the model of get a ref on a object before we use it. Like you also indicated the only reason it should fail is if scsi_mod was unloaded. If there was a problem we would probably oops on a dereference instead of failing one of the NULL returning checks in the call chain of class_get.
Quoted 116 lines · Christoph Hellwig [[email protected]] wrote:
Hi Mike,
what's the rationale behind all this class_get's on the scsi host and device classes? I stumbled over that code when merging up the final bits for proper
scsi_devicereference counting and it looks strange.If we every don't get a reference to the class in
scsi_device_putorscsi_remove_devicewe leak references toscsi_deviceswhich would be very bad.But how could we actually fail to acquire a reference? This means
scsi_modwould be unloading which seems impossible as all scsi hosts and devices are allocated by modules that use symbols fromscsi_mod.Should we just rip this stuff out like below?
drivers/scsi/hosts.c +8
-11@@ -323,23 +323,20 @@**/ struct Scsi_Host *scsi_host_lookup (unsigned short hostnum) { -struct class *class =class_get (&shost_class); +struct class *class = &shost_class;struct class_device *cdev;struct Scsi_Host *shost =ERR_PTR (-ENXIO ), *p; -if (class) { -down_read (&class->subsys.rwsem); -list_for_each_entry (cdev, &class->children, node) { - p =class_to_shost (cdev); -if (p->host_no == hostnum) { - shost =scsi_host_get (p); -break ; - } +down_read (&class->subsys.rwsem); +list_for_each_entry (cdev, &class->children, node) { + p =class_to_shost (cdev); +if (p->host_no == hostnum) { + shost =scsi_host_get (p); +break ; } -up_read (&class->subsys.rwsem); } +up_read (&class->subsys.rwsem); -class_put (&shost_class);return shost; }drivers/scsi/scsi.c +0
-12@@ -885,10 +885,6 @@int scsi_device_get (struct scsi_device *sdev) { -struct class *class =class_get (&sdev_class); - -if (!class) - goto out;if (test_bit (SDEV_DEL , &sdev->sdev_state)) goto out;if (!try_module_get (sdev->host->hostt->module))@@ -896,29 +892,21 @@if (!get_device (&sdev->sdev_gendev)) goto out_put_module;atomic_inc (&sdev->access_count); -class_put (&sdev_class);return 0 ; out_put_module:module_put (sdev->host->hostt->module); out: -class_put (&sdev_class);return -ENXIO ; }void scsi_device_put (struct scsi_device *sdev) { -struct class *class =class_get (&sdev_class); - -if (!class) -return ; -module_put (sdev->host->hostt->module);if (atomic_dec_and_test (&sdev->access_count))if (test_bit (SDEV_DEL , &sdev->sdev_state))device_del (&sdev->sdev_gendev);put_device (&sdev->sdev_gendev); -class_put (&sdev_class); }int scsi_device_cancel_cb (struct device *dev,void *data)=====
drivers/scsi/scsi_sysfs.c1.33 vs edited =====drivers/scsi/scsi_sysfs.c +1
-11@@ -403,22 +403,15 @@**/ void scsi_remove_device (struct scsi_device *sdev) { -struct class *class =class_get (&sdev_class); -class_device_unregister (&sdev->sdev_classdev); -if (class) { -down_write (&class->subsys.rwsem); -set_bit (SDEV_DEL , &sdev->sdev_state); -if (sdev->host->hostt->slave_destroy) - sdev->host->hostt->slave_destroy (sdev); -if (atomic_read (&sdev->access_count)) -device_del (&sdev->sdev_gendev); -up_write (&class->subsys.rwsem); - } +set_bit (SDEV_DEL , &sdev->sdev_state);
drivers/scsi/scsi_sysfs.c line 417
Quoted 10 lines
drivers/scsi/scsi_sysfs.c +4
-1+if (sdev->host->hostt->slave_destroy) + sdev->host->hostt->slave_destroy (sdev); +if (atomic_read (&sdev->access_count)) +device_del (&sdev->sdev_gendev);put_device (&sdev->sdev_gendev); -class_put (&sdev_class); }
-andmike
--
Michael Anderson
[email protected]