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

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

[Eric Piel]

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

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

[James Bottomley]

On Mon, 2008-03-24 at 15:00 +0100, Eric Piel wrote:
> 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?

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

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

[Greg KH]

On Mon, Mar 24, 2008 at 10:24:07AM -0500, James Bottomley wrote:
> 
> On Mon, 2008-03-24 at 15:00 +0100, Eric Piel wrote:
> > 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?
> 
> 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.

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

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

[James Bottomley]

On Mon, 2008-03-24 at 10:59 -0700, Greg KH wrote:
> On Mon, Mar 24, 2008 at 10:24:07AM -0500, James Bottomley wrote:
> > 
> > On Mon, 2008-03-24 at 15:00 +0100, Eric Piel wrote:
> > > 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?
> > 
> > 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.
> 
> 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

---

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index bf31a01..bc9f76d 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -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;

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

[Éric Piel]

24/03/08 19:16, James Bottomley wrote/a écrit:
> On Mon, 2008-03-24 at 10:59 -0700, Greg KH wrote:
>> On Mon, Mar 24, 2008 at 10:24:07AM -0500, James Bottomley wrote:
>>> 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?
> 
> 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.

---
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b9b09a7..7d29099 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -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;
 }
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7ee86d4..da6adfd 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -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);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index ab7acbe..0809a0b 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[James Bottomley]

On Tue, 2008-03-25 at 23:20 +0100, Éric Piel wrote:
> 24/03/08 19:16, James Bottomley wrote/a écrit:
> > On Mon, 2008-03-24 at 10:59 -0700, Greg KH wrote:
> >> On Mon, Mar 24, 2008 at 10:24:07AM -0500, James Bottomley wrote:
> >>> 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?
> > 
> > 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.

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.

> 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,

> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html