Re: [PATCH] get rid of ->detect for upper layer drivers

Re: [PATCH] get rid of ->detect for upper layer drivers

[Adam J. Richter]

>adam@yggdrasil.com said:
>> 	I would prefer bus_type.match because it slightly reduces the
>> overhead of another patch that I posted to the generic device code:
>> two fields in struct device_driver for having the generic device layer
>> do the initial kmalloc and dma_alloc_consistent for the device (and do
>> the failure if these allocations fail).  This can greatly reduce the
>> number of rarely tested error branches (e.g., alloc_disk failure in
>> st.c does not free buffer) because of the number of drivers that can
>> be cleaned up a bit by it: hundreds of drivers, thousands of lines of
>> code. 

>This worries me: dma_alloc_consistent will always fail on fully incoherent 
>architectures (like some PA-RISC ones), then you have to fall back to doing 
>the writeback, invalidates in the driver (which we can't abstract out because 
>the only the driver knows the sync points).  You can't really treat 
>dma_alloc_consistent failure as anything other than an indication that you 
>have to use a different dma access method.

>Since dma_alloc_consistent cannot properly fail on an x86 (unless 
>__get_free_pages does), do we really want to clutter up the main line with 
>lots of fallbacks that would be useless on the predominant architecture?

	All of the drivers that I've looked at abort if the initial
pci_alloc_consistent fails.  Those are the drivers that I had in mind
to use this facility.

	I'd be interested in pointers to drivers that soldier on after
{pci,sbus}_alloc_consistent fails (I think those are the only two
versions in 2.5.46).

	Bear in mind that these things are optional facilities, so
drivers that do not want to use them do not have to.

	If there really are a number of device drivers that want to
be able to continue on if the initial dma_alloc_consistent fails,
especially if they all do the same thing, like, say, try vmalloc,
then, yes I would like add a flag to support and a flag in struct
device or something to communicate the result of the memory allocation,
rather than have the same logic replicated over N device drivers.
It could be conditionally compiled and probably put into a separate
inline routine in drivers/base/bus.c if needed for cleanliness.

	Sorry I don't have time to write a more detailed response.  I
have to leave now and may not have much internet access for the next
ten days.  So, please excuse me if I'm not able to respond further for
a while.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: [PATCH] get rid of ->detect for upper layer drivers

[J.E.J. Bottomley]

adam@yggdrasil.com said:
> 	All of the drivers that I've looked at abort if the initial
> pci_alloc_consistent fails.  Those are the drivers that I had in mind
> to use this facility. 

That's OK on x86, or other fully consistent arch, because it means that 
__get_free_pages failed and we're out of memory.

My worry is that by making this into a mid layer API we're implying that we 
"get it right", rather than get it right for only x86.

> 	I'd be interested in pointers to drivers that soldier on after
> {pci,sbus}_alloc_consistent fails (I think those are the only two
> versions in 2.5.46). 

You're welcome to look through 53c700.c.  It drives the lasi chips on PA-RISC 
and also some odd controllers under x86.  It's actually got a compile flag 
(CONFIG_53C700_USE_CONSISTENT) to check whether it's supposed to do consistent 
allocations (it also has the full fallback stuff for if it fails).  The real 
design of the flag is to optimize out the unnecessary fallback checks on x86.

You can see from the sync points peppering the driver how difficult it would 
be to do a full abstraction.

The ncr8xxx driver is another one used for the Zalon controller in parisc, so 
it will eventually have the same issues.

> 	If there really are a number of device drivers that want to be able
> to continue on if the initial dma_alloc_consistent fails, especially
> if they all do the same thing, like, say, try vmalloc, then, yes I
> would like add a flag to support and a flag in struct device or
> something to communicate the result of the memory allocation, rather
> than have the same logic replicated over N device drivers. It could be
> conditionally compiled and probably put into a separate inline routine
> in drivers/base/bus.c if needed for cleanliness. 

A full abstraction would require the sync point functions that the 53c700 uses 
so that they can optimize away on unnecessary architectures.  If you're 
signing up to do this right, then I can try it out in the 53c700.  I still 
have two parisc machines, one which cannot do consistent allocations and one 
which can.

James

Re: [PATCH] get rid of ->detect for upper layer drivers

[Adam J. Richter]

	I'd like to respond to the claim that bus_type.match()
is inherently racy, as in the current PCI, USB and SCSI code.

	Bear in mind that I have to catch a plane shortly and will
have uncertain internet access for the next ten days.

	Regardless of whether you have a separate bus.match() routine
