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

[Xinghui Li]

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?