What should be done with wrong warning "please use bus_type methods." on sd, sr, st and osst?

DISCUSSION Eric Piel 3 participants 6 messages Mar 24, 2008 – Mar 25, 2008

Subsystems (2)

SCSI Driver core

lore

Share & tools

lore

Eric Piel

author

2008-03-24 14:00 UTC #0 lore raw

Hello,

Since commit 751bf4d7865e4ced406be93b04c7436d866d3684 (scsi_sysfs: restore prep_fn when ULD is removed), the warning "Driver '%s' needs updating - please use bus_type methods." is generated for several scsi drivers (sr, sg, st and osst). It does so because it thinks that the driver remove() functions will not be called (cf __device_release_driver()). Actually, they are called by scsi_bus_remove().

This has been noted already a couple of times [1] [2] [3], but it seems absolutely nothing conclusive came out of the reports.

What should be done? Delete the warning? Change it so it doesn't get triggered if drv->bus->remove == scsi_bus_remove? Merge scsi_bus_remove() into the driver remove() functions? Disregard the monthly warming reports?

Eric

[1] http://lkml.org/lkml/2008/1/10/47
[2] http://lkml.org/lkml/2008/2/20/547
[3] http://lkml.org/lkml/2008/3/22/198

James Bottomley

maintainer

2008-03-24 15:24 UTC #1 lore raw

Quoted 7 lines · Eric Piel

Since commit 751bf4d7865e4ced406be93b04c7436d866d3684 (scsi_sysfs:
restore prep_fn when ULD is removed), the warning "Driver '%s' needs updating - please use bus_type methods." is generated for several scsi drivers (sr, sg, st and osst). It does so because it thinks that the driver remove() functions will not be called (cf __device_release_driver()). Actually, they are called by scsi_bus_remove().

Quoted 7 lines

This has been noted already a couple of times [1] [2] [3], but it seems absolutely nothing conclusive came out of the reports.
What should be done? Delete the warning? Change it so it doesn't get triggered if drv->bus->remove == scsi_bus_remove? Merge scsi_bus_remove() into the driver remove() functions? Disregard the monthly warming reports?

At least for SCSI, the warning is wrong .. the drivers are properly converted. The issue that triggers the warning is that we use the bus_type power management methods to receive the notifies in the mid-layer (where we do mid-layer specific stuff) then we notify the drivers through the driver power management methods.

A solution would be to duplicate the power management methods in the scsi_driver structure, but this is a complete waste of space since the generic driver ones aren't going away (at least according to Kay and Greg). I still think the best thing to do is just to turn off this spurious warning.

James

Greg KH

2008-03-24 17:54 UTC #2 lore raw

Quoted 23 lines · James Bottomley

Quoted 7 lines · Eric Piel
Since commit 751bf4d7865e4ced406be93b04c7436d866d3684 (scsi_sysfs:
restore prep_fn when ULD is removed), the warning "Driver '%s' needs updating - please use bus_type methods." is generated for several scsi drivers (sr, sg, st and osst). It does so because it thinks that the driver remove() functions will not be called (cf __device_release_driver()). Actually, they are called by scsi_bus_remove().
Quoted 7 lines
This has been noted already a couple of times [1] [2] [3], but it seems absolutely nothing conclusive came out of the reports.
What should be done? Delete the warning? Change it so it doesn't get triggered if drv->bus->remove == scsi_bus_remove? Merge scsi_bus_remove() into the driver remove() functions? Disregard the monthly warming reports?
At least for SCSI, the warning is wrong .. the drivers are properly converted. The issue that triggers the warning is that we use the bus_type power management methods to receive the notifies in the mid-layer (where we do mid-layer specific stuff) then we notify the drivers through the driver power management methods.

Quoted 5 lines

A solution would be to duplicate the power management methods in the scsi_driver structure, but this is a complete waste of space since the generic driver ones aren't going away (at least according to Kay and Greg). I still think the best thing to do is just to turn off this spurious warning.

