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

[Grant Grundler]

On Sun, Nov 15, 2009 at 10:19 PM, Vivek Mahajan
<vivek.mahajan at freescale.com> wrote:
> The following patch adds MSI support. Some platforms
> may have broken MSI, so those are defaulted to use
> legacy PCI interrupts.
>
> Signed-off-by: Vivek Mahajan <vivek.mahajan at freescale.com>
> ---
>  drivers/ata/sata_sil24.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index e6946fc..1370df6 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -417,6 +417,10 @@ static struct ata_port_operations sil24_ops = {
>  #endif
>  };
>
> +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.

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.

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

I'm not able to test this since the chipset I have sata_sil24 devices
plugged into don't support MSI (older AMD/Nvidia chipset). :(


> +
>  /*
>  * Use bits 30-31 of port_flags to encode available port numbers.
>  * Current maxium is 4.
> @@ -1340,6 +1344,11 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
>        sil24_init_controller(host);
>
> +       if (sata_sil24_msi && !!pci_msi_enable(pdev)) {
> +               dev_printk(KERN_INFO, &pdev->dev, "Using MSI\n");
> +               pci_intx(pdev, 0);

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.

cheers,
grant

> +       }
> +
>        pci_set_master(pdev);
>        return ata_host_activate(host, pdev->irq, sil24_interrupt, IRQF_SHARED,
>                                 &sil24_sht);
> --
> 1.5.6.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>