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

[Sui Jingfeng <sui.jingfeng@linux.dev>]

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.