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

[Ilpo Järvinen]

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@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.

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).

-- 
 i.