[PATCH] libsas: move ATA bits into a separate module

[PATCH] libsas: move ATA bits into a separate module

[Darrick J. Wong]

Hi all,

Per James Bottomley's request, I've moved all the libsas SATA support
code into a separate module, named sas_ata.  To satisfy his further
requirement that libsas not require libata (and vice versa), ata_sas
maintains fixed function pointer tables to various required functions
within libsas and libata.  Unfortunately, this means that libsas and
libata both require sas_ata, but sas_ata is smaller than libata.
Unloads of libata/libsas at inopportune moments are prevented by
increasing the refcounts on both modules whenever libsas detects a SATA
device (and decreasing it when the device goes away, of course).  If the
module is removed from the .config, then all of hooks into libsas/libata
should go away.

This is a rough-cut at separating out the ATA code; please let me know
what I can improve.  At the moment, I can load and talk to SATA disks
with the module enabled, as well as watch nothing happen if the module
is not config'd in.

The patch is a bit large, so here's where it lives:
http://sweaglesw.net/~djwong/docs/sas-ata_2.patch

Thanks for any feedback that you can provide!

--D

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

Re: [PATCH] libsas: move ATA bits into a separate module

[Jeff Garzik]

Darrick J. Wong wrote:
> Hi all,
> 
> Per James Bottomley's request, I've moved all the libsas SATA support
> code into a separate module, named sas_ata.  To satisfy his further
> requirement that libsas not require libata (and vice versa), ata_sas
> maintains fixed function pointer tables to various required functions
> within libsas and libata.  Unfortunately, this means that libsas and
> libata both require sas_ata, but sas_ata is smaller than libata.
> Unloads of libata/libsas at inopportune moments are prevented by
> increasing the refcounts on both modules whenever libsas detects a SATA
> device (and decreasing it when the device goes away, of course).  If the
> module is removed from the .config, then all of hooks into libsas/libata
> should go away.
> 
> This is a rough-cut at separating out the ATA code; please let me know
> what I can improve.  At the moment, I can load and talk to SATA disks
> with the module enabled, as well as watch nothing happen if the module
> is not config'd in.
> 
> The patch is a bit large, so here's where it lives:
> http://sweaglesw.net/~djwong/docs/sas-ata_2.patch

I disagree completely with this approach.

You don't need a table of hooks for the case where libata is disabled in 
.config.  Thus, it's only useful for the case where libsas is loaded as 
a module, but libata is not.

And the cost of having libata loaded via the normal symbol resolution / 
module load mechanisms is low, so adding a table of hooks completely 
wrapping libata functions is just silly.

The libsas code should directly call libata functions.  If ATA support 
in libsas is disabled in .config, then those functions will never be 
called, thus never loaded the libata module.

	Jeff

Re: [PATCH v3] libsas: move ATA bits into a separate module

[Darrick J. Wong]

Jeff Garzik wrote:

> I disagree completely with this approach.
> 
> You don't need a table of hooks for the case where libata is disabled in
> .config.  Thus, it's only useful for the case where libsas is loaded as
> a module, but libata is not.

Indeed, I misunderstood what James Bottomley wanted, so I reworked the
patch.  It has the same functionality as before, but this module uses
the module loader/symbol resolver for all the functions in libata, and
allows libsas to (optionally) call into sas_ata with weak references by
pushing a table of the necessary function pointers into libsas at
sas_ata load time.  Thus, libsas doesn't need to load libata/sas_ata
unless it actually finds a SATA device.

> The libsas code should directly call libata functions.  If ATA support
> in libsas is disabled in .config, then those functions will never be
> called, thus never loaded the libata module.

I certainly can (and now do) call libata functions from sas_ata.
However, there are a few cases where libsas needs to call libata (ex.
sas_ioctl); for those cases, I think the function pointers are still
necessary because I don't want to make libsas require libata.  In any
case, if ATA support is disabled in .config, sata_is_dev always returns
0, so the dead-code eliminator should zap that out of libsas entirely.

As usual, thank you for any feedback that you have.

Link to version 3:
http://sweaglesw.net/~djwong/docs/sas-ata_3.patch

--D

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

Re: [PATCH v3] libsas: move ATA bits into a separate module

[Jeff Garzik]

