Fix reference counting for failed SCSI devices

whenever the scsi-ml tries to scan non-existent devices the reference count in scsi_alloc_sdev() and scsi_probe_and_add_lun() is not adjusted properly. Every call to XXX_initialize in the driver core sets the reference count to 1, so for a proper deallocation an explicit XXX_put() has to be done.

Quoted 5 lines · Hannes Reinecke

whenever the scsi-ml tries to scan non-existent devices the reference count in scsi_alloc_sdev() and scsi_probe_and_add_lun() is not adjusted properly. Every call to XXX_initialize in the driver core sets the reference count to 1, so for a proper deallocation an explicit XXX_put() has to be done.

That's true, but I don't see what the problem is if the device has never been made visible.

Review

Quoted 1 line

(quoted diff) +1 -0

+			put_device(&starget->dev);

this would amount to a double put, since the parent put method is called in the device release.

Review

Quoted 1 line

(quoted diff) +1 -0

+	class_device_put(&sdev->sdev_classdev);

This is unnecessary since the class device is simply occupying a private area in the scsi_device. As long as its never made visible to the system, its refcount is irrelevant

Quoted 5 lines

	put_device(&sdev->sdev_gendev);

out:
if (display_failure_msg)

(quoted diff) +0 -0

@@ -855,6 +857,8 @@ static int scsi_probe_and_add_lun(struct
 		if (sdev->host->hostt->slave_destroy)

Review static int scsi_probe_and_add_lun(struct · line 861

Quoted 4 lines

(quoted diff) +2 -0

 			sdev->host->hostt->slave_destroy(sdev);
 		transport_destroy_device(&sdev->sdev_gendev);
+		class_device_put(&sdev->sdev_classdev);
+		put_device(sdev->sdev_gendev.parent);

same should apply here. As long as this cascade occurs before scsi_add_lun() (which calls scsi_sysfs_add_sdev()), which is what makes the whole set of devices and classes visible.

Quoted 6 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	class_device_put(&sdev->sdev_classdev);

This is unnecessary since the class device is simply occupying a private area in the scsi_device. As long as its never made visible to the system, its refcount is irrelevant

It's not. Whenever you try to rmmod the adapter it becomes highly relevant. If it doesn't crash you've at least generated a memleak as the class device is never freed.
(And these are quite a few for Wide-SCSI Double-channel adapters ...)

Quoted 9 lines · Hannes Reinecke

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	class_device_put(&sdev->sdev_classdev);

This is unnecessary since the class device is simply occupying a private area in the scsi_device. As long as its never made visible to the system, its refcount is irrelevant

It's not. Whenever you try to rmmod the adapter it becomes highly relevant. If it doesn't crash you've at least generated a memleak as the class device is never freed.
(And these are quite a few for Wide-SCSI Double-channel adapters ...)

? Look at the code; you're not doing a put on a pointer to the sdev_classdev, you're doing a put on a reference to it.

It's defined in scsi_device.h:

struct scsi_device {
	...
	struct class_device sdev_classdev;
	...
};

so it's contained within the scsi_device. Freeing the scsi_device frees the classdev (and the gendev).

Quoted 3 lines

so it's contained within the scsi_device. Freeing the scsi_device frees the classdev (and the gendev).

But does not call the ->release function.

Put it the other way round: does 'rmmod aic7xxx' work for you? It certainly did _not_ work for aic79xx, hence the fix.

Quoted 4 lines · Hannes Reinecke

Quoted 3 lines

so it's contained within the scsi_device. Freeing the scsi_device frees the classdev (and the gendev).

But does not call the ->release function.

Please just read the code like I asked. If you do, you'll find that the sdev_classdev release method is NULL until scsi_sysfs_add_sdev() precisely for the reason that the class references don't matter until that point. We're free to kill the whole thing without bothering about the class devices until scsi_add_lun detects something and calls scsi_sysfs_add_sdev() to make the whole thing visible. Then all classdevs get a ref on the parent gendev which their release method relinquishes.

Quoted 2 lines

Put it the other way round: does 'rmmod aic7xxx' work for you? It certainly did _not_ work for aic79xx, hence the fix.

Well, I know aic7xxx works perfectly on a dual channel card, because I actually test the failure paths and insmod/rmmod is one of my tests. I can't comment on aic79xx because I don't have the hardware.

I guess it's time to convert aic79xx to the spi_transport class. Unfortunately my attempt segfaults when removing a device in attribute_container_device_trigger(); someone is overwriting the ->match function. Oh well, further debugging needed.

Large segments of the aic79xx driver are identical to the aic7xxx driver except that all the functions being ahd_ instead of ahc_, so you should just be able to mirror quite a lot of the aic7xxx updates. However, you need to include a large number of rather nasty u320 parameters in the SPI transport Class (and make sure they're coupled correctly) to get domain validation to work ... the transport class has also only been tested on speeds up to u160, so going to u320 will be a first for it too...