| Message ID | 20230808223412.1743176-8-sui.jingfeng@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Fix typos, comments and copyright | expand |
On Wed, 9 Aug 2023, Sui Jingfeng wrote: > From: Sui Jingfeng <suijingfeng@loongson.cn> > Changelog body is missing. > 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?? 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. > */ > 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. I'd prefer to initialize ret = 0 instead: int ret = 0; ... if (!vgadev) { err = -ENODEV; goto unlock; } ... unlock: ...
Hi, 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. >> */ >> 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; }
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).
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.
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 */ 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; } EXPORT_SYMBOL(vga_client_register);