Re: [PATCH 2.6.20-rc3-mm1 2/2] libata: SiS 180 pata/sata support

Re: [PATCH 2.6.20-rc3-mm1 2/2] libata: SiS 180 pata/sata support

[Jeff Garzik]

Uwe Koziolek wrote:
> This patch includes the PATA support for SiS180 importing PATA functionality from pata_sis.c
> 
> Signed-off-by: Uwe Koziolek <uwe.koziolek@gmx.net>
> 
> --- a/drivers/ata/sata_sis.c	2007-01-07 20:54:38.000000000 +0100
> +++ b/drivers/ata/sata_sis.c	2007-01-07 23:42:29.000000000 +0100
> @@ -138,6 +138,10 @@
>  	.port_ops	= &sis_ops,
>  };
>  
> +#ifdef CONFIG_PATA_SIS
> +extern struct ata_port_info sis_info133;
> +#endif

This is a poor way to share code between modules.  It fails when 
pata_sis is a module, and it forces another driver to be loaded, simply 
then to be used as a code library.

If the shared code is tiny, just create a header file and #include it 
(sata_promise and sata_sx4 do this).  Otherwise, create a shared library 
module.

	Jeff

Re: [PATCH 2.6.20-rc3-mm1 2/2] libata: SiS 180 pata/sata support

[Alan]

> If the shared code is tiny, just create a header file and #include it 
> (sata_promise and sata_sx4 do this).  Otherwise, create a shared library 
> module.

pata_sis is both the SiS driver and the shared library. What needs fixing
IMHO is the Kconfig so that the user isn't aware of this. Then all the
CONFIG ifdef stuff goes away too.

Alan

Re: [PATCH 2.6.20-rc3-mm1 2/2] libata: SiS 180 pata/sata support

[Jeff Garzik]

Alan wrote:
>> If the shared code is tiny, just create a header file and #include it 
>> (sata_promise and sata_sx4 do this).  Otherwise, create a shared library 
>> module.
> 
> pata_sis is both the SiS driver and the shared library. What needs fixing

It is /very/ bad policy to force the user to load a driver that drives 
real hardware, just to be able to access a few functions.  Hardware and 
probe side effects are a very real possibility, and with this setup, the 
user has /less/ control than normal over things such as load order when 
pata_sis is loaded as a dependency.

If the duplicated code in question is small, just putting it in a header 
file a la drivers/ata/sata_promise.h is fine.  Otherwise, a library 
approach with no side effects should be chosen.

	Jeff

Re: [PATCH 2.6.20-rc3-mm1 2/2] libata: SiS 180 pata/sata support

[Alan]

> It is /very/ bad policy to force the user to load a driver that drives 
> real hardware, just to be able to access a few functions.  Hardware and 
> probe side effects are a very real possibility, and with this setup, the 

No they are not.

> user has /less/ control than normal over things such as load order when 
> pata_sis is loaded as a dependency.

Where did I put the 2x4

Right....

* Whack * ;)

The only probing the SiS driver does is for devices matching the SiS PCI
identifiers in question. Since they are intergrated bridges it's actually
physically impossible to obtain a box which contains both at once

* Whack * ;)

If it was added as a library then the non library driver would look
contain only a call to register the pci driver and a PCI table. The other
7.5K would be waste space. That also means there is no load order issue.

* Whack * ;)

Now if this was an ISA driver or poked around in ugly places your
argument would make sense. In this case I don't think your argument makes
sense, but if you want a pointless tiny pata_sis module to waste memory
on everyones boxes you can just ask and I'll send you a diff or two.

Alan

Re: [PATCH 2.6.20-rc3-mm1 2/2] libata: SiS 180 pata/sata support

[Jeff Garzik]

Alan wrote:
> If it was added as a library then the non library driver would look
> contain only a call to register the pci driver and a PCI table. The other
> 7.5K would be waste space. That also means there is no load order issue.

Ok there is no load order issue, but it's still an ugly solution.

Far better would be, then, to combine the drivers into a single ata_sis 
driver, it sounds like.

	Jeff

Re: [PATCH 2.6.20-rc3-mm1 2/2] libata: SiS 180 pata/sata support

[Alan]

> Far better would be, then, to combine the drivers into a single ata_sis 
> driver, it sounds like.

Why do pure PATA users need all the SATA crap loaded ? 

Alan