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;
}