or you have the attach routine make that decision, a hot unplug can
occur at any time (or at least a request for one if you have an
architecture where the computer has to give the user permission to
remove the device).  So you need to have some locking to ensure that
the device address (e.g., SCSI unit number) that you are talking to
will not be reallocated by a subsequent hot plug event until the
software is ready to release it.  So, while routines like match() and
attach() may not have a guarantee that the device is still there, they
do have a guarantee that their efforts to communicate with the device
will fail in an orderly manner if it is not there.

	Also, if SCSI hot unplugging is a scheme where the user has to
wait for permission from the computer, then a simple way to achieve
what I described in the previous paragarph would be to allocate some
kind of mutex around the call to device_register in scsi_add_lun.
Even without bus.match(), the individual device.attach() functions
would have to do this kind of locking or have it done for them further
up the call chain.  Probably some larger section of code would be
bracketed that way, perhaps further up the call chain than
scsi_add_lun.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: [PATCH] get rid of ->detect for upper layer drivers

[Adam J. Richter]

Michael Anderson wrote:
>Adam J. Richter [adam@freya.yggdrasil.com] wrote:
>> Michael Anderson wrote:
>> > The removal of detect should eventually result in other reductions /
>> > cleaner code (If the sg binding issue can be resolved cleanly).
>> 
>>       What makes you think that?  You need a way to ask "is this
>> driver interested in this device" and that's all that the code
>> remaining in scsi_device_template.detect() does after you remove the
>> side effects (i.e., the counter incrementing), and it does so about as
>> simply as possible.
>
>If we support the device probe and remove interfaces then as a device is
>registered with the scsi bus each drivers probe routine will be called
>and if it wants the device it returns 0. 
>
>If a driver is registered with the scsi bus post device probing then the
>driver_attach routine will call bus_match and driver probe for all
>devices on the bus not claimed by a driver.

	I know you can do driver matching with or without bus_type.match().

	I would prefer bus_type.match because it slightly reduces the
overhead of another patch that I posted to the generic device code:
two fields in struct device_driver for having the generic device layer
do the initial kmalloc and dma_alloc_consistent for the device (and do
the failure if these allocations fail).  This can greatly reduce the
number of rarely tested error branches (e.g., alloc_disk failure in
st.c does not free buffer) because of the number of drivers that can
be cleaned up a bit by it: hundreds of drivers, thousands of lines of
code.

	Without ->match(), these changes would result in some unnecessary
memory allocations and frees, and a lot of unncessary memory traffic as
I have the initial kmalloc zero filling the memory that it allocates.

	I'll address Christoph's point about bus_type.match() being
"racy" in a separate messsage.

	By the way, I also have other changes in my tree that I posted
ages ago that allows SCSI drivers to have the upper scsi layer
automatically set up the DMA mappings for each request's
scatter-gather list and/or sense buffer, along with a trivial change
that recasts non-gather-scatter requests as gather-scatter.  I also
have a trivial routine to clean up walking gather-scatter lists in
non-DMA drivers.  I was able to shrink a bunch of drivers with these
SCSI-specific facilities a year ago.  I want to move the sglist
mapping stuff up to the block layer and also add a facility for the
consistent DMA memory pool of stubs used to make the hardware-specific
gather-scatter lists.  Together, I believe these facilities will shrink
Linux devices dramatically.

[...]
>I believed the device_interface was the answer also. scsi generic is even mentioned \
>in the documentation. In talking with Mochel on this issue he disagreed and said that \
>this was not the interface to use. 

	I'd be interested in knowing what reasons he gave.

>The idea of a field in struct scsi_device seems like the right approach.
>It is just that the device model needs a better bus symmetric interface to
>handle cleanup and handling sg insmod / rmmod post device probing.

	Hmm, I'm not sure if this is what you're referring to, but it
looks like in 2.5.46, register_interface never walks the list of
devices that were already registered, and unregister_interface never
walks that list either (that is, intf->add_device is only called when
a new device is registered after the interface is loaded).  I wonder
if that difference in comparison to device_driver is intentional.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: [PATCH] get rid of ->detect for upper layer drivers

[J.E.J. Bottomley]

adam@yggdrasil.com said:
> 	I would prefer bus_type.match because it slightly reduces the
> overhead of another patch that I posted to the generic device code:
> two fields in struct device_driver for having the generic device layer
> do the initial kmalloc and dma_alloc_consistent for the device (and do
> the failure if these allocations fail).  This can greatly reduce the
> number of rarely tested error branches (e.g., alloc_disk failure in
> st.c does not free buffer) because of the number of drivers that can
> be cleaned up a bit by it: hundreds of drivers, thousands of lines of
> code. 

