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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: Grant Grundler [mailto:grundler@xxxxxxxxxx] 
> 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@xxxxxxxxxx>" once the variable name is changed and 
> the pci_intx() call is removed.

Will take care in the upcoming patch

> 
> cheers,
> grant

Thanks,
Vivek
 
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux