Re: [PATCH v2] PCI:vmd: add the module param to adjust msi mode

[Patel, Nirmal]

On 3/9/2023 5:31 AM, Xinghui Li wrote:
> Bjorn Helgaas <helgaas@xxxxxxxxxx> 于2023年3月9日周四 06:57写道：
>> Please adjust the subject line to match previous history, e.g.,
>>
>>   PCI: vmd: Add ... MSI ...
>>
> OK. I will fix it. Sorry for ignoring the subject format.
>
>>> In the legacy, the vmd msi-mode can only be adjusted by configing
>>> vmd_ids table.This patch adds another way to adjust msi mode by
>>> adjusting module param, which allow users easier to adjust the vmd
>>> according to the I/O scenario without rebuilding driver.There are two
>>> params could be recognized: on, off. The default param is "NULL",
>>> the goal is not to affect the existing settings of the device.
>> Please add a space after the period that ends each sentence.
>> Capitalize "MSI" to match usage in spec.
>>
> Sorry for the format issue. I neglected them. My bad~
>
>>> Signed-off-by: Xinghui Li <korantli@xxxxxxxxxxx>
>>> Reviewed-by: Nirmal Patel <nirmal.patel@xxxxxxxxxxxxxxx>
>> I didn't see a response from Nirmal on the mailing list with the
>> Reviewed-by.  I think it's better if Nirmal responds to the patch
>> directly on the mailing list with the Reviewed-by, and whoever applies
>> the patch can incorporate it.  Otherwise we have no visibility into
>> any interaction between you and Nirmal.
>>
> I pinged Nirmal to reply to this patch, It seems he forgot to cc the
> mail list in the previous version's discussion.
>
>>> +/*
>>> + * The VMD msi_remap module parameter provides the alternative way
>>> + * to adjust msi mode when loading vmd.ko other than vmd_ids table.
>>> + * There are two params could be recognized:
>>> + *
>>> + * 1-off
>>> + * 2-on
>> It looks like your code matches either "on" or "off", not "1" or "2".
>>
> I will change the comment. It does mislead the reader. I mean the No.1
> param is "on" and the No.2 param is "off"
>
>>> + * The default param is "NULL", the goal is not to affect the existing
>>> + * settings of the device.
>>> + */
>>> +char *msi_remap = "NULL";
>> Looks like this should be static?  And using "NULL" (as opposed to
>> something like the empty string "") suggests some intrinsic meaning of
>> "NULL", but I think there is no intrinsic meaning and the only point
>> is that "NULL" doesn't match either "on" or "off".
>>
> The "static" one is better, I will add it.
> Initial parameters with "NULL" just aim to mismatch "on" or "off". Do
> you prefer to init it without the default string?

Please address Bjorn's comments. Thank you.

Reviewed-by: Nirmal Patel <nirmal.patel@xxxxxxxxxxxxxxx>