This worries me: dma_alloc_consistent will always fail on fully incoherent 
architectures (like some PA-RISC ones), then you have to fall back to doing 
the writeback, invalidates in the driver (which we can't abstract out because 
the only the driver knows the sync points).  You can't really treat 
dma_alloc_consistent failure as anything other than an indication that you 
have to use a different dma access method.

Since dma_alloc_consistent cannot properly fail on an x86 (unless 
__get_free_pages does), do we really want to clutter up the main line with 
lots of fallbacks that would be useless on the predominant architecture?

James

Re: [PATCH] get rid of ->detect for upper layer drivers

[Mike Anderson]

Adam J. Richter [adam@yggdrasil.com] wrote:
> 	I know you can do driver matching with or without bus_type.match().

Well you at least need a match function or device_attach / driver_attach
will do nothing.

> 
> 	I would prefer bus_type.match because it slightly reduces the
> overhead of another patch that I posted to the generic device code:
> two fields in struct device_driver for having the generic device layer
> do the initial kmalloc and dma_alloc_consistent for the device (and do
> the failure if these allocations fail).  This can greatly reduce the
> number of rarely tested error branches (e.g., alloc_disk failure in
> st.c does not free buffer) because of the number of drivers that can
> be cleaned up a bit by it: hundreds of drivers, thousands of lines of
> code.
> 
> 	Without ->match(), these changes would result in some unnecessary
> memory allocations and frees, and a lot of unncessary memory traffic as
> I have the initial kmalloc zero filling the memory that it allocates.
> 

I am sorry I have not looked at your previous patch, but are you saying
you want to do allocations in the match routine. If so what is your bus
based cleanup entry point. The bus_type lacks a add_device /
remove_device call like device_class.


> >I believed the device_interface was the answer also. scsi generic is even mentioned \
> >in the documentation. In talking with Mochel on this issue he disagreed and said that \
> >this was not the interface to use. 
> 
> 	I'd be interested in knowing what reasons he gave.
> 

In the previous conversation I thought device_interface was the answer
for sg so I went over to talk with Mochel. If I remember correctly the
current implementation of interfaces being tied to device_classes was
not right for sg. The sg driver can only be bound to one class that
would lead to all scsi devices ending up in one class and not a more
specific class (i.e. disks, tapes, etc.).

I could have the reason wrong, but the answer was this was not the api
to use.

I will ask the question again in a new thread and I know Mochel can
give a better answer.

> >The idea of a field in struct scsi_device seems like the right approach.
> >It is just that the device model needs a better bus symmetric interface to
> >handle cleanup and handling sg insmod / rmmod post device probing.
> 
> 	Hmm, I'm not sure if this is what you're referring to, but it
> looks like in 2.5.46, register_interface never walks the list of
> devices that were already registered, and unregister_interface never
> walks that list either (that is, intf->add_device is only called when
> a new device is registered after the interface is loaded).  I wonder
> if that difference in comparison to device_driver is intentional.

This is a good point, but in my previous statment I was talking about
how the bus does not have the add / remove callbacks like the
device_class.

I might not be intentional as some issues we ran into on cleanup where
just bugs / not implemented yet.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] get rid of ->detect for upper layer drivers

[Christoph Hellwig]

On Thu, Nov 07, 2002 at 07:32:08AM -0800, Adam J. Richter wrote:
> Michael Anderson wrote:
> >The removal of detect should eventually result in other reductions /
> >cleaner code (If the sg binding issue can be resolved cleanly).
> 
> 	What makes you think that?  You need a way to ask "is this
> driver interested in this device" and that's all that the code
> remaining in scsi_device_template.detect() does after you remove the
> side effects (i.e., the counter incrementing), and it does so about as
> simply as possible.

"Is this driver interested" is a fundamentally racy concept.  We want to
only support "attach if you're interested".  That's like the other
driver infrastructure works, too.

> 	I haven't looked at it closely, but there seems to be
> workaround in the generic device code for its inability to bind more
> than one driver to a device: struct device_interface, which seems to
> duplicate much of the generic driver interface but lacks
> device->driver_data, which we could compensate for by adding
> a field in struct scsi_device:
> 
> struct scsi_device {
>        ...
> 	struct sg_device *sg_dev;	/* For use by SCSI generic only. */
> };

Yupp, that's the way I'd prefer.  In addition get rid of the whole
driver template for sg.

Re: [PATCH] get rid of ->detect for upper layer drivers

[Mike Anderson]