Do you have a patch that can detect the usage that you currently have so that I can change the warning message to not trigger if things are set up that way instead?

thanks,

greg k-h

James Bottomley

maintainer

2008-03-24 18:16 UTC #3 lore raw

Quoted 30 lines · Greg KH

Quoted 23 lines · James Bottomley
Quoted 7 lines · Eric Piel
Since commit 751bf4d7865e4ced406be93b04c7436d866d3684 (scsi_sysfs:
restore prep_fn when ULD is removed), the warning "Driver '%s' needs updating - please use bus_type methods." is generated for several scsi drivers (sr, sg, st and osst). It does so because it thinks that the driver remove() functions will not be called (cf __device_release_driver()). Actually, they are called by scsi_bus_remove().
Quoted 7 lines
This has been noted already a couple of times [1] [2] [3], but it seems absolutely nothing conclusive came out of the reports.
What should be done? Delete the warning? Change it so it doesn't get triggered if drv->bus->remove == scsi_bus_remove? Merge scsi_bus_remove() into the driver remove() functions? Disregard the monthly warming reports?
At least for SCSI, the warning is wrong .. the drivers are properly converted. The issue that triggers the warning is that we use the bus_type power management methods to receive the notifies in the mid-layer (where we do mid-layer specific stuff) then we notify the drivers through the driver power management methods.
Quoted 5 lines
A solution would be to duplicate the power management methods in the scsi_driver structure, but this is a complete waste of space since the generic driver ones aren't going away (at least according to Kay and Greg). I still think the best thing to do is just to turn off this spurious warning.

Quoted 3 lines

Do you have a patch that can detect the usage that you currently have so that I can change the warning message to not trigger if things are set up that way instead?

Well, my suggested fix would be the attached one since you and Kay seem to be telling me that converting to bus_type X methods still leaves us free to reuse the driver X methods. If you're planning on deprecating the driver X methods, then sure, it makes sense for me to duplicate them in the scsi driver.

James

---

drivers/base/driver.c +0 -5

index bf31a01..bc9f76d 100644
@@ -215,11 +215,6 @@ int driver_register(struct device_driver *drv)
 {
 	int ret;
 
-	if ((drv->bus->probe && drv->probe) ||
-	    (drv->bus->remove && drv->remove) ||
-	    (drv->bus->shutdown && drv->shutdown))
-		printk(KERN_WARNING "Driver '%s' needs updating - please use "
-			"bus_type methods\n", drv->name);
 	ret = bus_add_driver(drv);
 	if (ret)
 		return ret;

Éric Piel

author

2008-03-25 22:21 UTC #4 lore raw

Quoted 11 lines · James Bottomley wrote/

Quoted 9 lines · Greg KH
Quoted 5 lines · James Bottomley
A solution would be to duplicate the power management methods in the scsi_driver structure, but this is a complete waste of space since the generic driver ones aren't going away (at least according to Kay and Greg). I still think the best thing to do is just to turn off this spurious warning.
Do you have a patch that can detect the usage that you currently have so that I can change the warning message to not trigger if things are set up that way instead?

Quoted 5 lines

Well, my suggested fix would be the attached one since you and Kay seem to be telling me that converting to bus_type X methods still leaves us free to reuse the driver X methods. If you're planning on deprecating the driver X methods, then sure, it makes sense for me to duplicate them in the scsi driver.

I guess the problem with removing the warning is that in some other cases it could really be useful (searching on the web seems to show a couple of true positives). I think Greg was more suggesting like adding a flag ".i_know_what_i_am_doing" somewhere and putting it to 1 to disable the warning.

Anyway, if the driver X methods are meaning something else, it makes sense to duplicate them specifically in the scsi driver structure. We are basically talking about 8 bytes per scsi device, which can be considered a fair trade-off if it allows to detect bugs in other places of the kernel. Following is an example of patch.

Eric
PS: Probably I'm an idiot, for the patch I didn't understand how to move ".remove" to scsi_driver, so I moved it to scsi_device... anyway it's just an example in order to be sure that everyone is talking about the same thing.

---

drivers/scsi/scsi_sysfs.c +2 -2

index b9b09a7..7d29099 100644
@@ -384,8 +384,8 @@ static int scsi_bus_remove(struct device *dev)
 	 * driver may have altered it and it's being removed */
 	blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
 
-	if (drv && drv->remove)
-		err = drv->remove(dev);
+	if (sdev->remove)
+		err = sdev->remove(dev);
 
 	return 0;
 }

