Re: [PATCH v3] Add pcie_hp=nomsi to disable MSI/MSI-X for pciehp driver

[MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com>]

On Thu, 19 Jan 2012 18:39:22 +0900,
Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote:
>
>> --- linux-3.2.orig/drivers/pci/hotplug/pciehp_core.c
>> +++ linux-3.2/drivers/pci/hotplug/pciehp_core.c
>> @@ -42,6 +42,7 @@ int pciehp_debug;
>>    int pciehp_poll_mode;
>>    int pciehp_poll_time;
>>    int pciehp_force;
>> +int pciehp_msi_disabled;
>
> I think pciehp_msi_disabled needs to be defined in port driver
> side because pciehp can be built as a module.

Agree.

>>    struct workqueue_struct *pciehp_wq;
>>    struct workqueue_struct *pciehp_ordered_wq;
>>
>> @@ -57,10 +58,12 @@ module_param(pciehp_debug, bool, 0644);
>>    module_param(pciehp_poll_mode, bool, 0644);
>>    module_param(pciehp_poll_time, int, 0644);
>>    module_param(pciehp_force, bool, 0644);
>> +module_param(pciehp_msi_disabled, bool, 0644);
>
> When pciehp is build as a module, the pciehp driver is loaded
> after the MSI(-X) of the port is initialized. So I don't think
> this module parameter works. When pciehp is build as built-in,
> pciehp.pciehp_msi_disabled would work, but I think it is
> meaningless because you also provide "pcie_hp=" boot parameter
> (and I don't know what will happen if user specify different
> values for pciehp.pciehp_msi_disabled and pcie_hp). As a result,
> I think this module parameter should be removed.

That's make sense.  Will remove this.

>>    MODULE_PARM_DESC(pciehp_debug, "Debugging mode enabled or not");
>>    MODULE_PARM_DESC(pciehp_poll_mode, "Using polling mechanism for hot-plug events or not");
>>    MODULE_PARM_DESC(pciehp_poll_time, "Polling mechanism frequency, in seconds");
>>    MODULE_PARM_DESC(pciehp_force, "Force pciehp, even if OSHP is missing");
>> +MODULE_PARM_DESC(pciehp_msi, "MSI/MSI-X enabled or not");
>
> Maybe it should be "pciehp_msi_disabled" instead of "pciehp_msi"?
> Anyway, I think we should remove this for the same reason above.

Ditto.

>>    #define PCIE_MODULE_NAME "pciehp"
>>
>> @@ -72,6 +75,15 @@ static int get_attention_status	(struct
>>    static int get_latch_status	(struct hotplug_slot *slot, u8 *value);
>>    static int get_adapter_status	(struct hotplug_slot *slot, u8 *value);
>>
>> +static int __init pciehp_setup(char *str)
>> +{
>> +	if (!strncmp(str, "nomsi", 5))
>> +		pciehp_msi_disabled = true;
>> +
>> +	return 1;
>> +}
>> +__setup("pcie_hp=", pciehp_setup);
>> +
>
> I think this needs to be in port driver side, otherwise pcie_hp=
> boot parameter doesn't work when building pciehp as a module.
> Additionally, I got the the following warning message when building
> pciehp as a module.
>
> drivers/pci/hotplug/pciehp_core.c:77: warning: 'pciehp_setup' defined but not used

You are corrent.  I forgot to confirm this.  Thank you for letting me
know.

>> -	/* We have to use INTx if MSI cannot be used for PCIe PME. */
>> -	if ((mask&   PCIE_PORT_SERVICE_PME)&&   pcie_pme_no_msi()) {
>> +	/* We have to use INTx if MSI cannot be used for PCIe PME or pciehp. */
>> +	if (((mask&   PCIE_PORT_SERVICE_PME)&&   pcie_pme_no_msi()) ||
>> +	    ((mask&   PCIE_PORT_SERVICE_HP)&&   pciehp_no_msi()) ||
>> +	    ((mask&   PCIE_PORT_SERVICE_HP)&&
>> +	     (dev->vendor == PCI_VENDOR_ID_IDT&&   dev->device == 0x807f))) {
>
> How about implementing vendor/device check as a quirk?

Oh, it looks good.
Will post a updated patch soon.

Thanks,
Takahiro