Adam J. Richter [adam@freya.yggdrasil.com] wrote:
> Michael Anderson wrote:
> >The removal of detect should eventually result in other reductions /
> >cleaner code (If the sg binding issue can be resolved cleanly).
> 
> 	What makes you think that?  You need a way to ask "is this
> driver interested in this device" and that's all that the code
> remaining in scsi_device_template.detect() does after you remove the
> side effects (i.e., the counter incrementing), and it does so about as
> simply as possible.

If we support the device probe and remove interfaces then as a device is
registered with the scsi bus each drivers probe routine will be called
and if it wants the device it returns 0. 

If a driver is registered with the scsi bus post device probing then the
driver_attach routine will call bus_match and driver probe for all
devices on the bus not claimed by a driver.

> 
> 	I haven't looked at it closely, but there seems to be
> workaround in the generic device code for its inability to bind more
> than one driver to a device: struct device_interface, which seems to
> duplicate much of the generic driver interface but lacks
> device->driver_data, which we could compensate for by adding
> a field in struct scsi_device:
> 

I believed the device_interface was the answer also. scsi generic is even mentioned in the documentation. In talking with Mochel on this issue he disagreed and said that this was not the interface to use. 

The idea of a field in struct scsi_device seems like the right approach.
It is just that the device model needs a better bus symmetric interface to
handle cleanup and handling sg insmod / rmmod post device probing.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] get rid of ->detect for upper layer drivers

[Adam J. Richter]

Christoph Hellwig wrote:
>I'd prefer to get rid of scsi_bus_match entirely.  But if we have to
>keep it having a side-effect free method ito the drivers is probably
>the right thing, yes.  I'd rather call it "math", though to a) show
>what it's used for and b) not fool people porting drivers from 2.4 to
>2.5.

I agree that renaming ->detect to ->match would be an improvement.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: [PATCH] get rid of ->detect for upper layer drivers

[Adam J. Richter]

[Resending due to an email problem on my end.  Sorry if this is a
duplicate. -Adam]

Michael Anderson wrote:
>The removal of detect should eventually result in other reductions /
>cleaner code (If the sg binding issue can be resolved cleanly).

        What makes you think that?  You need a way to ask "is this
driver interested in this device" and that's all that the code
remaining in scsi_device_template.detect() does after you remove the
side effects (i.e., the counter incrementing), and it does so about as
simply as possible.

        I haven't looked at it closely, but there seems to be
workaround in the generic device code for its inability to bind more
than one driver to a device: struct device_interface, which seems to
duplicate much of the generic driver interface but lacks
device->driver_data, which we could compensate for by adding
a field in struct scsi_device:

struct scsi_device {
       ...
        struct sg_device *sg_dev;       /* For use by SCSI generic only. */
};

        Since device_interface API closely resembles that of
device_driver, it would probably be easy to port sg to device_driver
that API is every changed to support multiple drivers per device.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: [PATCH] get rid of ->detect for upper layer drivers

[Adam J. Richter]

>Okay, we're finally at the point where ->detect has become entirely
>superflous.  In sd, sr and st it does nothing but return different
>values that are ignored anyway, in sg it increments a variable
>that is never read (and thus removed in this patch aswell), only
>in osst it increments a variable that is actually checked for beeing
>non-zero.  But if it was zero we'd never reached that place anyway
>(same check in the beginning of osst_detect and osst_attach).

>This make the upper layer interface a lot nicer and avoids the
>race condition where a device is remove between detect and attach
>(which currently wouldn't matter anyway as we aren't doing anything
>in detect anymore)

	I think we can get cleaner code by *not* doing that.

	If we keep Scsi_Device_Template.detect and just change it so
