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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEm4hYXeNR5by3ZX9V1y=ebOFcQ+JD1AgHGmFiJ3UE3xBJvRCw@mail.gmail.com>
Date:   Thu, 9 Mar 2023 20:31:55 +0800
From:   Xinghui Li <korantwork@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     nirmal.patel@...ux.intel.com, kbusch@...nel.org,
        jonathan.derrick@...ux.dev, lpieralisi@...nel.org,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Xinghui Li <korantli@...cent.com>
Subject: Re: [PATCH v2] PCI:vmd: add the module param to adjust msi mode

Bjorn Helgaas <helgaas@...nel.org> 于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@...cent.com>
> > Reviewed-by: Nirmal Patel <nirmal.patel@...ux.intel.com>
>
> 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?