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

[suijingfeng]

Hi,


On 2023/8/9 21:52, Ilpo Järvinen wrote:
> On Wed, 9 Aug 2023, Sui Jingfeng wrote:
>
> > From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> > ---
> >   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;
}