From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 10:48:04 +0200
A few update suggestions were taken into account from static source code analysis.
Markus Elfring (6): Use kmalloc_array() in mid_get_vbt_data_r10() Rename a jump label in mid_get_vbt_data_r10() Move a variable assignment in mid_get_vbt_data_r10() Fix indentation for a function call parameter in mid_get_vbt_data_r10() One error message less for a GCT revision mismatch in mid_get_vbt_data() Rename a jump label in mid_get_vbt_data()
drivers/gpu/drm/gma500/mid_bios.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 08:54:07 +0200
A multiplication for the size determination of a memory allocation indicated that an array data structure should be processed. Thus use the corresponding function "kmalloc_array".
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/gma500/mid_bios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index d75ecb3..a833568 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -235,7 +235,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr) if (read_vbt_r10(addr, &vbt)) return -1;
- gct = kmalloc(sizeof(*gct) * vbt.panel_count, GFP_KERNEL); + gct = kmalloc_array(vbt.panel_count, sizeof(*gct), GFP_KERNEL); if (!gct) return -1;
On Tue, 20 Sep 2016, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 08:54:07 +0200
A multiplication for the size determination of a memory allocation indicated that an array data structure should be processed. Thus use the corresponding function "kmalloc_array".
This issue was detected by using the Coccinelle software.
Did you test this running on the hardware?
BR, Jani.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
drivers/gpu/drm/gma500/mid_bios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index d75ecb3..a833568 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -235,7 +235,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr) if (read_vbt_r10(addr, &vbt)) return -1;
- gct = kmalloc(sizeof(*gct) * vbt.panel_count, GFP_KERNEL);
- gct = kmalloc_array(vbt.panel_count, sizeof(*gct), GFP_KERNEL); if (!gct) return -1;
A multiplication for the size determination of a memory allocation indicated that an array data structure should be processed. Thus use the corresponding function "kmalloc_array".
This issue was detected by using the Coccinelle software.
Did you test this running on the hardware?
No. - My "test computer" does not provide the corresponding hardware for the affected driver source files.
Regards, Markus
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 09:09:10 +0200
Adjust a jump label according to the current Linux coding style convention.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/gma500/mid_bios.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index a833568..cf4e605 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -242,7 +242,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr) gct_virtual = ioremap(addr + sizeof(vbt), sizeof(*gct) * vbt.panel_count); if (!gct_virtual) - goto out; + goto free_gct; memcpy_fromio(gct, gct_virtual, sizeof(*gct)); iounmap(gct_virtual);
@@ -270,7 +270,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr) dp_ti->vsync_pulse_width_lo = ti->vsync_pulse_width_lo;
ret = 0; -out: + free_gct: kfree(gct); return ret; }
On Tue, 20 Sep 2016, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 09:09:10 +0200
Adjust a jump label according to the current Linux coding style convention.
Generally, please don't send patches to fix checkpatch issues in existing code. Moreover, this particular "convention" is just something someone sneaked into CodingStyle, it's subjective, and it's probably going to be removed soon.
NAK.
BR, Jani.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
drivers/gpu/drm/gma500/mid_bios.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index a833568..cf4e605 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -242,7 +242,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr) gct_virtual = ioremap(addr + sizeof(vbt), sizeof(*gct) * vbt.panel_count); if (!gct_virtual)
goto out;
goto free_gct;@@ -270,7 +270,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr) dp_ti->vsync_pulse_width_lo = ti->vsync_pulse_width_lo;
ret = 0; -out:
- free_gct: kfree(gct); return ret;
}
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 10:32:12 +0200
One local variable was set to an error code before a concrete error situation was detected. Thus move the corresponding assignment into an if branch to indicate a software failure there.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/gma500/mid_bios.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index cf4e605..3caee42 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -230,7 +230,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr) struct gct_r10 *gct; struct oaktrail_timing_info *dp_ti = &dev_priv->gct_data.DTD; struct gct_r10_timing_info *ti; - int ret = -1; + int ret;
if (read_vbt_r10(addr, &vbt)) return -1; @@ -241,8 +241,10 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
gct_virtual = ioremap(addr + sizeof(vbt), sizeof(*gct) * vbt.panel_count); - if (!gct_virtual) + if (!gct_virtual) { + ret = -1; goto free_gct; + } memcpy_fromio(gct, gct_virtual, sizeof(*gct)); iounmap(gct_virtual);
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 10:34:28 +0200
Adjust the indentation for a single function call parameter here.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/gma500/mid_bios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index 3caee42..9004d30 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -240,7 +240,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr) return -1;
gct_virtual = ioremap(addr + sizeof(vbt), - sizeof(*gct) * vbt.panel_count); + sizeof(*gct) * vbt.panel_count); if (!gct_virtual) { ret = -1; goto free_gct;
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 10:36:19 +0200
A single error message should be sufficient to inform about the detection of an unknown GCT revision at the end. Thus return after the logging call in this case directly.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/gma500/mid_bios.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index 9004d30..e5cece0 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -320,6 +320,7 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv) break; default: dev_err(dev->dev, "Unknown revision of GCT!\n"); + return; }
out:
On Tue, 20 Sep 2016, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 10:36:19 +0200
A single error message should be sufficient to inform about the detection of an unknown GCT revision at the end. Thus return after the logging call in this case directly.
Did you test this?
BR, Jani.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
drivers/gpu/drm/gma500/mid_bios.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index 9004d30..e5cece0 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -320,6 +320,7 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv) break; default: dev_err(dev->dev, "Unknown revision of GCT!\n");
return;out:
A single error message should be sufficient to inform about the detection of an unknown GCT revision at the end. Thus return after the logging call in this case directly.
Did you test this?
What is your software development opinion for this update suggestion?
Regards, Markus
On Tue, Sep 20, 2016 at 01:07:35PM +0300, Jani Nikula wrote:
On Tue, 20 Sep 2016, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 10:36:19 +0200
A single error message should be sufficient to inform about the detection of an unknown GCT revision at the end. Thus return after the logging call in this case directly.
Did you test this?
Don't be a dummy... This is easy to review an it fixes a bug.
I'm fine with you NAKing all these patches based on who they are from. I mostly just delete these without responding because the guy has history of introducing bugs and never listens to feedback. But asking pointless rhetorical questions is not helpful.
A lot of people are CC'd and you're wasting everyone's time by asking questions where you know the answer.
regards, dan carpenter
A single error message should be sufficient to inform about the detection of an unknown GCT revision at the end. Thus return after the logging call in this case directly.
Did you test this?
Don't be a dummy... This is easy to review an it fixes a bug.
Thanks for this kind of constructive feedback.
I'm fine with you NAKing all these patches based on who they are from.
Would you like to clarify such an information a bit more?
I mostly just delete these without responding because the guy has history of introducing bugs and never listens to feedback.
I admit that I'll stumble on programming mistakes again occasionally as another ordinary free software developer who is struggling various open issues.
I am listening to various feedback. My responses might not be pleasing enough for you. Are you looking for any special information to improve a corresponding discussion?
Regards, Markus
On Tue, Sep 20, 2016 at 01:03:06PM +0200, SF Markus Elfring wrote:
Are you looking for any special information to improve a corresponding discussion?
If you restricted yourself to only sending bug fixes and not sending any more cleanups that would be good.
Please stop sending clean up patches.
regards, dan carpenter
If you restricted yourself to only sending bug fixes and not sending any more cleanups that would be good.
Thanks for another bit of constructive feedback.
Please stop sending clean up patches.
This will not happen for a while.
I am in the process of informing various developers about some software update opportunities. The proposed changes will belong to a mixture of error categories as you observe them so far usually.
The involved static source code analysis will point more details out for further considerations, won't it?
Regards, Markus
On Tue, 20 Sep 2016, Dan Carpenter dan.carpenter@oracle.com wrote:
Don't be a dummy... This is easy to review an it fixes a bug.
I'm fine with you NAKing all these patches based on who they are from. I mostly just delete these without responding because the guy has history of introducing bugs and never listens to feedback. But asking pointless rhetorical questions is not helpful.
A lot of people are CC'd and you're wasting everyone's time by asking questions where you know the answer.
Fair enough, sorry for the noise.
To be honest, I did only look at the patches, not who they were from. We have CI for drm/i915, but I don't think it's constructive to keep changing drivers like this where the upstream isn't actively developed and tested. But I digress. It's up to Patrik anyway.
BR, Jani.
On Tue, Sep 20, 2016 at 2:08 PM, Jani Nikula jani.nikula@linux.intel.com wrote:
On Tue, 20 Sep 2016, Dan Carpenter dan.carpenter@oracle.com wrote:
Don't be a dummy... This is easy to review an it fixes a bug.
In this particular case it might not be clear that an unknown GCT version causes a complete GCT failure so both messages are useful.
I'm fine with you NAKing all these patches based on who they are from. I mostly just delete these without responding because the guy has history of introducing bugs and never listens to feedback. But asking pointless rhetorical questions is not helpful.
A lot of people are CC'd and you're wasting everyone's time by asking questions where you know the answer.
Fair enough, sorry for the noise.
To be honest, I did only look at the patches, not who they were from. We have CI for drm/i915, but I don't think it's constructive to keep changing drivers like this where the upstream isn't actively developed and tested. But I digress. It's up to Patrik anyway.
Nothing in this series is very helpful so NAK. In general I'm not fond of trivial changes like this since it's hard to say what motivates the author. In theory it shouldn't matter but so far it's been directly related to the quality of the patches. I can help test changes for gma500 if needed but please make it worth my while.
Best regards Patrik
BR, Jani.
-- Jani Nikula, Intel Open Source Technology Center
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 10:40:22 +0200
* Adjust a jump target.
* Delete the explicit initialisation for the local variable "ret" which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/gma500/mid_bios.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index e5cece0..602d16f 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -284,7 +284,7 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv) u8 __iomem *vbt_virtual; struct mid_vbt_header vbt_header; struct pci_dev *pci_gfx_root = pci_get_bus_and_slot(0, PCI_DEVFN(2, 0)); - int ret = -1; + int ret;
/* Get the address of the platform config vbt */ pci_read_config_dword(pci_gfx_root, 0xFC, &addr); @@ -293,18 +293,18 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv) dev_dbg(dev->dev, "drm platform config address is %x\n", addr);
if (!addr) - goto out; + goto report_failure;
/* get the virtual address of the vbt */ vbt_virtual = ioremap(addr, sizeof(vbt_header)); if (!vbt_virtual) - goto out; + goto report_failure;
memcpy_fromio(&vbt_header, vbt_virtual, sizeof(vbt_header)); iounmap(vbt_virtual);
if (memcmp(&vbt_header.signature, "$GCT", 4)) - goto out; + goto report_failure;
dev_dbg(dev->dev, "GCT revision is %02x\n", vbt_header.revision);
@@ -322,9 +322,8 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv) dev_err(dev->dev, "Unknown revision of GCT!\n"); return; } - -out: if (ret) + report_failure: dev_err(dev->dev, "Unable to read GCT!"); else dev_priv->has_gct = true;
On Tue, 20 Sep 2016, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 10:40:22 +0200
- Adjust a jump target.
Please don't.
BR, Jani.
- Delete the explicit initialisation for the local variable "ret" which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
drivers/gpu/drm/gma500/mid_bios.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c index e5cece0..602d16f 100644 --- a/drivers/gpu/drm/gma500/mid_bios.c +++ b/drivers/gpu/drm/gma500/mid_bios.c @@ -284,7 +284,7 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv) u8 __iomem *vbt_virtual; struct mid_vbt_header vbt_header; struct pci_dev *pci_gfx_root = pci_get_bus_and_slot(0, PCI_DEVFN(2, 0));
- int ret = -1;
int ret;
/* Get the address of the platform config vbt */ pci_read_config_dword(pci_gfx_root, 0xFC, &addr);
@@ -293,18 +293,18 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv) dev_dbg(dev->dev, "drm platform config address is %x\n", addr);
if (!addr)
goto out;
goto report_failure;/* get the virtual address of the vbt */ vbt_virtual = ioremap(addr, sizeof(vbt_header)); if (!vbt_virtual)
goto out;
goto report_failure;memcpy_fromio(&vbt_header, vbt_virtual, sizeof(vbt_header)); iounmap(vbt_virtual);
if (memcmp(&vbt_header.signature, "$GCT", 4))
goto out;
goto report_failure;dev_dbg(dev->dev, "GCT revision is %02x\n", vbt_header.revision);
@@ -322,9 +322,8 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv) dev_err(dev->dev, "Unknown revision of GCT!\n"); return; }
-out: if (ret)
- report_failure: dev_err(dev->dev, "Unable to read GCT!"); else dev_priv->has_gct = true;
On Tue, Sep 20, 2016 at 01:08:12PM +0300, Jani Nikula wrote:
On Tue, 20 Sep 2016, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 20 Sep 2016 10:40:22 +0200
- Adjust a jump target.
Please don't.
Also there is nothing in CodingStyle that prohibits out: labels. I wrote that section and I wrote it in a deliberately way because no one wants to see a bunch of "cleanup" patches that change the label names for no reason.
People have been complaining about this in the kernel-summit mailing list as if CodingStyle bans out labels and that it's my fault. Neither of these complaints are accurate.
regards, dan carpenter
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Jani Nikula -
Patrik Jakobsson -
SF Markus Elfring