Message-ID: <2325a961-02a6-ac50-624d-0a3c97e89c7d@linux.intel.com>
Date:   Wed, 15 Mar 2023 14:49:04 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v2] PCI:vmd: add the module param to adjust msi mode
Content-Language: en-US
To:     Xinghui Li <korantwork@gmail.com>,
        Bjorn Helgaas <helgaas@kernel.org>
Cc:     kbusch@kernel.org, lpieralisi@kernel.org,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        Xinghui Li <korantli@tencent.com>
References: <20230307083559.2379758-1-korantwork@gmail.com>
 <20230308225718.GA1054189@bhelgaas>
 <CAEm4hYXeNR5by3ZX9V1y=ebOFcQ+JD1AgHGmFiJ3UE3xBJvRCw@mail.gmail.com>
From:   "Patel, Nirmal" <nirmal.patel@linux.intel.com>
In-Reply-To: <CAEm4hYXeNR5by3ZX9V1y=ebOFcQ+JD1AgHGmFiJ3UE3xBJvRCw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/9/2023 5:31 AM, Xinghui Li wrote:
> Bjorn Helgaas <helgaas@kernel.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@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 = "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@linux.intel.com>