that it has no side effects like incrementing a counter, then we can
shrink scsi_bus_match, previously a list of special cases, like so
(just typed this code in, haven't tried to compile it):


static int scsi_bus_match(struct device *dev, struct device_driver *drv)
{
	struct Scsi_Device_Template *scsi_dev_tmpl =
		container_of(drv, struct Scsi_Device_Template,
			     scsi_driverfs_driver);
	struct Scsi_Device_Template *scsi_dev =
		container_of(dev, struct Scsi_Device, sdev_driverfs_dev);

	return (*scsi_dev_templ->detect)(scsi_dev);
}

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: [PATCH] get rid of ->detect for upper layer drivers

[Mike Anderson]

Adam J. Richter [adam@yggdrasil.com] wrote:
> 	I think we can get cleaner code by *not* doing that.
> 
> 	If we keep Scsi_Device_Template.detect and just change it so
> that it has no side effects like incrementing a counter, then we can
> shrink scsi_bus_match, previously a list of special cases, like so
> (just typed this code in, haven't tried to compile it):
> 
> 
> static int scsi_bus_match(struct device *dev, struct device_driver *drv)
> {
> 	struct Scsi_Device_Template *scsi_dev_tmpl =
> 		container_of(drv, struct Scsi_Device_Template,
> 			     scsi_driverfs_driver);
> 	struct Scsi_Device_Template *scsi_dev =
> 		container_of(dev, struct Scsi_Device, sdev_driverfs_dev);
> 
> 	return (*scsi_dev_templ->detect)(scsi_dev);
> }

I believe the second container_of should be assigned to a struct scsi_device*.

If you got it to compile it should result in some unexpected behavior.

The reason scsi_bus_match is not clean is that scsi currently does not
follow the device model correctly. We currently allow multiple object
types on the scsi bus device list. This means your second container_of
may not be operating on a struct scsi_device object.

The multiple objects in the scsi bus device list is the result of some
mis-placed nodes and trying to work around the one driver to one device
binding issue. In scsi if sg is loaded a scsi device can have up to 2
drivers (i.e. sg + sd, etc).

The removal of detect should eventually result in other reductions /
cleaner code (If the sg binding issue can be resolved cleanly). Scsi can
then move to device model probe and remove routines and the elimination
of the scsi_devicelist.

I have ran a patch on top of Christoph's previous set that renamed
sd_attach and sd_detect to sd_probe and sd_remove. The old sd_attach and
sd_detect need to just take a struct device *dev and do a
to_scsi_device. 

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] get rid of ->detect for upper layer drivers

[Christoph Hellwig]

On Thu, Nov 07, 2002 at 06:25:25AM -0800, Mike Anderson wrote:
> The reason scsi_bus_match is not clean is that scsi currently does not
> follow the device model correctly. We currently allow multiple object
> types on the scsi bus device list. This means your second container_of
> may not be operating on a struct scsi_device object.
> 
> The multiple objects in the scsi bus device list is the result of some
> mis-placed nodes and trying to work around the one driver to one device
> binding issue. In scsi if sg is loaded a scsi device can have up to 2
> drivers (i.e. sg + sd, etc).
> 
> The removal of detect should eventually result in other reductions /
> cleaner code (If the sg binding issue can be resolved cleanly). Scsi can
> then move to device model probe and remove routines and the elimination
> of the scsi_devicelist.

Yupp, IMHO we should move to a better interpretation of the driver
model.  But the question how do we solve the sg issue.  There are two
ways I could imagine:

(a)  we have primary and secondary upperlevel drivers, a primary one
     is registered with the driver model (and you can have only one
     per scsi device), a secondary is not.
(b)  sg becomes something different than a upperlevel driver

I think (b) is the way to go, we should just make the direct access
a property of the scsi device so that each device gets a sg node,
without any probing.

Re: [PATCH] get rid of ->detect for upper layer drivers

[J.E.J. Bottomley]

hch@lst.de said:
> Yupp, IMHO we should move to a better interpretation of the driver
> model.  But the question how do we solve the sg issue.  There are two
> ways I could imagine:

There are other nasties waiting in the pipeline.  The one that's been 
bothering me is the SCSI3 Controller Commands (for arrays and the like).  This 
basically adds array controller information to a standard direct-access 
device, but crucially, it has two different (and incompatible with 
direct-access) ways of addressing the array: physical-by real disk and 
logical-by the actual RAID-n unit.

Most of the time a SCC device is an sd device but with a translation in the 
addressing mode.  It makes sense that SCC would somehow filter the sd 
attachment and add a special scc device type just for the controlling node.

By the way, we already have this in the tree: the compaq cpqfc driver does on 
the fly translation for SCC devices into SD at the low level (that's for the 
RA4100 array).

James

Re: [PATCH] get rid of ->detect for upper layer drivers

[Christoph Hellwig]

On Thu, Nov 07, 2002 at 10:50:08AM -0500, J.E.J. Bottomley wrote:
> There are other nasties waiting in the pipeline.  The one that's been 
> bothering me is the SCSI3 Controller Commands (for arrays and the like).  This 
> basically adds array controller information to a standard direct-access 
> device, but crucially, it has two different (and incompatible with 
> direct-access) ways of addressing the array: physical-by real disk and 
> logical-by the actual RAID-n unit.
> 
> Most of the time a SCC device is an sd device but with a translation in the 
> addressing mode.  It makes sense that SCC would somehow filter the sd 
> attachment and add a special scc device type just for the controlling node.
> 
> By the way, we already have this in the tree: the compaq cpqfc driver does on 
> the fly translation for SCC devices into SD at the low level (that's for the 
> RA4100 array).

I think that's another reason why we want the one upper layer per
device model.  That way the SCC driver (it could be part of the sd
module if they share enough code or a different one) would register the
SCC device so sd can't grab them anymore.  This just requires that
module to be linked in earlier (another good reasons why the two templates
should be in the same module - this way we can guarantee that ordering)

Re: [PATCH] get rid of ->detect for upper layer drivers

[Patrick Mansfield]

On Thu, Nov 07, 2002 at 10:50:08AM -0500, J.E.J. Bottomley wrote:
> hch@lst.de said:

> There are other nasties waiting in the pipeline.  The one that's been 
> bothering me is the SCSI3 Controller Commands (for arrays and the like).  This 
> basically adds array controller information to a standard direct-access 
> device, but crucially, it has two different (and incompatible with 
> direct-access) ways of addressing the array: physical-by real disk and 
> logical-by the actual RAID-n unit.

The SCSI Enclosure Services also allows similiar behaviour (I haven't
looked at the latest SES specs).

It would be nice if these device added another LUN for the SCC or
SES controller access.

-- Patrick Mansfield

Re: [PATCH] get rid of ->detect for upper layer drivers

[Christoph Hellwig]

On Thu, Nov 07, 2002 at 04:25:24AM -0800, Adam J. Richter wrote:
> 	I think we can get cleaner code by *not* doing that.
> 
> 	If we keep Scsi_Device_Template.detect and just change it so
> that it has no side effects like incrementing a counter, then we can
> shrink scsi_bus_match, previously a list of special cases, like so
> (just typed this code in, haven't tried to compile it):

I'd prefer to get rid of scsi_bus_match entirely.  But if we have to
keep it having a side-effect free method ito the drivers is probably
the right thing, yes.  I'd rather call it "math", though to a) show
what it's used for and b) not fool people porting drivers from 2.4 to
2.5.

[PATCH] get rid of ->detect for upper layer drivers

[Christoph Hellwig]

Okay, we're finally at the point where ->detect has become entirely
superflous.  In sd, sr and st it does nothing but return different
values that are ignored anyway, in sg it increments a variable
that is never read (and thus removed in this patch aswell), only
in osst it increments a variable that is actually checked for beeing
non-zero.  But if it was zero we'd never reached that place anyway
(same check in the beginning of osst_detect and osst_attach).

This make the upper layer interface a lot nicer and avoids the
race condition where a device is remove between detect and attach
(which currently wouldn't matter anyway as we aren't doing anything
in detect anymore)



--- 1.33/drivers/scsi/hosts.h	Wed Nov  6 17:53:34 2002
+++ edited/drivers/scsi/hosts.h	Thu Nov  7 02:07:35 2002
@@ -554,7 +554,6 @@
     const char * tag;
     struct module * module;	  /* Used for loadable modules */
     unsigned char scsi_type;
-    int (*detect)(Scsi_Device *); /* Returns 1 if we can attach this device */
     int (*attach)(Scsi_Device *); /* Attach devices to arrays */
     void (*detach)(Scsi_Device *);
     int (*init_command)(Scsi_Cmnd *);     /* Used by new queueing code. 
===== drivers/scsi/osst.c 1.25 vs edited =====
--- 1.25/drivers/scsi/osst.c	Tue Nov  5 12:26:53 2002
+++ edited/drivers/scsi/osst.c	Thu Nov  7 02:07:19 2002
@@ -153,10 +153,8 @@
 
 static int osst_init(void);
 static int osst_attach(Scsi_Device *);
-static int osst_detect(Scsi_Device *);
 static void osst_detach(Scsi_Device *);
 
-static int osst_dev_noticed;
 static int osst_nr_dev;
 static int osst_dev_max;
 
@@ -166,7 +164,6 @@
        name:		"OnStream tape",
        tag:		"osst",
        scsi_type:	TYPE_TAPE,
-       detect:		osst_detect,
        attach:		osst_attach,
        detach:		osst_detach
 };
@@ -5564,15 +5561,6 @@
 	return 0;
 };
 
-static int osst_detect(Scsi_Device * SDp)
-{
-	if (SDp->type != TYPE_TAPE) return 0;
-	if ( ! osst_supports(SDp) ) return 0;
-	
-	osst_dev_noticed++;
-	return 1;
-}
-
 static int osst_registered = 0;
 
 /* Driver initialization (not __initfunc because may be called later) */
@@ -5580,9 +5568,6 @@
 {
 	int i;
 
-	if (osst_dev_noticed == 0)
-		return 0;
-
 	if (!osst_registered) {
 		if (register_chrdev(MAJOR_NR,"osst",&osst_fops)) {
 			printk(KERN_ERR "osst :W: Unable to get major %d for OnStream tapes\n",MAJOR_NR);
@@ -5653,7 +5638,6 @@
 		os_scsi_tapes[i] = NULL;
 		scsi_slave_detach(SDp);
 		osst_nr_dev--;
-		osst_dev_noticed--;
 		return;
 	}
   }
===== drivers/scsi/scsi.c 1.57 vs edited =====
--- 1.57/drivers/scsi/scsi.c	Wed Nov  6 17:53:34 2002
+++ edited/drivers/scsi/scsi.c	Thu Nov  7 02:10:15 2002
@@ -1958,17 +1958,6 @@
 }
 #endif
 
-void scsi_detect_device(struct scsi_device *sdev)
-{
-	struct Scsi_Device_Template *sdt;
-
-	down_read(&scsi_devicelist_mutex);
-	for (sdt = scsi_devicelist; sdt; sdt = sdt->next)
-		if (sdt->detect)
-			(*sdt->detect)(sdev);
-	up_read(&scsi_devicelist_mutex);
-}
-
 int scsi_attach_device(struct scsi_device *sdev)
 {
 	struct Scsi_Device_Template *sdt;
@@ -2089,22 +2078,6 @@
 
 	driver_register(&tpnt->scsi_driverfs_driver);
 
-	/*
-	 * First scan the devices that we know about, and see if we notice them.
-	 */
-
-	for (shpnt = scsi_host_get_next(NULL); shpnt;
-	     shpnt = scsi_host_get_next(shpnt)) {
-		for (SDpnt = shpnt->host_queue; SDpnt;
-		     SDpnt = SDpnt->next) {
-			if (tpnt->detect)
-				(*tpnt->detect) (SDpnt);
-		}
-	}
-
-	/*
-	 * Now actually connect the devices to the new driver.
-	 */
 	for (shpnt = scsi_host_get_next(NULL); shpnt;
 	     shpnt = scsi_host_get_next(shpnt)) {
 		for (SDpnt = shpnt->host_queue; SDpnt;
===== drivers/scsi/scsi.h 1.34 vs edited =====
--- 1.34/drivers/scsi/scsi.h	Tue Nov  5 12:26:24 2002
+++ edited/drivers/scsi/scsi.h	Thu Nov  7 02:08:24 2002
@@ -480,7 +480,6 @@
 			int timeout, int retries);
 extern int scsi_dev_init(void);
 extern int scsi_mlqueue_insert(struct scsi_cmnd *, int);
-extern void scsi_detect_device(struct scsi_device *);
 extern int scsi_attach_device(struct scsi_device *);
 extern void scsi_detach_device(struct scsi_device *);
 
===== drivers/scsi/scsi_scan.c 1.33 vs edited =====
--- 1.33/drivers/scsi/scsi_scan.c	Tue Nov  5 12:27:24 2002
+++ edited/drivers/scsi/scsi_scan.c	Thu Nov  7 02:08:41 2002
@@ -1478,8 +1478,6 @@
 	 * function */
 	sdev->max_device_blocked = SCSI_DEFAULT_DEVICE_BLOCKED;
 
-	scsi_detect_device(sdev);
-
 	if (sdevnew != NULL)
 		*sdevnew = sdev;
 
===== drivers/scsi/sd.c 1.89 vs edited =====
--- 1.89/drivers/scsi/sd.c	Tue Nov  5 12:29:12 2002
+++ edited/drivers/scsi/sd.c	Thu Nov  7 02:04:51 2002
@@ -94,7 +94,6 @@
 static void sd_rw_intr(struct scsi_cmnd * SCpnt);
 
 static int sd_attach(struct scsi_device *);
-static int sd_detect(struct scsi_device *);
 static void sd_detach(struct scsi_device *);
 static int sd_init_command(struct scsi_cmnd *);
 static int sd_synchronize_cache(struct scsi_disk *, int);
@@ -107,7 +106,6 @@
 	.name		= "disk",
 	.tag		= "sd",
 	.scsi_type	= TYPE_DISK,
-	.detect		= sd_detect,
 	.attach		= sd_attach,
 	.detach		= sd_detach,
 	.init_command	= sd_init_command,
@@ -1163,23 +1161,6 @@
 	scsi_release_request(SRpnt);
 
 	kfree(buffer);
-}
-
-/**
- *	sd_detect - called at the start of driver initialization, once 
- *	for each scsi device (not just disks) present.
- *
- *	Returns 0 if not interested in this scsi device (e.g. scanner);
- *	1 if this device is of interest (e.g. a disk).
- *
- *	Note: this function is invoked from the scsi mid-level.
- **/
-static int sd_detect(struct scsi_device * sdp)
-{
-	SCSI_LOG_HLQUEUE(3, printk("sd_detect: type=%d\n", sdp->type));
-	if (sdp->type != TYPE_DISK && sdp->type != TYPE_MOD)
-		return 0;
-	return 1;
 }
 
 /**
===== drivers/scsi/sg.c 1.38 vs edited =====
--- 1.38/drivers/scsi/sg.c	Tue Nov  5 12:30:41 2002
+++ edited/drivers/scsi/sg.c	Thu Nov  7 02:06:38 2002
@@ -111,7 +111,6 @@
 #define SG_DEV_ARR_LUMP 6	/* amount to over allocate sg_dev_arr by */
 
 static int sg_attach(Scsi_Device *);
-static int sg_detect(Scsi_Device *);
 static void sg_detach(Scsi_Device *);
 
 static Scsi_Request *dummy_cmdp;	/* only used for sizeof */
@@ -124,7 +123,6 @@
 	.name = "generic",
 	.tag = "sg",
 	.scsi_type = 0xff,
-	.detect = sg_detect,
 	.attach = sg_attach,
 	.detach = sg_detach
 };
@@ -233,7 +231,6 @@
 #endif
 
 static Sg_device **sg_dev_arr = NULL;
-static int sg_dev_noticed;
 static int sg_dev_max;
 static int sg_nr_dev;
 
@@ -1338,13 +1335,6 @@
 	.fasync = sg_fasync,
 };
 
-static int
-sg_detect(Scsi_Device * scsidp)
-{
-	sg_dev_noticed++;
-	return 1;
-}
-
 #ifndef MODULE
 static int __init
 sg_def_reserved_size_setup(char *str)
@@ -1563,7 +1553,6 @@
 		}
 		scsi_slave_detach(scsidp);
 		sg_nr_dev--;