drivers/scsi/sr.c +1 -1

index 7ee86d4..da6adfd 100644
@@ -83,7 +83,6 @@ static struct scsi_driver sr_template = {
 	.gendrv = {
 		.name   	= "sr",
 		.probe		= sr_probe,
-		.remove		= sr_remove,
 	},
 	.done			= sr_done,
 };

@@ -635,6 +634,7 @@ static int sr_probe(struct device *dev)
 	sprintf(cd->cdi.name, "sr%d", minor);
 
 	sdev->sector_size = 2048;	/* A guess, just in case */
+	sdev->remove = sr_remove;
 
 	/* FIXME: need to handle a get_capabilities failure properly ?? */
 	get_capabilities(cd);

include/scsi/scsi_device.h +1 -0

index ab7acbe..0809a0b 100644
@@ -158,6 +158,7 @@ struct scsi_device {
 
 	struct device		sdev_gendev;
 	struct class_device	sdev_classdev;
+	int (*remove)		(struct device *);
 
 	struct execute_work	ew; /* used to get process context on put */

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

maintainer

2008-03-25 22:34 UTC #5 lore raw

Quoted 18 lines · Éric Piel

Quoted 11 lines · James Bottomley wrote/
Quoted 9 lines · Greg KH
Quoted 5 lines · James Bottomley
A solution would be to duplicate the power management methods in the scsi_driver structure, but this is a complete waste of space since the generic driver ones aren't going away (at least according to Kay and Greg). I still think the best thing to do is just to turn off this spurious warning.
Do you have a patch that can detect the usage that you currently have so that I can change the warning message to not trigger if things are set up that way instead?
Quoted 5 lines
Well, my suggested fix would be the attached one since you and Kay seem to be telling me that converting to bus_type X methods still leaves us free to reuse the driver X methods. If you're planning on deprecating the driver X methods, then sure, it makes sense for me to duplicate them in the scsi driver.

Quoted 5 lines

I guess the problem with removing the warning is that in some other cases it could really be useful (searching on the web seems to show a couple of true positives). I think Greg was more suggesting like adding a flag ".i_know_what_i_am_doing" somewhere and putting it to 1 to disable the warning.

Sure, but I just see all the fallout from the false positive on SCSI (Like about one email a week suggesting that I fix it), so I'm complaining about my particular piece of this.

Quoted 5 lines

Anyway, if the driver X methods are meaning something else, it makes sense to duplicate them specifically in the scsi driver structure. We are basically talking about 8 bytes per scsi device, which can be considered a fair trade-off if it allows to detect bugs in other places of the kernel. Following is an example of patch.

Well, what I'd like is to establish whether this usage is correct. I do think that if the probe and remove methods aren't going away, then it is,

Quoted 4 lines

PS: Probably I'm an idiot, for the patch I didn't understand how to move ".remove" to scsi_driver, so I moved it to scsi_device... anyway it's just an example in order to be sure that everyone is talking about the same thing.

No ... it's a reasonable approach. The struct scsi_device isn't quite the right place to do it ... we have one struct scsi_driver for each ULD (that's sd, sr, st etc,) We have one struct scsi_device for every device you have in the system (which can be thousands in an enterprise system), so putting the method in struct scsi_device is a bit of duplication overkill, but moving it to struct scsi_driver is very feasible.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html