MIME-Version: 1.0
References: <20230307083559.2379758-1-korantwork@gmail.com> <20230308225718.GA1054189@bhelgaas>
In-Reply-To: <20230308225718.GA1054189@bhelgaas>
From:   Xinghui Li <korantwork@gmail.com>
Date:   Thu, 9 Mar 2023 20:31:55 +0800
Message-ID: <CAEm4hYXeNR5by3ZX9V1y=ebOFcQ+JD1AgHGmFiJ3UE3xBJvRCw@mail.gmail.com>
Subject: Re: [PATCH v2] PCI:vmd: add the module param to adjust msi mode
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     nirmal.patel@linux.intel.com, kbusch@kernel.org,
        jonathan.derrick@linux.dev, lpieralisi@kernel.org,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        Xinghui Li <korantli@tencent.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Bjorn Helgaas <helgaas@kernel.org> =E4=BA=8E2023=E5=B9=B43=E6=9C=889=E6=97=
=A5=E5=91=A8=E5=9B=9B 06:57=E5=86=99=E9=81=93=EF=BC=9A
>
> 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@tencent.com>
> > Reviewed-by: Nirmal Patel <nirmal.patel@linux.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 =3D "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?