-		sg_dev_noticed--;	/* from <dan@lectra.fr> */
 		break;
 	}
 	write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
===== drivers/scsi/sr.c 1.64 vs edited =====
--- 1.64/drivers/scsi/sr.c	Tue Nov  5 12:37:40 2002
+++ edited/drivers/scsi/sr.c	Thu Nov  7 02:04:59 2002
@@ -66,7 +66,6 @@
 	 CDC_CD_R|CDC_CD_RW|CDC_DVD|CDC_DVD_R|CDC_GENERIC_PACKET)
 
 static int sr_attach(struct scsi_device *);
-static int sr_detect(struct scsi_device *);
 static void sr_detach(struct scsi_device *);
 static int sr_init_command(struct scsi_cmnd *);
 
@@ -75,7 +74,6 @@
 	.name		= "cdrom",
 	.tag		= "sr",
 	.scsi_type	= TYPE_ROM,
-	.detect		= sr_detect,
 	.attach		= sr_attach,
 	.detach		= sr_detach,
 	.init_command	= sr_init_command
@@ -487,14 +485,6 @@
 		get_sectorsize(cd);
 
 	return 0;
-}
-
-static int sr_detect(struct scsi_device * SDp)
-{
-
-	if (SDp->type != TYPE_ROM && SDp->type != TYPE_WORM)
-		return 0;
-	return 1;
 }
 
 static int sr_attach(struct scsi_device *sdev)
===== drivers/scsi/st.c 1.40 vs edited =====
--- 1.40/drivers/scsi/st.c	Tue Nov  5 12:36:00 2002
+++ edited/drivers/scsi/st.c	Thu Nov  7 02:05:21 2002
@@ -170,7 +170,6 @@
 static int sgl_unmap_user_pages(struct scatterlist *, const unsigned int, int);
 
 static int st_attach(Scsi_Device *);
-static int st_detect(Scsi_Device *);
 static void st_detach(Scsi_Device *);
 
 static struct Scsi_Device_Template st_template = {
@@ -178,7 +177,6 @@
 	.name =		"tape", 
 	.tag =		"st", 
 	.scsi_type =	TYPE_TAPE,
-	.detect =	st_detect, 
 	.attach =	st_attach, 
 	.detach =	st_detach
 };
@@ -3884,13 +3882,6 @@
 
 	return 0;
 };
-
-static int st_detect(Scsi_Device * SDp)
-{
-	if (SDp->type != TYPE_TAPE || st_incompatible(SDp))
-		return 0;
-	return 1;
-}
 
 static void st_detach(Scsi_Device * SDp)
 {