[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bfe424f2-6dad-c8c9-ec82-8eda70f23cdf@loongson.cn>
Date: Thu, 10 Aug 2023 19:56:26 +0800
From: suijingfeng <suijingfeng@...ngson.cn>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Sui Jingfeng <sui.jingfeng@...ux.dev>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
Dave Airlie <airlied@...hat.com>,
Daniel Vetter <daniel@...ll.ch>, linux-pci@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 07/11] PCI/VGA: vga_client_register() return -ENODEV on
failure, not -1
Hi,
On 2023/8/9 21:52, Ilpo Järvinen wrote:
> On Wed, 9 Aug 2023, Sui Jingfeng wrote:
>
>> From: Sui Jingfeng <suijingfeng@...ngson.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@...ngson.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.
>> */
>> int vga_client_register(struct pci_dev *pdev,
>> unsigned int (*set_decode)(struct pci_dev *pdev, bool decode))
>> @@ -975,16 +975,13 @@ int vga_client_register(struct pci_dev *pdev,
>>
>> spin_lock_irqsave(&vga_lock, flags);
>> vgadev = vgadev_find(pdev);
>> - if (!vgadev)
>> - goto bail;
>> -
>> - vgadev->set_decode = set_decode;
>> - ret = 0;
>> -
>> -bail:
>> + if (vgadev) {
>> + vgadev->set_decode = set_decode;
>> + ret = 0;
>> + }
>> spin_unlock_irqrestore(&vga_lock, flags);
>> - return ret;
>>
>> + return ret;
> No logic changes in this at all? I don't think it belongs to the same
> patch. I'm not sure if the new logic is improvement anyway.
Yes, the new logic is just improvement, no function change.
Strict speaking, you are right. One patch do one thing.
> 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;
}
Powered by blists - more mailing lists