Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2015-05-05 at 10:25 -0400, Alan Stern wrote:
> On Mon, 4 May 2015, James Bottomley wrote:
> 
> > On Mon, 2015-05-04 at 16:09 -0400, Alan Stern wrote:
> > > On Mon, 4 May 2015, James Bottomley wrote:
> > > 
> > > > However, it does also strike me that these three drivers have problems
> > > > because they're using the wrong initialisation pattern: the template is
> > > > supposed to be in the bus connector for compound drivers not in the
> > > > core.
> > > 
> > > Why is it supposed to be done that way?  Isn't that less efficient?  It 
> > > means you have to have a separate copy of the template for each bus 
> > > connector driver, instead of letting them all share a common template 
> > > in the core.
> > 
> > Well, no it doesn't.  The way 53c700 implements it is that there is a
> > common template in the core.  The drivers just initialise their variant
> > fields (for 53c700 it's name, proc_name and this_id) and the core fills
> > in the rest.  Admittedly wd33c93 doesn't quite get this right, that's
> > why I cited 53c700.
> 
> You seem to be agreeing with me, even though you think you are 
> disagreeing.
> 
> 	"... there is a common template in the core." -- that's one
> 	scsi_host_template structure.
> 
> 	"The drivers just initialize their variant fields ... and the
> 	core fills in the rest." -- that's an additional
> 	scsi_host_template structure for each driver.
> 
> The total comes to N+1 scsi_host_templates, where N is the number of 
> drivers.
> 
> Now consider the way usb-storage does things.
> 
> 	There is a common template in the core.  That's one
> 	scsi_host_template structure.
> 
> 	The drivers use the core's template.  They have _no_ variant
> 	fields other than .owner.  That's why I thought Akinobu's idea
> 	of putting the owner field in the Scsi_Host structure was a
> 	good idea.
> 
> The total comes to 1 scsi_host_template.  Is that not better than N+1?
> 
> Consider the patch Akinobu just posted.  In addition to a whole bunch 
> of new code, it adds this:
> 
> > --- a/drivers/usb/storage/alauda.c
> > +++ b/drivers/usb/storage/alauda.c
> ...
> > @@ -1232,6 +1233,8 @@ static int alauda_transport(struct scsi_cmnd *srb, struct us_data *us)
> >  	return USB_STOR_TRANSPORT_FAILED;
> >  }
> >  
> > +static struct scsi_host_template alauda_host_template;
> 
> alauda.c was the only driver modified in that patch, and it gained an 
> new scsi_host_template.
> 
> For the case where the variants are identical in all respects other 
> than .owner, doesn't it make sense to allow them to share a single 
> scsi_host_template?
> 
> The original design of the SCSI stack envisioned multiple drivers, each 
> in control of multiple SCSI hosts.  The idea was that 
> scsi_host_template would be associated with the driver and Scsi_Host 
> with the individual host.
> 
> Now the kernel has evolved, and we have multiple drivers, some of which
> contain multiple subdrivers, each in control of multiple SCSI hosts.  
> In this situation we should be flexible enough to allow the
> scsi_host_template to be associated with either the driver or the
> subdriver (decision to be made by the driver).  When the only variant
> field is .owner, it makes sense to associate the scsi_host_template
> with the driver, not force it to be associated with the subdriver.
> 
> The cost is duplication of the .owner field in every Scsi_Host.  The 
> savings is a reduction in the number of scsi_host_templates.

So your essential objection is the host template duplication?  I know
it's a couple of hundred bytes, but surely its dwarfed by all the other
stuff you have to duplicate ... the module size of each of these is
around 0.25MB, so a couple of hundred bytes would seem a bit
insignificant.

James