Re: [PATCH v2 07/11] PCI/VGA: vga_client_register() return -ENODEV on failure, not -1

Sui Jingfeng
2023-08-10T12:18:45Z

Hi,

On 2023/8/10 20:13, Ilpo Järvinen wrote:
> On Thu, 10 Aug 2023, suijingfeng wrote:
>> On 2023/8/9 21:52, Ilpo Järvinen wrote:
>>> On Wed, 9 Aug 2023, Sui Jingfeng wrote:
>>>
>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>
>>> Changelog body is missing.
>>
>> I thought that probably the Fixes tag could be taken as the body of this
>> commit,
>> since there are no warnings when I check the whole series with checkpatch.pl.
>>
>>
>>>> Fixes: 934f992c763a ("drm/i915: Recognise non-VGA display devices")
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>> drivers/pci/vgaarb.c | 15 ++++++---------
>>>> 1 file changed, 6 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>>>> index 811510253553..a6b8c0def35d 100644
>>>> --- a/drivers/pci/vgaarb.c
>>>> +++ b/drivers/pci/vgaarb.c
>>>> @@ -964,7 +964,7 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
>>>> *
>>>> * To unregister just call vga_client_unregister().
>>>> *
>>>> - * Returns: 0 on success, -1 on failure
>>>> + * Returns: 0 on success, -ENODEV on failure
>>> So this is the true substance of this change??
>>>
>> Yes.
>>
>>
>>> It doesn't warrant Fixes tag which requires a real problem to fix. An
>>> incorrect comment is not enough.
>>>
>>> I think the shortlog is a bit misleading as is because it doesn't in any
>>> way indicate the problem is only in a comment.
>> But it's that commit(934f992c763a) alter the return value of
>> vga_client_register(),
>> which make the commit and code don't match anymore.
> This is useful information, no point in withholding it which forces
> others to figure it out by looking that commit up so put that detail into
> the changelog body.
>
>>> I'd prefer to
>>> initialize ret = 0 instead:
>>>
>>> int ret = 0;
>>> ...
>>> if (!vgadev) {
>>> err = -ENODEV;
>>> goto unlock;
>>> }
>>> ...
>>> unlock:
>>> ...
>>>
>> But this is same as the original coding style, no fundamental improve.
>> The key point is to make the wrapped code between the spin_lock_irqsave() and
>> spin_unlock_irqrestore() compact.
>> my patch remove the necessary 'goto' statement and the 'bail' label.
>> After apply my patch, the vga_client_register() function became as this:
>>
>> int vga_client_register(struct pci_dev *pdev,
>>         unsigned int (*set_decode)(struct pci_dev *pdev, bool decode))
>> {
>>     int ret = -ENODEV;
>>     struct vga_device *vgadev;
>>     unsigned long flags;
>>
>>     spin_lock_irqsave(&vga_lock, flags);
>>     vgadev = vgadev_find(pdev);
>>     if (vgadev) {
>>         vgadev->set_decode = set_decode;
>>         ret = 0;
>>     }
>>     spin_unlock_irqrestore(&vga_lock, flags);
>>
>>     return ret;
>> }
> I'm not too attached to either of the ways around since there's no
> correctness issues here. Feel free to ignore my alternative suggestion
> (make the separate patch out of it in anycase).

OK, will be done at the next version.