[PATCH] [media] v4l2-common: Delete an unnecessary check before the function call "spi_unregister_device"
From: Markus Elfring Date: Tue, 19 Jul 2016 19:54:16 +0200 The spi_unregister_device() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/media/v4l2-core/v4l2-common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 5b80850..57cfe26a 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -291,7 +291,7 @@ struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev, error: /* If we have a client but no subdev, then something went wrong and we must unregister the client. */ - if (spi && sd == NULL) + if (!sd) spi_unregister_device(spi); return sd; -- 2.9.2
[PATCH] [media] tw686x: Delete an unnecessary check before the function call "video_unregister_device"
From: Markus Elfring
Date: Tue, 19 Jul 2016 21:24:26 +0200
The video_unregister_device() function tests whether its argument is NULL
and then returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/media/pci/tw686x/tw686x-video.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/pci/tw686x/tw686x-video.c
b/drivers/media/pci/tw686x/tw686x-video.c
index cdb16de..4475a9d9 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1093,8 +1093,7 @@ void tw686x_video_free(struct tw686x_dev *dev)
for (ch = 0; ch < max_channels(dev); ch++) {
struct tw686x_video_channel *vc = &dev->video_channels[ch];
- if (vc->device)
- video_unregister_device(vc->device);
+ video_unregister_device(vc->device);
if (dev->dma_ops->free)
for (pb = 0; pb < 2; pb++)
--
2.9.2
Re: [PATCH] WiMAX-i2400m: Delete an unnecessary check before the function call "kfree_skb"
> From: Markus Elfring > Date: Mon, 16 Nov 2015 11:17:55 +0100 > > The kfree_skb() function tests whether its argument is NULL and then > returns immediately. Thus the test around the call is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring > --- > drivers/net/wimax/i2400m/control.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wimax/i2400m/control.c > b/drivers/net/wimax/i2400m/control.c > index 4c41790..4de22b7 100644 > --- a/drivers/net/wimax/i2400m/control.c > +++ b/drivers/net/wimax/i2400m/control.c > @@ -644,7 +644,7 @@ void i2400m_msg_to_dev_cancel_wait(struct i2400m *i2400m, > int code) > > spin_lock_irqsave(&i2400m->rx_lock, flags); > ack_skb = i2400m->ack_skb; > - if (ack_skb && !IS_ERR(ack_skb)) > + if (!IS_ERR(ack_skb)) > kfree_skb(ack_skb); > i2400m->ack_skb = ERR_PTR(code); > spin_unlock_irqrestore(&i2400m->rx_lock, flags); > How do you think about to integrate this update suggestion into another source code repository? Regards, Markus
[PATCH] of: Delete an unnecessary check before the function call "of_node_put"
From: Markus Elfring Date: Tue, 19 Jul 2016 22:42:22 +0200 The of_node_put() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/of/base.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/of/base.c b/drivers/of/base.c index a4b6087..2601167 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -1609,8 +1609,7 @@ static int __of_parse_phandle_with_args(const struct device_node *np, */ err: - if (it.node) - of_node_put(it.node); + of_node_put(it.node); return rc; } -- 2.9.2
Re: [PATCH] sh-DWARF: Delete unnecessary checks before the function call "mempool_destroy"
> From: Markus Elfring > Date: Mon, 16 Nov 2015 08:20:36 +0100 > > The mempool_destroy() function tests whether its argument is NULL > and then returns immediately. Thus the test around the calls is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring > --- > arch/sh/kernel/dwarf.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/arch/sh/kernel/dwarf.c b/arch/sh/kernel/dwarf.c > index 9d209a0..e1d751a 100644 > --- a/arch/sh/kernel/dwarf.c > +++ b/arch/sh/kernel/dwarf.c > @@ -1009,10 +1009,8 @@ static void __init dwarf_unwinder_cleanup(void) > rbtree_postorder_for_each_entry_safe(cie, next_cie, &cie_root, node) > kfree(cie); > > - if (dwarf_reg_pool) > - mempool_destroy(dwarf_reg_pool); > - if (dwarf_frame_pool) > - mempool_destroy(dwarf_frame_pool); > + mempool_destroy(dwarf_reg_pool); > + mempool_destroy(dwarf_frame_pool); > kmem_cache_destroy(dwarf_reg_cachep); > kmem_cache_destroy(dwarf_frame_cachep); > } > Would you like to integrate this update suggestion into another source code repository? Regards, Markus
Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection
@@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend
*pending_req,
tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
if (!tmr) {
target_put_sess_cmd(se_cmd);
- goto err;
+ goto do_resp;
}
>>>
>>> Hmm, I'm not convinced this is an improvement.
>>>
>>> I'd rather rename the new error label to "put_cmd" and get rid of the
>>> braces in above if statement:
>>>
>>> - if (!tmr) {
>>> - target_put_sess_cmd(se_cmd);
>>> - goto err;
>>> - }
>>> + if (!tmr)
>>> + goto put_cmd;
>>>
>>> and then in the error path:
>>>
>>> -err:
>>> +put_cmd:
>>> + target_put_sess_cmd(se_cmd);
>>
>> I am unsure on the relevance of this function on such a source position.
>> Would it make sense to move it further down at the end?
>
> You only want to call it in the first error case (allocation failure).
Thanks for your clarification.
I find that my update suggestion (from Saturday) is still appropriate
in this case.
https://lkml.org/lkml/2016/7/16/172
>>> +free_tmr:
>>> kfree(tmr);
>>
>> How do you think about to skip this function call after a memory
>> allocation failure?
>
> I think this just doesn't matter. If it were a hot path, yes. But trying
> to do micro-optimizations in an error path is just not worth the effort.
Would you like to reduce also the amount of function calls in such special
run-time situations?
> I like a linear error path containing all the needed cleanups best.
I would prefer to keep the discussed single function call within
the basic block of the if statement.
Have we got different opinions about the shown implementation details?
Regards,
Markus
[PATCH v2 0/3] xen-scsiback: Fine-tuning for scsiback_device_action()
From: Markus Elfring Date: Wed, 20 Jul 2016 13:20:04 +0200 Further update suggestions were taken into account after a patch was applied from static source code analysis. Markus Elfring (3): Delete an unnecessary check before the function call "kfree" Rename jump labels in scsiback_device_action() Pass a failure indication as a constant drivers/xen/xen-scsiback.c | 19 +-- 1 file changed, 9 insertions(+), 10 deletions(-) -- 2.9.2
[PATCH v2 1/3] xen-scsiback: Delete an unnecessary check before the function call "kfree"
From: Markus Elfring Date: Tue, 19 Jul 2016 15:42:19 +0200 The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- v2: Rebased on source files from "Linux next-20160719" drivers/xen/xen-scsiback.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c index d6950e0..4a48c06 100644 --- a/drivers/xen/xen-scsiback.c +++ b/drivers/xen/xen-scsiback.c @@ -627,8 +627,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, transport_generic_free_cmd(&pending_req->se_cmd, 1); return; err: - if (tmr) - kfree(tmr); + kfree(tmr); scsiback_do_resp_with_sense(NULL, err, 0, pending_req); } -- 2.9.2
[PATCH v2 2/3] xen-scsiback: Rename jump labels in scsiback_device_action()
From: Markus Elfring
Date: Wed, 20 Jul 2016 13:03:16 +0200
* Adjust jump targets according to the Linux coding style convention.
* A bit of refactoring for the control flow
Suggested-by: Jürgen Groß
Signed-off-by: Markus Elfring
---
v2: Rebased on source files from "Linux next-20160719"
Changes from a bit of code review
drivers/xen/xen-scsiback.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 4a48c06..eb274df 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -604,10 +604,8 @@ static void scsiback_device_action(struct vscsibk_pend
*pending_req,
int rc, err = FAILED;
tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
- if (!tmr) {
- target_put_sess_cmd(se_cmd);
- goto err;
- }
+ if (!tmr)
+ goto put_cmd;
init_waitqueue_head(&tmr->tmr_wait);
@@ -616,7 +614,7 @@ static void scsiback_device_action(struct vscsibk_pend
*pending_req,
unpacked_lun, tmr, act, GFP_KERNEL,
tag, TARGET_SCF_ACK_KREF);
if (rc)
- goto err;
+ goto free_tmr;
wait_event(tmr->tmr_wait, atomic_read(&tmr->tmr_complete));
@@ -626,7 +624,9 @@ static void scsiback_device_action(struct vscsibk_pend
*pending_req,
scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
transport_generic_free_cmd(&pending_req->se_cmd, 1);
return;
-err:
+put_cmd:
+ target_put_sess_cmd(se_cmd);
+free_tmr:
kfree(tmr);
scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
}
--
2.9.2
[PATCH v2 3/3] xen-scsiback: Pass a failure indication as a constant
From: Markus Elfring Date: Wed, 20 Jul 2016 13:12:33 +0200 Pass the constant "FAILED" in a function call directly instead of using an intialisation for a local variable. Signed-off-by: Markus Elfring --- v2: Rebased on source files from "Linux next-20160719" drivers/xen/xen-scsiback.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c index eb274df..fa08ec6 100644 --- a/drivers/xen/xen-scsiback.c +++ b/drivers/xen/xen-scsiback.c @@ -601,7 +601,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, struct se_cmd *se_cmd = &pending_req->se_cmd; struct scsiback_tmr *tmr; u64 unpacked_lun = pending_req->v2p->lun; - int rc, err = FAILED; + int rc, err; tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL); if (!tmr) @@ -628,7 +628,7 @@ put_cmd: target_put_sess_cmd(se_cmd); free_tmr: kfree(tmr); - scsiback_do_resp_with_sense(NULL, err, 0, pending_req); + scsiback_do_resp_with_sense(NULL, FAILED, 0, pending_req); } /* -- 2.9.2
Re: [PATCH] SRAM: Delete an unnecessary check before the function call "of_node_put"
> From: Markus Elfring > Date: Fri, 6 Nov 2015 10:36:49 +0100 > > The of_node_put() function tests whether its argument is NULL and then > returns immediately. Thus the test around the call is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring > --- > drivers/misc/sram.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c > index 736dae7..34287f7 100644 > --- a/drivers/misc/sram.c > +++ b/drivers/misc/sram.c > @@ -326,9 +326,7 @@ static int sram_reserve_regions(struct sram_dev *sram, > struct resource *res) > } > > err_chunks: > - if (child) > - of_node_put(child); > - > + of_node_put(child); > kfree(rblocks); > > return ret; > How do you think about to integrate this update suggestion into another source code repository? Regards, Markus
[PATCH] cxl: Delete an unnecessary check before the function call "of_node_put"
From: Markus Elfring Date: Wed, 20 Jul 2016 15:10:32 +0200 The of_node_put() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/misc/cxl/of.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c index edc4583..333256a 100644 --- a/drivers/misc/cxl/of.c +++ b/drivers/misc/cxl/of.c @@ -490,8 +490,7 @@ int cxl_of_probe(struct platform_device *pdev) adapter->slices = 0; } - if (afu_np) - of_node_put(afu_np); + of_node_put(afu_np); return 0; } -- 2.9.2
[PATCH] drm/atomic: Delete an unnecessary check before drm_property_unreference_blob()
From: Markus Elfring Date: Wed, 20 Jul 2016 17:54:32 +0200 The drm_property_unreference_blob() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/gpu/drm/drm_atomic.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3cee084..8d2f111 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -404,8 +404,7 @@ drm_atomic_replace_property_blob(struct drm_property_blob **blob, if (old_blob == new_blob) return; - if (old_blob) - drm_property_unreference_blob(old_blob); + drm_property_unreference_blob(old_blob); if (new_blob) drm_property_reference_blob(new_blob); *blob = new_blob; -- 2.9.2
[PATCH] GPU-DRM-sun4i: Delete an unnecessary check before drm_fbdev_cma_hotplug_event()
From: Markus Elfring
Date: Wed, 20 Jul 2016 18:32:34 +0200
The drm_fbdev_cma_hotplug_event() function tests whether its argument
is NULL and then returns immediately.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/sun4i/sun4i_framebuffer.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
index a0b30c2..70688fe 100644
--- a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
@@ -20,8 +20,7 @@ static void sun4i_de_output_poll_changed(struct drm_device
*drm)
{
struct sun4i_drv *drv = drm->dev_private;
- if (drv->fbdev)
- drm_fbdev_cma_hotplug_event(drv->fbdev);
+ drm_fbdev_cma_hotplug_event(drv->fbdev);
}
static const struct drm_mode_config_funcs sun4i_de_mode_config_funcs = {
--
2.9.2
[PATCH] GPU-DRM-nouveau: Delete an unnecessary check before the function call "pci_dev_put"
From: Markus Elfring Date: Wed, 20 Jul 2016 19:43:27 +0200 The pci_dev_put() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 66c1280..7b09841 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -525,8 +525,7 @@ nouveau_drm_unload(struct drm_device *dev) nouveau_vga_fini(drm); nvif_device_fini(&drm->device); - if (drm->hdmi_device) - pci_dev_put(drm->hdmi_device); + pci_dev_put(drm->hdmi_device); nouveau_cli_destroy(&drm->client); return 0; } -- 2.9.2
Re: staging: ks7010: Rename jump labels
>> Adjust jump targets according to the Linux coding style convention. > > Really? Is that documented somewhere? How do you think about information from the chapter "7: Centralized exiting of functions"? https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?id=47ef4ad2684d380dd6d596140fb79395115c3950#n389 > Quoting Jean Delvare: > > "> It is generally accepted to indent labels with a single space. This > > avoids breaking the -p option of diff." Would you like to take another look at the warning "LEADING_SPACE"? https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=47ef4ad2684d380dd6d596140fb79395115c3950#n3004 Does such a check need further considerations? > So, NACK for now unless we know 'diff' has been fixed. I am also curious on corresponding software evolution. Regards, Markus
Re: [PATCH 3/9] staging: ks7010: Return directly after a failed kmalloc()
>>> @@ -713,10 +713,8 @@ static int ks7010_sdio_update_index(struct
>>> ks_wlan_private *priv, u32 index)
>>> unsigned char *data_buf;
>>>
>>> data_buf = kmalloc(sizeof(u32), GFP_KERNEL);
>>> - if (!data_buf) {
>>> - rc = 1;
>>> - goto error_out;
>>> - }
>>> + if (!data_buf)
>>> + return 1;
>>
>> One could rather wonder why the function has such strange error values...
>
> Agreed. Markus, can you check if we can use -ENOMEM in those places.
I find that I do not know this software good enough at the moment
so that I could safely decide on the shown special error values.
I guess that further clarification might be needed for affected
implementation details.
Regards,
Markus
Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the variable "retval"
>>> @@ -90,7 +90,6 @@ static int ks7010_sdio_write(struct ks_wlan_private
>>> *priv, unsigned int address,
>>> void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
>>> {
>>> unsigned char rw_data;
>>> - int retval;
>>>
>>> DPRINTK(4, "\n");
>>>
>>> @@ -99,9 +98,10 @@ void ks_wlan_hw_sleep_doze_request(struct
>>> ks_wlan_private *priv)
>>>
>>> if (atomic_read(&priv->sleepstatus.status) == 0) {
>>> rw_data = GCR_B_DOZE;
>>> - retval =
>>> - ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
>>> - if (retval) {
>>> + if (ks7010_sdio_write(priv,
>>> + GCR_B,
>>> + &rw_data,
>>> + sizeof(rw_data))) {
>>
>> A multi-line function call in an if test does not look nice at all. The
>> original code was an easy-to-read expectable pattern.
>
> I agree. I am not strict on the 80 char limit, especially in cases like
> the above.
Would you try an other source code formatting for the suggested change pattern?
Regards,
Markus
Re: staging: ks7010: Rename jump labels
> > How do you think about information from the chapter "7: Centralized exiting > > of functions"? > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?id=47ef4ad2684d380dd6d596140fb79395115c3950#n389 > > I'm not impressed by this piece of documentation. I would also appreciate further improvements there. > Back to the lack of space before labels, it's at best a personal preference. > If you insist on standardizing, I'd call it a bug in the documentation, > which should be fixed. One space before label is the way to go. Would you like to contribute another patch for such a coding style issue? > That being said... checkpatch does not complain about leading space > before labels. Not even with --strict. So why are you mentioning it here? How do you think about a similar software update? staging: lustre: Fix a jump label position in osc_get_info() https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c71d264543f759fea147734cb63de36397817534 https://lkml.org/lkml/2015/12/21/401 > Just looking at the thread on lkml makes me feel dizzy. I understand your concern to some degree. > When you are about to send that amount of messages, > you should pause and think again. This happened already. > Is it really worth it? I came to such a conclusion for the shown source code clean-up. Some software developers can also support it. > I think I'd be less annoyed by regular spam. It can be harder occasionally to become familiar and comfortable with the suggested change pattern. Could you accept other details from my update suggestion? Regards, Markus
Re: staging: ks7010: Delete unnecessary uses of the variable "retval"
> I think the original code was fine. I suggest to reconsider involved implementation details once more. > x = blah(); if (x) ... is a perfectly familiar kernel coding pattern. I can agree to such a general information. > There is no benefit in terms of performance It might be possible that a good compiler can also optimise some unnecessary variable accesses away. Examples for further background information: * "Minimize local variables" https://eventhelix.com/realtimemantra/basics/optimizingcandcppcode.htm#Minimize%20Local%20Variables * "Temporary Objects" by Danny Kalev http://www.informit.com/guides/content.aspx?g=cplusplus&seqNum=198 > or understandability in dropping the variable. I guess that we have got different opinions on such an aspect. * Do you really want to assign every return value from a function call to an extra variable before it is used again? * How many reading and understanding capacity do you need for each extra variable? Regards, Markus
Re: staging: ks7010: Delete unnecessary uses of the variable "retval"
> if (atomic_read(&priv->sleepstatus.status) == 0) {
> rw_data = GCR_B_DOZE;
> - retval =
> - ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
> - if (retval) {
> + if (ks7010_sdio_write(priv,
> + GCR_B,
> + &rw_data,
> + sizeof(rw_data))) {
A multi-line function call in an if test does not look nice at all. The
original code was an easy-to-read expectable pattern.
>>>
>>> I agree. I am not strict on the 80 char limit, especially in cases like
>>> the above.
Will this line length limitation trigger any more collateral evolution
in the discussed software module?
>> Would you try an other source code formatting for the suggested change
>> pattern?
>
> I don't understand the question?
Can you follow expectations around the proposed refactoring of any
function implementations?
Regards,
Markus
Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable initialisations
>> @@ -323,14 +323,14 @@ static void tx_device_task(void *dev)
>> {
>> struct ks_wlan_private *priv = (struct ks_wlan_private *)dev;
>> struct tx_device_buffer *sp;
>> -int rc = 0;
>>
>> DPRINTK(4, "\n");
>> if (cnt_txqbody(priv) > 0
>> && atomic_read(&priv->psstatus.status) != PS_SNOOZE) {
>> sp = &priv->tx_dev.tx_dev_buff[priv->tx_dev.qhead];
>> if (priv->dev_state >= DEVICE_STATE_BOOT) {
>> -rc = write_to_device(priv, sp->sendp, sp->size);
>> +int rc = write_to_device(priv, sp->sendp, sp->size);
>
> This does not look appealing to me, neither the declaration in the middle
> of the function, nor the intiialization to the result of a complex
> expression, nor the separation of the call and the error checking code by
> a blank line. There is nothing wrong with having the rc variable be
> declared at the the top of the function, in its normal place.
* Do you occasionally care for a refactoring like "Reduce scope of variable"?
http://refactoring.com/catalog/reduceScopeOfVariable.html
* How do you think about to remove the extra assignment at the beginning
of this function implementation?
Regards,
Markus
Re: staging: ks7010: Delete three unnecessary variable initialisations
>> * Do you occasionally care for a refactoring like "Reduce scope of variable"? >> >> http://refactoring.com/catalog/reduceScopeOfVariable.html > > Probably not. Certainly not in this case. In which use cases would the suggested change pattern be more interesting for you? Regards, Markus
Re: staging: ks7010: Rename jump labels
> That being said... checkpatch does not complain about leading space > before labels. Not even with --strict. So why are you mentioning it here? I remembered a warning like "INDENTED_LABEL" instead. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=92d21ac74a9e3c09b0b01c764e530657e4c85c49#n4326 How do you generally think about jump label renaming? Regards, Markus
[PATCH] GPU-DRM-Exynos: Delete an unnecessary check before the function call "vunmap"
From: Markus Elfring
Date: Thu, 21 Jul 2016 19:23:25 +0200
The vunmap() function performs also input parameter validation.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index dd09117..4cfb39d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -269,8 +269,7 @@ static void exynos_drm_fbdev_destroy(struct drm_device *dev,
struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem;
struct drm_framebuffer *fb;
- if (exynos_gem->kvaddr)
- vunmap(exynos_gem->kvaddr);
+ vunmap(exynos_gem->kvaddr);
/* release drm framebuffer and real buffer */
if (fb_helper->fb && fb_helper->fb->funcs) {
--
2.9.2
Re: staging: ks7010: Rename jump labels
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=92d21ac74a9e3c09b0b01c764e530657e4c85c49#n4326 > > "#goto labels aren't indented, allow a single space however" > > Can't be clearer :-) Should such information from a comment in this script be also explicitly mentioned in the document "CodingStyle" anyhow? How should the support for indentation of jump labels with a single space be expressed in all relevant Linux development documentation? >> How do you generally think about jump label renaming? > > Renaming from "out0:", "out1:" etc to something meaningful, yes. I suggest to take another look at such identifiers. Would you like to support the renaming of a label like "error_out1" (in the function "ks7010_upload_firmware" for example)? > Did you have anything else in mind? Not really for this update suggestion. Will the software evolution be continued also with information from the topic "Source code review around jump label usage"? https://lkml.org/lkml/2015/12/11/378 http://article.gmane.org/gmane.linux.kernel/2106190 Regards, Markus
Re: staging: ks7010: Delete unnecessary uses of the variable "retval"
>> Can you follow expectations around the proposed refactoring of any
>> function implementations?
>
> I don't understand both questions. Maybe you need to give examples?
I suggest to try the following script (semantic patch for working with
the Coccinelle software) out on the discussed source files.
@checking_function_calls_directly@
identifier checker, retval, work;
expression list el;
statement is, es;
type rt;
@@
rt checker(...)
{
<+...
-retval = work(el);
if (
-retval
+work(el)
)
is
else
es
...+>
}
Do you find such a source code transformation useful?
Regards,
Markus
[PATCH] GPU-DRM-OMAP: Delete unnecessary checks before two function calls
From: Markus Elfring
Date: Fri, 22 Jul 2016 08:28:31 +0200
The following functions test whether their argument is NULL and then
return immediately.
* backlight_device_unregister
* drm_gem_object_unreference_unlocked
Thus the test around the calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 3 +--
drivers/gpu/drm/omapdrm/omap_fb.c | 4 ++--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
index 1b0cf2d..0eae8af 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
@@ -1284,8 +1284,7 @@ static int dsicm_probe(struct platform_device *pdev)
return 0;
err_sysfs_create:
- if (bldev != NULL)
- backlight_device_unregister(bldev);
+ backlight_device_unregister(bldev);
err_bl:
destroy_workqueue(ddata->workqueue);
err_reg:
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
b/drivers/gpu/drm/omapdrm/omap_fb.c
index 983c8cf..31f5178 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -115,8 +115,8 @@ static void omap_framebuffer_destroy(struct drm_framebuffer
*fb)
for (i = 0; i < n; i++) {
struct plane *plane = &omap_fb->planes[i];
- if (plane->bo)
- drm_gem_object_unreference_unlocked(plane->bo);
+
+ drm_gem_object_unreference_unlocked(plane->bo);
}
kfree(omap_fb);
--
2.9.2
Re: staging: ks7010: Return directly after a failed kmalloc()
>> I guess that further clarification might be needed for affected >> implementation details. > > That's OK, too. > > Acked-by: Wolfram Sang Does this acknowledgement include also the acceptance for the suggested change around calls of the functions "sdio_claim_host" and "sdio_release_host" within the implementation of the function "ks7010_upload_firmware"? https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/staging/ks7010/ks7010_sdio.c?id=13123042d0dbf7635f052efc2ae69fd9af624f1d#n771 Regards, Markus
[PATCH] GPU-DRM-GMA500: Delete unnecessary checks before two function calls
From: Markus Elfring
Date: Fri, 22 Jul 2016 10:30:30 +0200
The functions pci_dev_put() and psb_intel_i2c_destroy() test whether
their argument is NULL and then return immediately.
Thus the tests around their calls are not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 3 +--
drivers/gpu/drm/gma500/cdv_intel_lvds.c | 9 +++--
drivers/gpu/drm/gma500/psb_drv.c| 6 ++
drivers/gpu/drm/gma500/psb_intel_lvds.c | 9 +++--
4 files changed, 9 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
index 28f9d90..563f193 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
@@ -246,8 +246,7 @@ static void cdv_hdmi_destroy(struct drm_connector
*connector)
{
struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
- if (gma_encoder->i2c_bus)
- psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+ psb_intel_i2c_destroy(gma_encoder->i2c_bus);
drm_connector_unregister(connector);
drm_connector_cleanup(connector);
kfree(connector);
diff --git a/drivers/gpu/drm/gma500/cdv_intel_lvds.c
b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
index 813ef23..38dc890 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
@@ -444,8 +444,7 @@ static void cdv_intel_lvds_destroy(struct drm_connector
*connector)
{
struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
- if (gma_encoder->i2c_bus)
- psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+ psb_intel_i2c_destroy(gma_encoder->i2c_bus);
drm_connector_unregister(connector);
drm_connector_cleanup(connector);
kfree(connector);
@@ -780,12 +779,10 @@ out:
failed_find:
mutex_unlock(&dev->mode_config.mutex);
printk(KERN_ERR "Failed find\n");
- if (gma_encoder->ddc_bus)
- psb_intel_i2c_destroy(gma_encoder->ddc_bus);
+ psb_intel_i2c_destroy(gma_encoder->ddc_bus);
failed_ddc:
printk(KERN_ERR "Failed DDC\n");
- if (gma_encoder->i2c_bus)
- psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+ psb_intel_i2c_destroy(gma_encoder->i2c_bus);
failed_blc_i2c:
printk(KERN_ERR "Failed BLC\n");
drm_encoder_cleanup(encoder);
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 82b8ce4..50eb944f 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -210,10 +210,8 @@ static int psb_driver_unload(struct drm_device *dev)
iounmap(dev_priv->aux_reg);
dev_priv->aux_reg = NULL;
}
- if (dev_priv->aux_pdev)
- pci_dev_put(dev_priv->aux_pdev);
- if (dev_priv->lpc_pdev)
- pci_dev_put(dev_priv->lpc_pdev);
+ pci_dev_put(dev_priv->aux_pdev);
+ pci_dev_put(dev_priv->lpc_pdev);
/* Destroy VBT data */
psb_intel_destroy_bios(dev);
diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c
b/drivers/gpu/drm/gma500/psb_intel_lvds.c
index b1b9331..e55733c 100644
--- a/drivers/gpu/drm/gma500/psb_intel_lvds.c
+++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c
@@ -561,8 +561,7 @@ void psb_intel_lvds_destroy(struct drm_connector *connector)
struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
struct psb_intel_lvds_priv *lvds_priv = gma_encoder->dev_priv;
- if (lvds_priv->ddc_bus)
- psb_intel_i2c_destroy(lvds_priv->ddc_bus);
+ psb_intel_i2c_destroy(lvds_priv->ddc_bus);
drm_connector_unregister(connector);
drm_connector_cleanup(connector);
kfree(connector);
@@ -835,11 +834,9 @@ out:
failed_find:
mutex_unlock(&dev->mode_config.mutex);
- if (lvds_priv->ddc_bus)
- psb_intel_i2c_destroy(lvds_priv->ddc_bus);
+ psb_intel_i2c_destroy(lvds_priv->ddc_bus);
failed_ddc:
- if (lvds_priv->i2c_bus)
- psb_intel_i2c_destroy(lvds_priv->i2c_bus);
+ psb_intel_i2c_destroy(lvds_priv->i2c_bus);
failed_blc_i2c:
drm_encoder_cleanup(encoder);
drm_connector_cleanup(connector);
--
2.9.2
[PATCH] drm/mgag200: Delete an unnecessary check before drm_gem_object_unreference_unlocked()
From: Markus Elfring
Date: Fri, 22 Jul 2016 11:20:27 +0200
The drm_gem_object_unreference_unlocked() function tests whether
its argument is NULL and then returns immediately.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/mgag200/mgag200_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c
b/drivers/gpu/drm/mgag200/mgag200_main.c
index 615cbb0..13798b3 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -17,8 +17,8 @@
static void mga_user_framebuffer_destroy(struct drm_framebuffer *fb)
{
struct mga_framebuffer *mga_fb = to_mga_framebuffer(fb);
- if (mga_fb->obj)
- drm_gem_object_unreference_unlocked(mga_fb->obj);
+
+ drm_gem_object_unreference_unlocked(mga_fb->obj);
drm_framebuffer_cleanup(fb);
kfree(fb);
}
--
2.9.2
[PATCH] GPU-DRM: Delete an unnecessary check before drm_property_unreference_blob()
From: Markus Elfring
Date: Fri, 22 Jul 2016 12:48:12 +0200
The drm_property_unreference_blob() function tests whether its argument
is NULL and then returns immediately.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 24aa3ba..07541d0 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -112,9 +112,7 @@ static void mtk_drm_crtc_reset(struct drm_crtc *crtc)
struct mtk_crtc_state *state;
if (crtc->state) {
- if (crtc->state->mode_blob)
- drm_property_unreference_blob(crtc->state->mode_blob);
-
+ drm_property_unreference_blob(crtc->state->mode_blob);
state = to_mtk_crtc_state(crtc->state);
memset(state, 0, sizeof(*state));
} else {
--
2.9.2
[PATCH] drm/vmwgfx: Delete an unnecessary check before the function call "vfree"
From: Markus Elfring
Date: Fri, 22 Jul 2016 13:31:00 +0200
The vfree() function performs also input parameter validation.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 1a1a87c..dc5beff 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -3625,9 +3625,7 @@ static int vmw_resize_cmd_bounce(struct vmw_sw_context
*sw_context,
(sw_context->cmd_bounce_size >> 1));
}
- if (sw_context->cmd_bounce != NULL)
- vfree(sw_context->cmd_bounce);
-
+ vfree(sw_context->cmd_bounce);
sw_context->cmd_bounce = vmalloc(sw_context->cmd_bounce_size);
if (sw_context->cmd_bounce == NULL) {
--
2.9.2
[PATCH] drm/qxl: Delete an unnecessary check before drm_gem_object_unreference_unlocked()
From: Markus Elfring
Date: Fri, 22 Jul 2016 14:14:54 +0200
The drm_gem_object_unreference_unlocked() function tests whether
its argument is NULL and then returns immediately.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/qxl/qxl_display.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index ad42968..3aef127 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -468,8 +468,7 @@ void qxl_user_framebuffer_destroy(struct drm_framebuffer
*fb)
{
struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
- if (qxl_fb->obj)
- drm_gem_object_unreference_unlocked(qxl_fb->obj);
+ drm_gem_object_unreference_unlocked(qxl_fb->obj);
drm_framebuffer_cleanup(fb);
kfree(qxl_fb);
}
--
2.9.2
[PATCH] drm/bridge: ps8622: Delete an unnecessary check before backlight_device_unregister()
From: Markus Elfring
Date: Fri, 22 Jul 2016 14:45:51 +0200
The backlight_device_unregister() function tests whether its argument
is NULL and then returns immediately.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/bridge/parade-ps8622.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c
b/drivers/gpu/drm/bridge/parade-ps8622.c
index 5cd8dd7..583b8ce 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -636,9 +636,7 @@ static int ps8622_remove(struct i2c_client *client)
{
struct ps8622_bridge *ps8622 = i2c_get_clientdata(client);
- if (ps8622->bl)
- backlight_device_unregister(ps8622->bl);
-
+ backlight_device_unregister(ps8622->bl);
drm_bridge_remove(&ps8622->bridge);
return 0;
--
2.9.2
[PATCH 0/4] GPU-DRM-Etnaviv: Fine-tuning for a few functions
From: Markus Elfring Date: Fri, 22 Jul 2016 17:34:32 +0200 Further update suggestions were taken into account after a patch was applied from static source code analysis. Markus Elfring (4): Delete unnecessary checks before two function calls Delete unnecessary if statement in __etnaviv_gem_new() Rename jump labels Optimize error handling in etnaviv_gem_new_userptr() drivers/gpu/drm/etnaviv/etnaviv_gem.c | 36 +-- 1 file changed, 13 insertions(+), 23 deletions(-) -- 2.9.2
[PATCH 1/4] GPU-DRM-Etnaviv: Delete unnecessary checks before two function calls
From: Markus Elfring
Date: Fri, 22 Jul 2016 15:56:15 +0200
The functions drm_gem_object_unreference_unlocked() and vunmap() perform
also input parameter validation.
Thus the tests around their calls are not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 8c6f750..8eee742 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -535,8 +535,7 @@ void etnaviv_gem_describe_objects(struct
etnaviv_drm_private *priv,
static void etnaviv_gem_shmem_release(struct etnaviv_gem_object *etnaviv_obj)
{
- if (etnaviv_obj->vaddr)
- vunmap(etnaviv_obj->vaddr);
+ vunmap(etnaviv_obj->vaddr);
put_pages(etnaviv_obj);
}
@@ -670,9 +669,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct
drm_device *dev,
return obj;
fail:
- if (obj)
- drm_gem_object_unreference_unlocked(obj);
-
+ drm_gem_object_unreference_unlocked(obj);
return ERR_PTR(ret);
}
--
2.9.2
[PATCH 2/4] GPU-DRM-Etnaviv: Delete unnecessary if statement in __etnaviv_gem_new()
From: Markus Elfring Date: Fri, 22 Jul 2016 16:45:22 +0200 Move a return statement into a block for successful function execution. Omit a duplicate check for the local variable "ret" then at the end. Signed-off-by: Markus Elfring --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 8eee742..851a8ba 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -661,13 +661,9 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, */ mapping = obj->filp->f_mapping; mapping_set_gfp_mask(mapping, GFP_HIGHUSER); + return obj; } - if (ret) - goto fail; - - return obj; - fail: drm_gem_object_unreference_unlocked(obj); return ERR_PTR(ret); -- 2.9.2
[PATCH 4/4] GPU-DRM-Etnaviv: Optimize error handling in etnaviv_gem_new_userptr()
From: Markus Elfring
Date: Fri, 22 Jul 2016 17:17:48 +0200
Refactor this function implementation so that the
drm_gem_object_unreference_unlocked() function will only be called once
in case of a failure according to the Linux coding style recommendation
for centralized exiting of functions.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 0a5c00c..007577c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -909,15 +909,12 @@ int etnaviv_gem_new_userptr(struct drm_device *dev,
struct drm_file *file,
get_task_struct(current);
ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
- if (ret) {
- drm_gem_object_unreference_unlocked(&etnaviv_obj->base);
- return ret;
- }
+ if (ret)
+ goto unreference;
ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle);
-
+unreference:
/* drop reference from allocate - handle holds it now */
drm_gem_object_unreference_unlocked(&etnaviv_obj->base);
-
return ret;
}
--
2.9.2
[PATCH 3/4] GPU-DRM-Etnaviv: Rename jump labels
From: Markus Elfring
Date: Fri, 22 Jul 2016 16:51:00 +0200
Adjust jump targets according to the Linux coding style convention.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 851a8ba..0a5c00c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -308,17 +308,17 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get(
mapping = NULL;
mutex_unlock(&gpu->mmu->lock);
if (mapping)
- goto out;
+ goto unlock;
} else {
mapping->use += 1;
- goto out;
+ goto unlock;
}
}
pages = etnaviv_gem_get_pages(etnaviv_obj);
if (IS_ERR(pages)) {
ret = PTR_ERR(pages);
- goto out;
+ goto unlock;
}
/*
@@ -330,7 +330,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get(
mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
if (!mapping) {
ret = -ENOMEM;
- goto out;
+ goto unlock;
}
INIT_LIST_HEAD(&mapping->scan_node);
@@ -349,7 +349,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get(
else
list_add_tail(&mapping->obj_node, &etnaviv_obj->vram_list);
-out:
+unlock:
mutex_unlock(&etnaviv_obj->lock);
if (ret)
@@ -646,7 +646,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct
drm_device *dev,
ret = etnaviv_gem_new_impl(dev, size, flags, NULL,
&etnaviv_gem_shmem_ops, &obj);
if (ret)
- goto fail;
+ goto unreference;
ret = drm_gem_object_init(dev, obj, size);
if (ret == 0) {
@@ -664,7 +664,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct
drm_device *dev,
return obj;
}
-fail:
+unreference:
drm_gem_object_unreference_unlocked(obj);
return ERR_PTR(ret);
}
--
2.9.2
[PATCH] ASoC: Intel: Skylake: Delete an unnecessary check before the function call "release_firmware"
From: Markus Elfring Date: Fri, 22 Jul 2016 18:58:14 +0200 The release_firmware() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- sound/soc/intel/skylake/skl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 2337748..cd59536 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -781,8 +781,7 @@ static void skl_remove(struct pci_dev *pci) struct hdac_ext_bus *ebus = pci_get_drvdata(pci); struct skl *skl = ebus_to_skl(ebus); - if (skl->tplg) - release_firmware(skl->tplg); + release_firmware(skl->tplg); if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev); -- 2.9.2
[PATCH] zsmalloc: Delete an unnecessary check before the function call "iput"
From: Markus Elfring
Date: Fri, 22 Jul 2016 19:54:20 +0200
The iput() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
mm/zsmalloc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 5e5237c..7b5fd2b 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -2181,8 +2181,7 @@ static int zs_register_migration(struct zs_pool *pool)
static void zs_unregister_migration(struct zs_pool *pool)
{
flush_work(&pool->free_work);
- if (pool->inode)
- iput(pool->inode);
+ iput(pool->inode);
}
/*
--
2.9.2
[PATCH] block: Delete an unnecessary check before the function call "kmem_cache_destroy"
From: Markus Elfring
Date: Fri, 22 Jul 2016 21:07:37 +0200
The kmem_cache_destroy() function tests whether its argument is NULL
and then returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
block/elevator.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/elevator.c b/block/elevator.c
index 7096c22..29dc177 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -846,8 +846,7 @@ int elv_register(struct elevator_type *e)
spin_lock(&elv_list_lock);
if (elevator_find(e->elevator_name)) {
spin_unlock(&elv_list_lock);
- if (e->icq_cache)
- kmem_cache_destroy(e->icq_cache);
+ kmem_cache_destroy(e->icq_cache);
return -EBUSY;
}
list_add_tail(&e->list, &elv_list);
--
2.9.2
[PATCH] block: Delete unnecessary checks before the function call "mempool_destroy"
From: Markus Elfring
Date: Fri, 22 Jul 2016 21:45:26 +0200
The mempool_destroy() function tests whether its argument is NULL
and then returns immediately. Thus the test around the calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
block/bio-integrity.c | 7 ++-
block/bio.c | 8 ++--
block/blk-core.c | 3 +--
3 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index f70cc3b..7506a3e 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -499,11 +499,8 @@ EXPORT_SYMBOL(bioset_integrity_create);
void bioset_integrity_free(struct bio_set *bs)
{
- if (bs->bio_integrity_pool)
- mempool_destroy(bs->bio_integrity_pool);
-
- if (bs->bvec_integrity_pool)
- mempool_destroy(bs->bvec_integrity_pool);
+ mempool_destroy(bs->bio_integrity_pool);
+ mempool_destroy(bs->bvec_integrity_pool);
}
EXPORT_SYMBOL(bioset_integrity_free);
diff --git a/block/bio.c b/block/bio.c
index 54ee384..21a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1843,12 +1843,8 @@ void bioset_free(struct bio_set *bs)
if (bs->rescue_workqueue)
destroy_workqueue(bs->rescue_workqueue);
- if (bs->bio_pool)
- mempool_destroy(bs->bio_pool);
-
- if (bs->bvec_pool)
- mempool_destroy(bs->bvec_pool);
-
+ mempool_destroy(bs->bio_pool);
+ mempool_destroy(bs->bvec_pool);
bioset_integrity_free(bs);
bio_put_slab(bs);
diff --git a/block/blk-core.c b/block/blk-core.c
index a687e9c..db8c6b4 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -636,8 +636,7 @@ int blk_init_rl(struct request_list *rl, struct
request_queue *q,
void blk_exit_rl(struct request_list *rl)
{
- if (rl->rq_pool)
- mempool_destroy(rl->rq_pool);
+ mempool_destroy(rl->rq_pool);
}
struct request_queue *blk_alloc_queue(gfp_t gfp_mask)
--
2.9.2
Re: [PATCH] PM-wakeup: Delete unnecessary checks before two function calls
Am 28.06.2015 um 12:21 schrieb SF Markus Elfring:
> From: Markus Elfring
> Date: Sun, 28 Jun 2015 12:14:43 +0200
>
> The functions dev_pm_disarm_wake_irq() and wakeup_source_unregister() test
> whether their argument is NULL and then return immediately.
> Thus the test around the calls is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring
> ---
> drivers/base/power/wakeup.c | 10 +++---
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 40f7160..3741bc2 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -341,8 +341,7 @@ void device_wakeup_arm_wake_irqs(void)
>
> rcu_read_lock();
> list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
> - if (ws->wakeirq)
> - dev_pm_arm_wake_irq(ws->wakeirq);
> + dev_pm_arm_wake_irq(ws->wakeirq);
> }
> rcu_read_unlock();
> }
> @@ -358,8 +357,7 @@ void device_wakeup_disarm_wake_irqs(void)
>
> rcu_read_lock();
> list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
> - if (ws->wakeirq)
> - dev_pm_disarm_wake_irq(ws->wakeirq);
> + dev_pm_disarm_wake_irq(ws->wakeirq);
> }
> rcu_read_unlock();
> }
> @@ -396,9 +394,7 @@ int device_wakeup_disable(struct device *dev)
> return -EINVAL;
>
> ws = device_wakeup_detach(dev);
> - if (ws)
> - wakeup_source_unregister(ws);
> -
> + wakeup_source_unregister(ws);
> return 0;
> }
> EXPORT_SYMBOL_GPL(device_wakeup_disable);
>
How do you think about to integrate this update suggestion
into another source code repository?
Regards,
Markus
Re: PM-wakeup: Delete unnecessary checks before two function calls
>> How do you think about to integrate this update suggestion >> into another source code repository? > > I'm not really sure what you mean. Do you find the suggested source code change acceptable? http://article.gmane.org/gmane.linux.power-management.general/61766 https://lkml.org/lkml/2015/6/28/21 Who would dare to commit it? Regards, Markus
[PATCH] IB/hfi1: Delete an unnecessary check before the function call "sc_return_credits"
From: Markus Elfring Date: Sat, 23 Jul 2016 08:30:52 +0200 The sc_return_credits() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/infiniband/hw/hfi1/file_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c index c702a00..32c19fa 100644 --- a/drivers/infiniband/hw/hfi1/file_ops.c +++ b/drivers/infiniband/hw/hfi1/file_ops.c @@ -228,7 +228,7 @@ static long hfi1_file_ioctl(struct file *fp, unsigned int cmd, sizeof(struct hfi1_base_info)); break; case HFI1_IOCTL_CREDIT_UPD: - if (uctxt && uctxt->sc) + if (uctxt) sc_return_credits(uctxt->sc); break; -- 2.9.2
[PATCH 0/3] IB/mthca: Fine-tuning for mthca_reset()
From: Markus Elfring Date: Sat, 23 Jul 2016 12:50:12 +0200 Further update suggestions were taken into account after a patch was applied from static source code analysis. Markus Elfring (3): Delete an unnecessary check before the function call "pci_dev_put" Less function calls after error detection Delete unnecessary variable initialisations drivers/infiniband/hw/mthca/mthca_reset.c | 48 +++ 1 file changed, 23 insertions(+), 25 deletions(-) -- 2.9.2
[PATCH 1/3] IB/mthca: Delete an unnecessary check before the function call "pci_dev_put"
From: Markus Elfring Date: Sat, 23 Jul 2016 10:05:12 +0200 The pci_dev_put() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/infiniband/hw/mthca/mthca_reset.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/infiniband/hw/mthca/mthca_reset.c b/drivers/infiniband/hw/mthca/mthca_reset.c index 74c6a94..c521654 100644 --- a/drivers/infiniband/hw/mthca/mthca_reset.c +++ b/drivers/infiniband/hw/mthca/mthca_reset.c @@ -279,8 +279,7 @@ good: } out: - if (bridge) - pci_dev_put(bridge); + pci_dev_put(bridge); kfree(bridge_header); kfree(hca_header); -- 2.9.2
[PATCH 2/3] IB/mthca: Less function calls in mthca_reset() after error detection
From: Markus Elfring
Date: Sat, 23 Jul 2016 11:28:30 +0200
The kfree() function was called in a few cases by the mthca_reset()
function during error handling even if the passed variables "bridge_header"
and "hca_header" contained a null pointer.
Adjust jump targets according to the Linux coding style convention.
Signed-off-by: Markus Elfring
---
drivers/infiniband/hw/mthca/mthca_reset.c | 41 +++
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/drivers/infiniband/hw/mthca/mthca_reset.c
b/drivers/infiniband/hw/mthca/mthca_reset.c
index c521654..6727af2 100644
--- a/drivers/infiniband/hw/mthca/mthca_reset.c
+++ b/drivers/infiniband/hw/mthca/mthca_reset.c
@@ -98,7 +98,7 @@ int mthca_reset(struct mthca_dev *mdev)
err = -ENOMEM;
mthca_err(mdev, "Couldn't allocate memory to save HCA "
"PCI header, aborting.\n");
- goto out;
+ goto put_dev;
}
for (i = 0; i < 64; ++i) {
@@ -108,7 +108,7 @@ int mthca_reset(struct mthca_dev *mdev)
err = -ENODEV;
mthca_err(mdev, "Couldn't save HCA "
"PCI header, aborting.\n");
- goto out;
+ goto free_hca;
}
}
@@ -121,7 +121,7 @@ int mthca_reset(struct mthca_dev *mdev)
err = -ENOMEM;
mthca_err(mdev, "Couldn't allocate memory to save HCA "
"bridge PCI header, aborting.\n");
- goto out;
+ goto free_hca;
}
for (i = 0; i < 64; ++i) {
@@ -131,7 +131,7 @@ int mthca_reset(struct mthca_dev *mdev)
err = -ENODEV;
mthca_err(mdev, "Couldn't save HCA bridge "
"PCI header, aborting.\n");
- goto out;
+ goto free_bh;
}
}
bridge_pcix_cap = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
@@ -139,7 +139,7 @@ int mthca_reset(struct mthca_dev *mdev)
err = -ENODEV;
mthca_err(mdev, "Couldn't locate HCA bridge "
"PCI-X capability, aborting.\n");
- goto out;
+ goto free_bh;
}
}
@@ -152,7 +152,7 @@ int mthca_reset(struct mthca_dev *mdev)
err = -ENOMEM;
mthca_err(mdev, "Couldn't map HCA reset register, "
"aborting.\n");
- goto out;
+ goto free_bh;
}
writel(MTHCA_RESET_VALUE, reset);
@@ -172,7 +172,7 @@ int mthca_reset(struct mthca_dev *mdev)
err = -ENODEV;
mthca_err(mdev, "Couldn't access HCA after
reset, "
"aborting.\n");
- goto out;
+ goto free_bh;
}
if (v != 0x)
@@ -184,7 +184,7 @@ int mthca_reset(struct mthca_dev *mdev)
err = -ENODEV;
mthca_err(mdev, "PCI device did not come back after reset, "
"aborting.\n");
- goto out;
+ goto free_bh;
}
good:
@@ -195,14 +195,14 @@ good:
err = -ENODEV;
mthca_err(mdev, "Couldn't restore HCA bridge Upstream "
"split transaction control, aborting.\n");
- goto out;
+ goto free_bh;
}
if (pci_write_config_dword(bridge, bridge_pcix_cap + 0xc,
bridge_header[(bridge_pcix_cap + 0xc) / 4])) {
err = -ENODEV;
mthca_err(mdev, "Couldn't restore HCA bridge Downstream
"
"split transaction control, aborting.\n");
- goto out;
+ goto free_bh;
}
/*
* Bridge control register is at 0x3e, so we'll
@@ -216,7 +216,7 @@ good:
err = -ENODEV;
mthca_err(mdev, "Couldn't restore HCA bridge
reg %x, "
"aborting.\n", i);
- goto out;
+ goto free_bh;
}
}
@@ -225,7 +225,7 @@ good:
err = -ENODEV;
mthca_err(mdev, "Couldn't restore HCA bridge COMMAND, "
[PATCH 3/3] IB/mthca: Delete unnecessary variable initialisations in mthca_reset()
From: Markus Elfring
Date: Sat, 23 Jul 2016 11:54:06 +0200
Three local variables will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
drivers/infiniband/hw/mthca/mthca_reset.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/mthca/mthca_reset.c
b/drivers/infiniband/hw/mthca/mthca_reset.c
index 6727af2..0a458c6 100644
--- a/drivers/infiniband/hw/mthca/mthca_reset.c
+++ b/drivers/infiniband/hw/mthca/mthca_reset.c
@@ -42,12 +42,12 @@ int mthca_reset(struct mthca_dev *mdev)
{
int i;
int err = 0;
- u32 *hca_header= NULL;
+ u32 *hca_header;
u32 *bridge_header = NULL;
struct pci_dev *bridge = NULL;
int bridge_pcix_cap = 0;
- int hca_pcie_cap = 0;
- int hca_pcix_cap = 0;
+ int hca_pcie_cap;
+ int hca_pcix_cap;
u16 devctl;
u16 linkctl;
--
2.9.2
[PATCH] power: generic-adc-battery: Delete unnecessary checks before the function call "iio_channel_release"
From: Markus Elfring
Date: Sat, 23 Jul 2016 14:50:47 +0200
The iio_channel_release() function tests whether its argument is NULL
and then returns immediately. Thus the test around the calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/power/generic-adc-battery.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/power/generic-adc-battery.c
b/drivers/power/generic-adc-battery.c
index edb36bf..8f7f24c 100644
--- a/drivers/power/generic-adc-battery.c
+++ b/drivers/power/generic-adc-battery.c
@@ -350,10 +350,8 @@ err_gpio:
gpio_req_fail:
power_supply_unregister(adc_bat->psy);
err_reg_fail:
- for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
- if (adc_bat->channel[chan])
- iio_channel_release(adc_bat->channel[chan]);
- }
+ for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++)
+ iio_channel_release(adc_bat->channel[chan]);
second_mem_fail:
kfree(psy_desc->properties);
first_mem_fail:
@@ -373,11 +371,8 @@ static int gab_remove(struct platform_device *pdev)
gpio_free(pdata->gpio_charge_finished);
}
- for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
- if (adc_bat->channel[chan])
- iio_channel_release(adc_bat->channel[chan]);
- }
-
+ for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++)
+ iio_channel_release(adc_bat->channel[chan]);
kfree(adc_bat->psy_desc.properties);
cancel_delayed_work(&adc_bat->bat_work);
return 0;
--
2.9.2
[PATCH] PM-wakeup: Delete unnecessary checks before three function calls
From: Markus Elfring
Date: Sat, 23 Jul 2016 17:04:00 +0200
The following functions test whether their argument is NULL
and then return immediately.
* dev_pm_arm_wake_irq
* dev_pm_disarm_wake_irq
* wakeup_source_unregister
Thus the test around the calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/base/power/wakeup.c | 16 +---
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 5fb7718..c35d3f5 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -334,10 +334,8 @@ void device_wakeup_arm_wake_irqs(void)
struct wakeup_source *ws;
rcu_read_lock();
- list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
- if (ws->wakeirq)
- dev_pm_arm_wake_irq(ws->wakeirq);
- }
+ list_for_each_entry_rcu(ws, &wakeup_sources, entry)
+ dev_pm_arm_wake_irq(ws->wakeirq);
rcu_read_unlock();
}
@@ -351,10 +349,8 @@ void device_wakeup_disarm_wake_irqs(void)
struct wakeup_source *ws;
rcu_read_lock();
- list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
- if (ws->wakeirq)
- dev_pm_disarm_wake_irq(ws->wakeirq);
- }
+ list_for_each_entry_rcu(ws, &wakeup_sources, entry)
+ dev_pm_disarm_wake_irq(ws->wakeirq);
rcu_read_unlock();
}
@@ -390,9 +386,7 @@ int device_wakeup_disable(struct device *dev)
return -EINVAL;
ws = device_wakeup_detach(dev);
- if (ws)
- wakeup_source_unregister(ws);
-
+ wakeup_source_unregister(ws);
return 0;
}
EXPORT_SYMBOL_GPL(device_wakeup_disable);
--
2.9.2
[PATCH] memory: omap-gpmc: Delete an unnecessary check before the function call "gpiochip_free_own_desc"
From: Markus Elfring Date: Sat, 23 Jul 2016 18:54:02 +0200 The gpiochip_free_own_desc() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/memory/omap-gpmc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c index 869c83f..e138875 100644 --- a/drivers/memory/omap-gpmc.c +++ b/drivers/memory/omap-gpmc.c @@ -2143,9 +2143,7 @@ err_child_fail: ret = -ENODEV; err_cs: - if (waitpin_desc) - gpiochip_free_own_desc(waitpin_desc); - + gpiochip_free_own_desc(waitpin_desc); err: gpmc_cs_free(cs); -- 2.9.2
[PATCH] omapfb: panel-dsi-cm: Delete an unnecessary check before backlight_device_unregister()
From: Markus Elfring Date: Sat, 23 Jul 2016 19:29:28 +0200 The backlight_device_unregister() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c index b58012b..9d308db 100644 --- a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c +++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c @@ -1283,8 +1283,7 @@ static int dsicm_probe(struct platform_device *pdev) return 0; err_sysfs_create: - if (bldev != NULL) - backlight_device_unregister(bldev); + backlight_device_unregister(bldev); err_bl: destroy_workqueue(ddata->workqueue); err_reg: -- 2.9.2
[PATCH] coresight: tmc: Delete an unnecessary check before the function call "kfree"
From: Markus Elfring Date: Sat, 23 Jul 2016 20:04:09 +0200 The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- drivers/hwtracing/coresight/coresight-tmc-etf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index e68289b..5fa49c4 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -168,7 +168,7 @@ out: spin_unlock_irqrestore(&drvdata->spinlock, flags); /* Free memory outside the spinlock if need be */ - if (!used && buf) + if (!used) kfree(buf); if (!ret) -- 2.9.2
[PATCH 0/5] GPU-DRM: Fine-tuning for four function implementations
From: Markus Elfring Date: Mon, 19 Sep 2016 17:47:37 +0200 A few update suggestions were taken into account from static source code analysis. Markus Elfring (5): Use kmalloc_array() in drm_legacy_addbufs_pci() Replace two kzalloc() calls by kcalloc() in drm_legacy_addbufs_pci() Replace a kzalloc() call by kcalloc() in drm_legacy_addbufs_agp() Replace a kzalloc() call by kcalloc() in drm_legacy_addbufs_sg() Rename a jump label in drm_legacy_mapbufs() drivers/gpu/drm/drm_bufs.c | 28 ++-- 1 file changed, 14 insertions(+), 14 deletions(-) -- 2.10.0
[PATCH 1/5] GPU-DRM: Use kmalloc_array() in drm_legacy_addbufs_pci()
From: Markus Elfring
Date: Mon, 19 Sep 2016 17:07:06 +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
---
drivers/gpu/drm/drm_bufs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 3219151..ed33f43 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -923,8 +923,9 @@ int drm_legacy_addbufs_pci(struct drm_device *dev,
/* Keep the original pagelist until we know all the allocations
* have succeeded
*/
- temp_pagelist = kmalloc((dma->page_count + (count << page_order)) *
- sizeof(*dma->pagelist), GFP_KERNEL);
+ temp_pagelist = kmalloc_array(dma->page_count + (count << page_order),
+ sizeof(*dma->pagelist),
+ GFP_KERNEL);
if (!temp_pagelist) {
kfree(entry->buflist);
kfree(entry->seglist);
--
2.10.0
[PATCH 2/5] GPU-DRM: Replace two kzalloc() calls by kcalloc() in drm_legacy_addbufs_pci()
From: Markus Elfring
Date: Mon, 19 Sep 2016 17:17:34 +0200
The script "checkpatch.pl" can point information out like the following.
WARNING: Prefer kcalloc over kzalloc with multiply
Thus fix the affected source code places.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/drm_bufs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index ed33f43..8a31dac 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -905,14 +905,14 @@ int drm_legacy_addbufs_pci(struct drm_device *dev,
return -EINVAL;
}
- entry->buflist = kzalloc(count * sizeof(*entry->buflist), GFP_KERNEL);
+ entry->buflist = kcalloc(count, sizeof(*entry->buflist), GFP_KERNEL);
if (!entry->buflist) {
mutex_unlock(&dev->struct_mutex);
atomic_dec(&dev->buf_alloc);
return -ENOMEM;
}
- entry->seglist = kzalloc(count * sizeof(*entry->seglist), GFP_KERNEL);
+ entry->seglist = kcalloc(count, sizeof(*entry->seglist), GFP_KERNEL);
if (!entry->seglist) {
kfree(entry->buflist);
mutex_unlock(&dev->struct_mutex);
--
2.10.0
[PATCH 3/5] GPU-DRM: Replace a kzalloc() call by kcalloc() in drm_legacy_addbufs_agp()
From: Markus Elfring
Date: Mon, 19 Sep 2016 17:24:20 +0200
The script "checkpatch.pl" can point information out like the following.
WARNING: Prefer kcalloc over kzalloc with multiply
Thus fix the affected source code place.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/drm_bufs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 8a31dac..36dd685 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -755,7 +755,7 @@ int drm_legacy_addbufs_agp(struct drm_device *dev,
return -EINVAL;
}
- entry->buflist = kzalloc(count * sizeof(*entry->buflist), GFP_KERNEL);
+ entry->buflist = kcalloc(count, sizeof(*entry->buflist), GFP_KERNEL);
if (!entry->buflist) {
mutex_unlock(&dev->struct_mutex);
atomic_dec(&dev->buf_alloc);
--
2.10.0
[PATCH 5/5] GPU-DRM: Rename a jump label in drm_legacy_mapbufs()
From: Markus Elfring
Date: Mon, 19 Sep 2016 17:37:27 +0200
Adjust jump labels according to the current Linux coding style convention.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/drm_bufs.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index adb1dd7..0d5ee1e 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -1476,7 +1476,7 @@ int drm_legacy_mapbufs(struct drm_device *dev, void *data,
if (!map) {
retcode = -EINVAL;
- goto done;
+ goto status_indication;
}
virtual = vm_mmap(file_priv->filp, 0, map->size,
PROT_READ | PROT_WRITE,
@@ -1490,7 +1490,7 @@ int drm_legacy_mapbufs(struct drm_device *dev, void *data,
if (virtual > -1024UL) {
/* Real error */
retcode = (signed long)virtual;
- goto done;
+ goto status_indication;
}
request->virtual = (void __user *)virtual;
@@ -1499,28 +1499,28 @@ int drm_legacy_mapbufs(struct drm_device *dev, void
*data,
&dma->buflist[i]->idx,
sizeof(request->list[0].idx))) {
retcode = -EFAULT;
- goto done;
+ goto status_indication;
}
if (copy_to_user(&request->list[i].total,
&dma->buflist[i]->total,
sizeof(request->list[0].total))) {
retcode = -EFAULT;
- goto done;
+ goto status_indication;
}
if (copy_to_user(&request->list[i].used,
&zero, sizeof(zero))) {
retcode = -EFAULT;
- goto done;
+ goto status_indication;
}
address = virtual + dma->buflist[i]->offset;/* ***
*/
if (copy_to_user(&request->list[i].address,
&address, sizeof(address))) {
retcode = -EFAULT;
- goto done;
+ goto status_indication;
}
}
}
- done:
+ status_indication:
request->count = dma->buf_count;
DRM_DEBUG("%d buffers, retcode = %d\n", request->count, retcode);
--
2.10.0
[PATCH 4/5] GPU-DRM: Replace a kzalloc() call by kcalloc() in drm_legacy_addbufs_sg()
From: Markus Elfring
Date: Mon, 19 Sep 2016 17:30:31 +0200
The script "checkpatch.pl" can point information out like the following.
WARNING: Prefer kcalloc over kzalloc with multiply
Thus fix the affected source code place.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/drm_bufs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 36dd685..adb1dd7 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -1117,8 +1117,7 @@ static int drm_legacy_addbufs_sg(struct drm_device *dev,
return -EINVAL;
}
- entry->buflist = kzalloc(count * sizeof(*entry->buflist),
- GFP_KERNEL);
+ entry->buflist = kcalloc(count, sizeof(*entry->buflist), GFP_KERNEL);
if (!entry->buflist) {
mutex_unlock(&dev->struct_mutex);
atomic_dec(&dev->buf_alloc);
--
2.10.0
Re: samples: move auxdisplay example code from Documentation
> Documentation/auxdisplay/cfag12864b needs to be updated to reflect the new > home for the program (which probably does belong in samples/). > > I wonder if these programs need MAINTAINERS entries too? Do you find any more update suggestions interesting around this software module? https://patchwork.kernel.org/project/LKML/list/?q=cfag12864b Regards, Markus
[PATCH 0/6] GPU-DRM-GMA500: Fine-tuning for two function implementations
From: Markus Elfring 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(-) -- 2.10.0
[PATCH 1/6] GPU-DRM-GMA500: Use kmalloc_array() in mid_get_vbt_data_r10()
From: Markus Elfring 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 --- 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; -- 2.10.0
[PATCH 2/6] GPU-DRM-GMA500: Rename a jump label in mid_get_vbt_data_r10()
From: Markus Elfring 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 --- 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; } -- 2.10.0
[PATCH 3/6] GPU-DRM-GMA500: Move a variable assignment in mid_get_vbt_data_r10()
From: Markus Elfring
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
---
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);
--
2.10.0
[PATCH 4/6] GPU-DRM-GMA500: Fix indentation for a function call parameter in mid_get_vbt_data_r10()
From: Markus Elfring
Date: Tue, 20 Sep 2016 10:34:28 +0200
Adjust the indentation for a single function call parameter here.
Signed-off-by: Markus Elfring
---
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;
--
2.10.0
[PATCH 5/6] GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()
From: Markus Elfring 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 --- 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: -- 2.10.0
[PATCH 6/6] GPU-DRM-GMA500: Rename a jump label in mid_get_vbt_data()
From: Markus Elfring 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 --- 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; -- 2.10.0
Re: GPU-DRM-GMA500: Use kmalloc_array() in mid_get_vbt_data_r10()
>> 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
Re: GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()
>> 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
Re: GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()
>>> 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
Re: GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()
> 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
[PATCH 0/5] GPU-DRM-nouveau: Fine-tuning for five function implementations
From: Markus Elfring Date: Wed, 21 Sep 2016 09:09:09 +0200 A few update suggestions were taken into account from static source code analysis. Markus Elfring (5): Use kmalloc_array() in nvbios_iccsense_parse() Use kmalloc_array() in gt215_link_train() Delete unnecessary braces Adjust a kzalloc() call in gt215_ram_new() Add space after an "if" drivers/gpu/drm/nouveau/nvkm/subdev/bios/iccsense.c | 4 +++- drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c | 21 + 2 files changed, 12 insertions(+), 13 deletions(-) -- 2.10.0
[PATCH 2/5] GPU-DRM-nouveau: Use kmalloc_array() in gt215_link_train()
From: Markus Elfring Date: Tue, 20 Sep 2016 22:32:14 +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. * Replace the specification of a data type by a pointer dereference to make the corresponding size determination a bit safer according to the Linux coding style convention. Signed-off-by: Markus Elfring --- drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c index d15ea88..dbaf577 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c @@ -170,7 +170,7 @@ gt215_link_train(struct gt215_ram *ram) return -ENOSYS; /* XXX: Multiple partitions? */ - result = kmalloc(64 * sizeof(u32), GFP_KERNEL); + result = kmalloc_array(64, sizeof(*result), GFP_KERNEL); if (!result) return -ENOMEM; -- 2.10.0
[PATCH 3/5] GPU-DRM-nouveau: Delete unnecessary braces
From: Markus Elfring
Date: Wed, 21 Sep 2016 08:28:08 +0200
Do not use curly brackets at four source code places
where a single statement should be sufficient.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c
index dbaf577..cb50539 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c
@@ -127,12 +127,11 @@ gt215_link_train_calc(u32 *vals, struct gt215_ltrain
*train)
}
/* Find the best value for 0xe0 */
- for (i = 0; i < 4; i++) {
+ for (i = 0; i < 4; i++)
if (bins[i] > qty) {
bin = i + 3;
qty = bins[i];
}
- }
train->r_100720 = 0;
for (i = 0; i < 8; i++) {
@@ -729,9 +728,8 @@ gt215_ram_calc(struct nvkm_ram *base, u32 freq)
ram_mask(fuc, 0x1007e0, 0x, r100760);
}
- if (device->chipset == 0xa3 && freq > 50) {
+ if (device->chipset == 0xa3 && freq > 50)
ram_mask(fuc, 0x100700, 0x0006, 0x);
- }
/* Final switch */
if (mclk.pll) {
@@ -745,12 +743,11 @@ gt215_ram_calc(struct nvkm_ram *base, u32 freq)
ram_nsec(fuc, 2000);
/* Set RAM MR parameters and timings */
- for (i = 2; i >= 0; i--) {
+ for (i = 2; i >= 0; i--)
if (ram_rd32(fuc, mr[i]) != ram->base.mr[i]) {
ram_wr32(fuc, mr[i], ram->base.mr[i]);
ram_nsec(fuc, 1000);
}
- }
ram_wr32(fuc, 0x100220[3], timing[3]);
ram_wr32(fuc, 0x100220[1], timing[1]);
@@ -838,11 +835,10 @@ gt215_ram_calc(struct nvkm_ram *base, u32 freq)
if (!next->bios.ramcfg_DLLoff)
nvkm_sddr2_dll_reset(fuc);
- if (ram->base.type == NVKM_RAM_TYPE_GDDR3) {
+ if (ram->base.type == NVKM_RAM_TYPE_GDDR3)
ram_nsec(fuc, 31000);
- } else {
+ else
ram_nsec(fuc, 14000);
- }
if (ram->base.type == NVKM_RAM_TYPE_DDR3) {
ram_wr32(fuc, 0x100264, 0x1);
--
2.10.0
[PATCH 1/5] GPU-DRM-nouveau: Use kmalloc_array() in nvbios_iccsense_parse()
From: Markus Elfring Date: Tue, 20 Sep 2016 22:22:14 +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. * Replace the specification of a data structure by a pointer dereference to make the corresponding size determination a bit safer according to the Linux coding style convention. Signed-off-by: Markus Elfring --- drivers/gpu/drm/nouveau/nvkm/subdev/bios/iccsense.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/iccsense.c b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/iccsense.c index 0843280..91cf1ee 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/iccsense.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/iccsense.c @@ -72,7 +72,9 @@ nvbios_iccsense_parse(struct nvkm_bios *bios, struct nvbios_iccsense *iccsense) } iccsense->nr_entry = cnt; - iccsense->rail = kmalloc(sizeof(struct pwr_rail_t) * cnt, GFP_KERNEL); + iccsense->rail = kmalloc_array(cnt, + sizeof(*iccsense->rail), + GFP_KERNEL); if (!iccsense->rail) return -ENOMEM; -- 2.10.0
[PATCH 4/5] GPU-DRM-nouveau: Adjust a kzalloc() call in gt215_ram_new()
From: Markus Elfring Date: Wed, 21 Sep 2016 08:44:38 +0200 The script "checkpatch.pl" can point out that assignments should usually not be performed within condition checks. Thus move the assignment for one local variable to a separate statement in this function. Signed-off-by: Markus Elfring --- drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c index cb50539..edbe8e4 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c @@ -940,7 +940,8 @@ gt215_ram_new(struct nvkm_fb *fb, struct nvkm_ram **pram) struct gt215_ram *ram; int ret, i; - if (!(ram = kzalloc(sizeof(*ram), GFP_KERNEL))) + ram = kzalloc(sizeof(*ram), GFP_KERNEL); + if (!ram) return -ENOMEM; *pram = &ram->base; -- 2.10.0
[PATCH 5/5] GPU-DRM-nouveau: Add space after an "if"
From: Markus Elfring Date: Wed, 21 Sep 2016 08:58:41 +0200 Use another space character behind the keyword "if" according to the Linux coding style convention. Signed-off-by: Markus Elfring --- drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c index edbe8e4..f95800d 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgt215.c @@ -256,7 +256,7 @@ gt215_link_train(struct gt215_ram *ram) return ret; out: - if(ret == -EBUSY) + if (ret == -EBUSY) f = NULL; train->state = NVA3_TRAIN_UNSUPPORTED; -- 2.10.0
Re: virtio_console: Less function calls in init_vqs() after error detection
>> The kfree() function was called in up to five cases >> by the init_vqs() function during error handling even if >> the passed variable contained a null pointer. >> >> * Return directly after a call of the function "kmalloc_array" failed >> at the beginning. >> >> * Split a condition check for memory allocation failures so that >> each pointer from these function calls will be checked immediately. >> >> See also background information: >> Topic "CWE-754: Improper check for unusual or exceptional conditions" >> Link: https://cwe.mitre.org/data/definitions/754.html >> >> * Adjust jump targets according to the Linux coding style convention. > > So I've seen this series and I'm not yet sure how I feel about the > patches - f.e. in this one, it adds more lines than it removes to > achieve the same effect. I find this consequence still debatable. > I think the code is currently more readable than after these changes. Thanks for your constructive feedback. Can it be that an other software development concern is eventually overlooked? > And even if kfree is called multiple times, it isn't a huge bother I know also that the implementation of this function tolerates the passing of null pointers. > -- it's error case anyway, very unlikely to trigger, but keeps everything > very readble. I suggest to reconsider this design detail if it is really acceptable for the safe implementation of such a software module. * How much will it matter in general that four function call were performed in this use case without checking their return values immediately? * Should it usually be determined quicker if a required resource like memory could be acquired before trying the next allocation? Regards, Markus
Re: GPU-DRM-nouveau: Delete unnecessary braces
> The original style was correct, the new style is wrong. I find your feedback interesting for further clarifications. > Multi-line indents get curly braces for readability. How do you think about to transform such an information into an official specification for the the document "CodingStyle"? Regards, Markus
[PATCH 00/14] GPU-DRM-OMAP: Fine-tuning for several function implementations
From: Markus Elfring Date: Wed, 21 Sep 2016 18:28:38 +0200 Several update suggestions were taken into account from static source code analysis. Markus Elfring (14): Use kmalloc_array() in tiler_map_show() Replace another kmalloc() call by kmalloc_array() in tiler_map_show() Less function calls in tiler_map_show() after error detection Delete an unnecessary variable initialisation in tiler_map_show() Improve a size determination in dmm_txn_append() Improve a size determination in omap_dmm_probe() Rename a jump label in omap_dmm_probe() Rename a jump label in dmm_txn_commit() Delete an unnecessary variable initialisation in dmm_txn_commit() Use kmalloc_array() in omap_gem_attach_pages() Replace a kzalloc() call by kcalloc() in omap_gem_attach_pages() Move a variable assignment in omap_gem_attach_pages() Rename a jump label in omap_gem_new_dmabuf() Rename a jump label in four functions drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 58 drivers/gpu/drm/omapdrm/omap_gem.c | 44 +++- 2 files changed, 49 insertions(+), 53 deletions(-) -- 2.10.0
[PATCH 01/14] GPU-DRM-OMAP: Use kmalloc_array() in tiler_map_show()
From: Markus Elfring Date: Wed, 21 Sep 2016 12:23:46 +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 --- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index 4ceed7a9..7b32dd3 100644 --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c @@ -917,8 +917,7 @@ int tiler_map_show(struct seq_file *s, void *arg) h_adj = omap_dmm->container_height / ydiv; w_adj = omap_dmm->container_width / xdiv; - - map = kmalloc(h_adj * sizeof(*map), GFP_KERNEL); + map = kmalloc_array(h_adj, sizeof(*map), GFP_KERNEL); global_map = kmalloc((w_adj + 1) * h_adj, GFP_KERNEL); if (!map || !global_map) -- 2.10.0
[PATCH 02/14] GPU-DRM-OMAP: Replace another kmalloc() call by kmalloc_array() in tiler_map_show()
From: Markus Elfring Date: Wed, 21 Sep 2016 12: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" at another place. Signed-off-by: Markus Elfring --- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index 7b32dd3..3a4f91b 100644 --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c @@ -918,8 +918,7 @@ int tiler_map_show(struct seq_file *s, void *arg) h_adj = omap_dmm->container_height / ydiv; w_adj = omap_dmm->container_width / xdiv; map = kmalloc_array(h_adj, sizeof(*map), GFP_KERNEL); - global_map = kmalloc((w_adj + 1) * h_adj, GFP_KERNEL); - + global_map = kmalloc_array(h_adj, w_adj + 1, GFP_KERNEL); if (!map || !global_map) goto error; -- 2.10.0
[PATCH 03/14] GPU-DRM-OMAP: Less function calls in tiler_map_show() after error detection
From: Markus Elfring
Date: Wed, 21 Sep 2016 13:16:20 +0200
The kfree() function was called in up to two cases
by the tiler_map_show() function during error handling even if
the passed variable contained a null pointer.
* Adjust jump targets according to the Linux coding style convention.
* Split a condition check for memory allocation failures so that
each pointer from these function calls will be checked immediately.
See also background information:
Topic "CWE-754: Improper check for unusual or exceptional conditions"
Link: https://cwe.mitre.org/data/definitions/754.html
* Return directly after a call of the function "kmalloc_array" failed
at the beginning.
* Move an assignment for the local variable "w_adj" behind the first
memory allocation.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 3a4f91b..60beeb9 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -916,11 +916,14 @@ int tiler_map_show(struct seq_file *s, void *arg)
}
h_adj = omap_dmm->container_height / ydiv;
- w_adj = omap_dmm->container_width / xdiv;
map = kmalloc_array(h_adj, sizeof(*map), GFP_KERNEL);
+ if (!map)
+ return 0;
+
+ w_adj = omap_dmm->container_width / xdiv;
global_map = kmalloc_array(h_adj, w_adj + 1, GFP_KERNEL);
- if (!map || !global_map)
- goto error;
+ if (!global_map)
+ goto free_map;
for (lut_idx = 0; lut_idx < omap_dmm->num_lut; lut_idx++) {
memset(map, 0, h_adj * sizeof(*map));
@@ -982,10 +985,9 @@ int tiler_map_show(struct seq_file *s, void *arg)
}
}
-error:
- kfree(map);
kfree(global_map);
-
+ free_map:
+ kfree(map);
return 0;
}
#endif
--
2.10.0
[PATCH 04/14] GPU-DRM-OMAP: Delete an unnecessary variable initialisation in tiler_map_show()
From: Markus Elfring
Date: Wed, 21 Sep 2016 13:31:45 +0200
The local variable "map" will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 60beeb9..c262ef5 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -896,7 +896,7 @@ static void map_2d_info(char **map, int xdiv, int ydiv,
char *nice,
int tiler_map_show(struct seq_file *s, void *arg)
{
int xdiv = 2, ydiv = 1;
- char **map = NULL, *global_map;
+ char **map, *global_map;
struct tiler_block *block;
struct tcm_area a, p;
int i;
--
2.10.0
[PATCH 05/14] GPU-DRM-OMAP: Improve a size determination in dmm_txn_append()
From: Markus Elfring Date: Wed, 21 Sep 2016 13:53:11 +0200 Replace the specification of a data structure by a pointer dereference as the parameter for the operator "sizeof" to make the corresponding size determination a bit safer according to the Linux coding style convention. Signed-off-by: Markus Elfring --- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index c262ef5..f110965 100644 --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c @@ -224,7 +224,7 @@ static void dmm_txn_append(struct dmm_txn *txn, struct pat_area *area, int rows = (1 + area->y1 - area->y0); int i = columns*rows; - pat = alloc_dma(txn, sizeof(struct pat), &pat_pa); + pat = alloc_dma(txn, sizeof(*pat), &pat_pa); if (txn->last_pat) txn->last_pat->next_pa = (uint32_t)pat_pa; -- 2.10.0
[PATCH 06/14] GPU-DRM-OMAP: Improve a size determination in omap_dmm_probe()
From: Markus Elfring
Date: Wed, 21 Sep 2016 17:21:57 +0200
Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index f110965..c6a7197 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -735,7 +735,8 @@ static int omap_dmm_probe(struct platform_device *dev)
/* alloc engines */
omap_dmm->engines = kcalloc(omap_dmm->num_engines,
- sizeof(struct refill_engine), GFP_KERNEL);
+ sizeof(*omap_dmm->engines),
+ GFP_KERNEL);
if (!omap_dmm->engines) {
ret = -ENOMEM;
goto fail;
--
2.10.0
[PATCH 07/14] GPU-DRM-OMAP: Rename a jump label in omap_dmm_probe()
From: Markus Elfring
Date: Wed, 21 Sep 2016 17:30:25 +0200
Adjust jump labels according to the current Linux coding style convention.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index c6a7197..5f6f21b 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -624,7 +624,7 @@ static int omap_dmm_probe(struct platform_device *dev)
omap_dmm = kzalloc(sizeof(*omap_dmm), GFP_KERNEL);
if (!omap_dmm)
- goto fail;
+ goto check_dmm_removal;
/* initialize lists */
INIT_LIST_HEAD(&omap_dmm->alloc_head);
@@ -648,20 +648,20 @@ static int omap_dmm_probe(struct platform_device *dev)
mem = platform_get_resource(dev, IORESOURCE_MEM, 0);
if (!mem) {
dev_err(&dev->dev, "failed to get base address resource\n");
- goto fail;
+ goto check_dmm_removal;
}
omap_dmm->base = ioremap(mem->start, SZ_2K);
if (!omap_dmm->base) {
dev_err(&dev->dev, "failed to get dmm base address\n");
- goto fail;
+ goto check_dmm_removal;
}
omap_dmm->irq = platform_get_irq(dev, 0);
if (omap_dmm->irq < 0) {
dev_err(&dev->dev, "failed to get IRQ resource\n");
- goto fail;
+ goto check_dmm_removal;
}
omap_dmm->dev = &dev->dev;
@@ -699,7 +699,7 @@ static int omap_dmm_probe(struct platform_device *dev)
dev_err(&dev->dev, "couldn't register IRQ %d, error %d\n",
omap_dmm->irq, ret);
omap_dmm->irq = -1;
- goto fail;
+ goto check_dmm_removal;
}
/* Enable all interrupts for each refill engine except
@@ -714,13 +714,13 @@ static int omap_dmm_probe(struct platform_device *dev)
if (!omap_dmm->dummy_page) {
dev_err(&dev->dev, "could not allocate dummy page\n");
ret = -ENOMEM;
- goto fail;
+ goto check_dmm_removal;
}
/* set dma mask for device */
ret = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
if (ret)
- goto fail;
+ goto check_dmm_removal;
omap_dmm->dummy_pa = page_to_phys(omap_dmm->dummy_page);
@@ -730,7 +730,7 @@ static int omap_dmm_probe(struct platform_device *dev)
&omap_dmm->refill_pa, GFP_KERNEL);
if (!omap_dmm->refill_va) {
dev_err(&dev->dev, "could not allocate refill memory\n");
- goto fail;
+ goto check_dmm_removal;
}
/* alloc engines */
@@ -739,7 +739,7 @@ static int omap_dmm_probe(struct platform_device *dev)
GFP_KERNEL);
if (!omap_dmm->engines) {
ret = -ENOMEM;
- goto fail;
+ goto check_dmm_removal;
}
for (i = 0; i < omap_dmm->num_engines; i++) {
@@ -758,7 +758,7 @@ static int omap_dmm_probe(struct platform_device *dev)
GFP_KERNEL);
if (!omap_dmm->tcm) {
ret = -ENOMEM;
- goto fail;
+ goto check_dmm_removal;
}
/* init containers */
@@ -772,7 +772,7 @@ static int omap_dmm_probe(struct platform_device *dev)
if (!omap_dmm->tcm[i]) {
dev_err(&dev->dev, "failed to allocate container\n");
ret = -ENOMEM;
- goto fail;
+ goto check_dmm_removal;
}
omap_dmm->tcm[i]->lut_id = i;
@@ -812,8 +812,7 @@ static int omap_dmm_probe(struct platform_device *dev)
dev_info(omap_dmm->dev, "initialized all PAT entries\n");
return 0;
-
-fail:
+ check_dmm_removal:
if (omap_dmm_remove(dev))
dev_err(&dev->dev, "cleanup failed\n");
return ret;
--
2.10.0
[PATCH 09/14] GPU-DRM-OMAP: Delete an unnecessary variable initialisation in dmm_txn_commit()
From: Markus Elfring
Date: Wed, 21 Sep 2016 17:34:40 +0200
The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index c8ced158..c5c3793 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -262,7 +262,7 @@ static void dmm_txn_append(struct dmm_txn *txn, struct
pat_area *area,
*/
static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
{
- int ret = 0;
+ int ret;
struct refill_engine *engine = txn->engine_handle;
struct dmm *dmm = engine->dmm;
--
2.10.0
[PATCH 10/14] GPU-DRM-OMAP: Use kmalloc_array() in omap_gem_attach_pages()
From: Markus Elfring
Date: Wed, 21 Sep 2016 17:37:04 +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
---
drivers/gpu/drm/omapdrm/omap_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
b/drivers/gpu/drm/omapdrm/omap_gem.c
index 505dee0..e4f1924 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -259,7 +259,7 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
* DSS, GPU, etc. are not cache coherent:
*/
if (omap_obj->flags & (OMAP_BO_WC|OMAP_BO_UNCACHED)) {
- addrs = kmalloc(npages * sizeof(*addrs), GFP_KERNEL);
+ addrs = kmalloc_array(npages, sizeof(*addrs), GFP_KERNEL);
if (!addrs) {
ret = -ENOMEM;
goto free_pages;
--
2.10.0
[PATCH 12/14] GPU-DRM-OMAP: Move a variable assignment in omap_gem_attach_pages()
From: Markus Elfring Date: Wed, 21 Sep 2016 17:42:28 +0200 Move one assignment for the local variable "npages" so that its setting will only be performed after a call of the function "drm_gem_get_pages" succeeded by this function. Signed-off-by: Markus Elfring --- drivers/gpu/drm/omapdrm/omap_gem.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 26f1212..3c49ad9 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -243,7 +243,7 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj) struct drm_device *dev = obj->dev; struct omap_gem_object *omap_obj = to_omap_bo(obj); struct page **pages; - int npages = obj->size >> PAGE_SHIFT; + int npages; int i, ret; dma_addr_t *addrs; @@ -255,6 +255,8 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj) return PTR_ERR(pages); } + npages = obj->size >> PAGE_SHIFT; + /* for non-cached buffers, ensure the new pages are clean because * DSS, GPU, etc. are not cache coherent: */ -- 2.10.0
[PATCH 11/14] GPU-DRM-OMAP: Replace a kzalloc() call by kcalloc() in omap_gem_attach_pages()
From: Markus Elfring
Date: Wed, 21 Sep 2016 17:40:20 +0200
The script "checkpatch.pl" can point information out like the following.
WARNING: Prefer kcalloc over kzalloc with multiply
Thus fix the affected source code place.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/omap_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
b/drivers/gpu/drm/omapdrm/omap_gem.c
index e4f1924..26f1212 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -283,7 +283,7 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
}
}
} else {
- addrs = kzalloc(npages * sizeof(*addrs), GFP_KERNEL);
+ addrs = kcalloc(npages, sizeof(*addrs), GFP_KERNEL);
if (!addrs) {
ret = -ENOMEM;
goto free_pages;
--
2.10.0
[PATCH 13/14] GPU-DRM-OMAP: Rename a jump label in omap_gem_new_dmabuf()
From: Markus Elfring
Date: Wed, 21 Sep 2016 17:45:04 +0200
Adjust jump labels according to the current Linux coding style convention.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/omap_gem.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
b/drivers/gpu/drm/omapdrm/omap_gem.c
index 3c49ad9..92510de 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1442,7 +1442,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct
drm_device *dev, size_t size,
obj = omap_gem_new(dev, gsize, OMAP_BO_MEM_DMABUF | OMAP_BO_WC);
if (!obj) {
obj = ERR_PTR(-ENOMEM);
- goto done;
+ goto unlock;
}
omap_obj = to_omap_bo(obj);
@@ -1462,7 +1462,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct
drm_device *dev, size_t size,
if (!pages) {
omap_gem_free_object(obj);
obj = ERR_PTR(-ENOMEM);
- goto done;
+ goto unlock;
}
omap_obj->pages = pages;
@@ -1476,11 +1476,10 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct
drm_device *dev, size_t size,
if (WARN_ON(i != npages)) {
omap_gem_free_object(obj);
obj = ERR_PTR(-ENOMEM);
- goto done;
+ goto unlock;
}
}
-
-done:
+ unlock:
mutex_unlock(&dev->struct_mutex);
return obj;
}
--
2.10.0
[PATCH 14/14] GPU-DRM-OMAP: Rename a jump label in four functions
From: Markus Elfring
Date: Wed, 21 Sep 2016 18:00:23 +0200
Adjust jump labels according to the current Linux coding style convention.
Thus replace the identifier "fail" by "unlock" for this refactoring.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/omap_gem.c | 27 +++
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
b/drivers/gpu/drm/omapdrm/omap_gem.c
index 92510de..ea7ad1c 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -549,7 +549,7 @@ int omap_gem_fault(struct vm_area_struct *vma, struct
vm_fault *vmf)
/* if a shmem backed object, make sure we have pages attached now */
ret = get_pages(obj, &pages);
if (ret)
- goto fail;
+ goto unlock;
/* where should we do corresponding put_pages().. we are mapping
* the original page, rather than thru a GART, so we can't rely
@@ -561,9 +561,7 @@ int omap_gem_fault(struct vm_area_struct *vma, struct
vm_fault *vmf)
ret = fault_2d(obj, vma, vmf);
else
ret = fault_1d(obj, vma, vmf);
-
-
-fail:
+ unlock:
mutex_unlock(&dev->struct_mutex);
switch (ret) {
case 0:
@@ -682,14 +680,13 @@ int omap_gem_dumb_map_offset(struct drm_file *file,
struct drm_device *dev,
obj = drm_gem_object_lookup(file, handle);
if (obj == NULL) {
ret = -ENOENT;
- goto fail;
+ goto unlock;
}
*offset = omap_gem_mmap_offset(obj);
drm_gem_object_unreference_unlocked(obj);
-
-fail:
+ unlock:
return ret;
}
@@ -719,13 +716,12 @@ int omap_gem_roll(struct drm_gem_object *obj, uint32_t
roll)
struct page **pages;
ret = get_pages(obj, &pages);
if (ret)
- goto fail;
+ goto unlock;
ret = tiler_pin(omap_obj->block, pages, npages, roll, true);
if (ret)
dev_err(obj->dev->dev, "could not repin: %d\n", ret);
}
-
-fail:
+ unlock:
mutex_unlock(&obj->dev->struct_mutex);
return ret;
@@ -825,7 +821,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
ret = get_pages(obj, &pages);
if (ret)
- goto fail;
+ goto unlock;
if (omap_obj->flags & OMAP_BO_TILED) {
block = tiler_reserve_2d(fmt,
@@ -839,7 +835,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
ret = PTR_ERR(block);
dev_err(obj->dev->dev,
"could not remap: %d (%d)\n", ret, fmt);
- goto fail;
+ goto unlock;
}
/* TODO: enable async refill.. */
@@ -849,7 +845,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
tiler_release(block);
dev_err(obj->dev->dev,
"could not pin: %d\n", ret);
- goto fail;
+ goto unlock;
}
omap_obj->paddr = tiler_ssptr(block);
@@ -865,10 +861,9 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
*paddr = omap_obj->paddr;
} else {
ret = -EINVAL;
- goto fail;
+ goto unlock;
}
-
-fail:
+ unlock:
mutex_unlock(&obj->dev->struct_mutex);
return ret;
--
2.10.0
[PATCH 08/14] GPU-DRM-OMAP: Rename a jump label in dmm_txn_commit()
From: Markus Elfring
Date: Wed, 21 Sep 2016 17:32:42 +0200
Adjust a jump target so that redundant checks can be avoided at the end.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 5f6f21b..c8ced158 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -269,7 +269,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
if (!txn->last_pat) {
dev_err(engine->dmm->dev, "need at least one txn\n");
ret = -EINVAL;
- goto cleanup;
+ goto release_engine;
}
txn->last_pat->next_pa = 0;
@@ -281,7 +281,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
ret = wait_status(engine, DMM_PATSTATUS_READY);
if (ret) {
ret = -EFAULT;
- goto cleanup;
+ goto release_engine;
}
/* mark whether it is async to denote list management in IRQ handler */
@@ -301,9 +301,9 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
}
}
-cleanup:
/* only place engine back on list if we are done with it */
if (ret || wait)
+ release_engine:
release_engine(engine);
return ret;
--
2.10.0
[PATCH 1/4] GPU-DRM-QXL: Use kmalloc_array() in qxl_device_init()
From: Markus Elfring Date: Wed, 21 Sep 2016 22:26:08 +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. * Replace the specification of a data structure by a pointer dereference to make the corresponding size determination a bit safer according to the Linux coding style convention. * Add a bit of exception handling for a detected null pointer. Signed-off-by: Markus Elfring --- drivers/gpu/drm/qxl/qxl_kms.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index e642242..76852f1 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -216,10 +216,11 @@ static int qxl_device_init(struct qxl_device *qdev, qdev->slot_id_bits = qdev->rom->slot_id_bits; qdev->va_slot_mask = (~(uint64_t)0) >> (qdev->slot_id_bits + qdev->slot_gen_bits); - - qdev->mem_slots = - kmalloc(qdev->n_mem_slots * sizeof(struct qxl_memslot), - GFP_KERNEL); + qdev->mem_slots = kmalloc_array(qdev->n_mem_slots, + sizeof(*qdev->mem_slots), + GFP_KERNEL); + if (!qdev->mem_slots) + return -ENOMEM; idr_init(&qdev->release_idr); spin_lock_init(&qdev->release_idr_lock); -- 2.10.0
[PATCH 0/4] GPU-DRM-QXL: Fine-tuning for three function implementations
From: Markus Elfring Date: Thu, 22 Sep 2016 08:08:08 +0200 A few update suggestions were taken into account from static source code analysis. Markus Elfring (4): Use kmalloc_array() in qxl_device_init() Move three assignments in qxl_device_init() Improve a size determination in qxl_driver_load() Adjust checks for null pointers in three functions drivers/gpu/drm/qxl/qxl_kms.c | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) -- 2.10.0
[PATCH 2/4] GPU-DRM-QXL: Move three assignments in qxl_device_init()
From: Markus Elfring Date: Wed, 21 Sep 2016 22:33:54 +0200 Move the assignments for three data structure members to the end so that they will only be performed if the desired resource allocations succeeded by this function. Signed-off-by: Markus Elfring --- drivers/gpu/drm/qxl/qxl_kms.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index 76852f1..76780c2 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -212,10 +212,6 @@ static int qxl_device_init(struct qxl_device *qdev, /* TODO - slot initialization should happen on reset. where is our * reset handler? */ qdev->n_mem_slots = qdev->rom->slots_end; - qdev->slot_gen_bits = qdev->rom->slot_gen_bits; - qdev->slot_id_bits = qdev->rom->slot_id_bits; - qdev->va_slot_mask = - (~(uint64_t)0) >> (qdev->slot_id_bits + qdev->slot_gen_bits); qdev->mem_slots = kmalloc_array(qdev->n_mem_slots, sizeof(*qdev->mem_slots), GFP_KERNEL); @@ -260,7 +256,10 @@ static int qxl_device_init(struct qxl_device *qdev, INIT_WORK(&qdev->gc_work, qxl_gc_work); - + qdev->slot_gen_bits = qdev->rom->slot_gen_bits; + qdev->slot_id_bits = qdev->rom->slot_id_bits; + qdev->va_slot_mask = + (~(uint64_t)0) >> (qdev->slot_id_bits + qdev->slot_gen_bits); return 0; } -- 2.10.0
