[PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Mahajan Vivek-B08308]

> From: Grant Grundler [mailto:grundler at google.com] 
> Sent: Monday, November 16, 2009 11:08 PM
> > +static int sata_sil24_msi;    /* Disable MSI */ 
> > +module_param_named(msi, sata_sil24_msi, bool, S_IRUGO); 
> > +MODULE_PARM_DESC(msi, "Enable MSI (Default: false)");
> 
> Vivek,
> Do we even still need the parameter? I'm thinking either MSI 
> works with a chipset or it doesn't. The kernel has globals to 
> "know" which state is true.

Sometimes even in a platform, some PCIe endpoints do very 
well with MSI while others may have to resort to legacy ints. 
Should we let the endpoints make the final call.

> 
> If the parameter is needed, when this driver is compiled into 
> the kernel, how is "msi" parameter specified?
> I think the parameter needs to be documented and fit in with 
> other "msi" parameters.
> See "nomsi" in Documentation/kernel-parameters.txt.

In this case "msi" is supposed to be passed via insmod and 
not via kernel cmdline. If the driver is built-in the kernel, 
then force sata_sil24_msi = 1 in the driver to enable it.

> 
> If you want to keep this module parameter, can I suggest 
> calling the exported parameter "sata_sil24_msi"?
> 

Will do it in the subsequent patch.

> pci_intx() isn't documented in MSI-HOWTO.txt  - because it's 
> already called:
>     pci_msi_enable() -> pci_msi_enable_block() -> 
> msi_capability_init()
>        -> pci_intx_for_msi(dev, 0) -> pci_intx(pdev, 0);
> 
> (thanks to willy (Mathew Wilcox) for pointing me at
> msi_capability_init() - I overlooked it)
> 
> Please add "Reviewed-by: Grant Grundler 
> <grundler at google.com>" once the variable name is changed and 
> the pci_intx() call is removed.

Will take care in the upcoming patch

> 
> cheers,
> grant

Thanks,
Vivek