Darrick J. Wong wrote:
> Jeff Garzik wrote:
> 
>> I disagree completely with this approach.
>>
>> You don't need a table of hooks for the case where libata is disabled in
>> .config.  Thus, it's only useful for the case where libsas is loaded as
>> a module, but libata is not.
> 
> Indeed, I misunderstood what James Bottomley wanted, so I reworked the
> patch.  It has the same functionality as before, but this module uses
> the module loader/symbol resolver for all the functions in libata, and
> allows libsas to (optionally) call into sas_ata with weak references by
> pushing a table of the necessary function pointers into libsas at
> sas_ata load time.  Thus, libsas doesn't need to load libata/sas_ata
> unless it actually finds a SATA device.
> 
>> The libsas code should directly call libata functions.  If ATA support
>> in libsas is disabled in .config, then those functions will never be
>> called, thus never loaded the libata module.
> 
> I certainly can (and now do) call libata functions from sas_ata.
> However, there are a few cases where libsas needs to call libata (ex.
> sas_ioctl); for those cases, I think the function pointers are still
> necessary because I don't want to make libsas require libata.  In any
> case, if ATA support is disabled in .config, sata_is_dev always returns
> 0, so the dead-code eliminator should zap that out of libsas entirely.

Looks MUCH better to me, and eliminates my objection to the 
libata-related hooks.

There remains the issue that I poke James about on IRC, namely that 
there is no need to emulate the SATA phy registers.  libata permits a 
driver high level access to the ATA engine without needing SATA SCRs. 
Witness all the PATA drivers, which obviously do not have SCRs at all.

	Jeff

Re: [PATCH v3] libsas: move ATA bits into a separate module

[Christoph Hellwig]

On Thu, Sep 14, 2006 at 03:40:55PM -0700, Darrick J. Wong wrote:
> Jeff Garzik wrote:
> 
> > I disagree completely with this approach.
> > 
> > You don't need a table of hooks for the case where libata is disabled in
> > .config.  Thus, it's only useful for the case where libsas is loaded as
> > a module, but libata is not.
> 
> Indeed, I misunderstood what James Bottomley wanted, so I reworked the
> patch.  It has the same functionality as before, but this module uses
> the module loader/symbol resolver for all the functions in libata, and
> allows libsas to (optionally) call into sas_ata with weak references by
> pushing a table of the necessary function pointers into libsas at
> sas_ata load time.  Thus, libsas doesn't need to load libata/sas_ata
> unless it actually finds a SATA device.

NACK again.  Week references are bad.  Please change it back to normal
hard references so that it works like everything else in the kernel.

Re: [PATCH v3] libsas: move ATA bits into a separate module

[Jeff Garzik]

Christoph Hellwig wrote:
> On Thu, Sep 14, 2006 at 03:40:55PM -0700, Darrick J. Wong wrote:
>> Jeff Garzik wrote:
>>
>>> I disagree completely with this approach.
>>>
>>> You don't need a table of hooks for the case where libata is disabled in
>>> .config.  Thus, it's only useful for the case where libsas is loaded as
>>> a module, but libata is not.
>> Indeed, I misunderstood what James Bottomley wanted, so I reworked the
>> patch.  It has the same functionality as before, but this module uses
>> the module loader/symbol resolver for all the functions in libata, and
>> allows libsas to (optionally) call into sas_ata with weak references by
>> pushing a table of the necessary function pointers into libsas at
>> sas_ata load time.  Thus, libsas doesn't need to load libata/sas_ata
>> unless it actually finds a SATA device.
> 
> NACK again.  Week references are bad.  Please change it back to normal
> hard references so that it works like everything else in the kernel.

I strongly agree.

The kernel code will bloat, and performance will suffer, if we did weak 
refs and jump tables everywhere.

I just don't see the overhead of loading libata, and not using it, as a 
huge penalty, when looking at the alternatives.

Consider the common use cases:  (a) normal distro usage, often servers 
where libata loading will be common anyway due to SATA presence on 
motherboard, and (b) embedded use, where ATA support can be .config'd 
out at compile time.

Thus, the use cases where end users really will care about libata being 
loaded, but not used, are slim to none.

	Jeff

Re: [PATCH] libsas: move ATA bits into a separate module

[Christoph Hellwig]

On Wed, Sep 13, 2006 at 05:21:54PM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> Per James Bottomley's request, I've moved all the libsas SATA support
> code into a separate module, named sas_ata.  To satisfy his further
> requirement that libsas not require libata (and vice versa), ata_sas
> maintains fixed function pointer tables to various required functions
> within libsas and libata.  Unfortunately, this means that libsas and
> libata both require sas_ata, but sas_ata is smaller than libata.
> Unloads of libata/libsas at inopportune moments are prevented by
> increasing the refcounts on both modules whenever libsas detects a SATA
> device (and decreasing it when the device goes away, of course).  If the
> module is removed from the .config, then all of hooks into libsas/libata
> should go away.
> 
> This is a rough-cut at separating out the ATA code; please let me know
> what I can improve.  At the moment, I can load and talk to SATA disks
> with the module enabled, as well as watch nothing happen if the module
> is not config'd in.

I think this while idea is flawed.  Please separate out the ATA code
so it can be compiled out whelibata isn't built in, but it defintly
shouldn't be a separate module.  We're not doing any complexity like
that in any other subsystem (we used to do for AGP but got rid of it
again because it caused lots of problems).