* [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
@ 2020-06-03 13:51 ` Colin King
0 siblings, 0 replies; 10+ messages in thread
From: Colin King @ 2020-06-03 13:51 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The variable error is being initialized with a value that is
never read and the -ENOMEM error return is being returned anyhow
by the error exit path to label err_free_mem. The assignment is
redundant and can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/input/misc/ims-pcu.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index d8dbfc030d0f..4ba68aa3d281 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
if (!gamepad || !input) {
dev_err(pcu->dev,
"Not enough memory for gamepad device\n");
- error = -ENOMEM;
goto err_free_mem;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
@ 2020-06-03 13:51 ` Colin King
0 siblings, 0 replies; 10+ messages in thread
From: Colin King @ 2020-06-03 13:51 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The variable error is being initialized with a value that is
never read and the -ENOMEM error return is being returned anyhow
by the error exit path to label err_free_mem. The assignment is
redundant and can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/input/misc/ims-pcu.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index d8dbfc030d0f..4ba68aa3d281 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
if (!gamepad || !input) {
dev_err(pcu->dev,
"Not enough memory for gamepad device\n");
- error = -ENOMEM;
goto err_free_mem;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
2020-06-03 13:51 ` Colin King
@ 2020-06-03 14:09 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2020-06-03 14:09 UTC (permalink / raw)
To: Colin King; +Cc: Dmitry Torokhov, linux-input, kernel-janitors, linux-kernel
On Wed, Jun 03, 2020 at 02:51:02PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The variable error is being initialized with a value that is
> never read and the -ENOMEM error return is being returned anyhow
> by the error exit path to label err_free_mem. The assignment is
> redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/input/misc/ims-pcu.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index d8dbfc030d0f..4ba68aa3d281 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
> if (!gamepad || !input) {
> dev_err(pcu->dev,
> "Not enough memory for gamepad device\n");
> - error = -ENOMEM;
> goto err_free_mem;
It would be better to change the return instead.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
@ 2020-06-03 14:09 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2020-06-03 14:09 UTC (permalink / raw)
To: Colin King; +Cc: Dmitry Torokhov, linux-input, kernel-janitors, linux-kernel
On Wed, Jun 03, 2020 at 02:51:02PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The variable error is being initialized with a value that is
> never read and the -ENOMEM error return is being returned anyhow
> by the error exit path to label err_free_mem. The assignment is
> redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/input/misc/ims-pcu.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index d8dbfc030d0f..4ba68aa3d281 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
> if (!gamepad || !input) {
> dev_err(pcu->dev,
> "Not enough memory for gamepad device\n");
> - error = -ENOMEM;
> goto err_free_mem;
It would be better to change the return instead.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
2020-06-03 14:09 ` Dan Carpenter
@ 2020-06-03 14:18 ` Colin Ian King
-1 siblings, 0 replies; 10+ messages in thread
From: Colin Ian King @ 2020-06-03 14:18 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Dmitry Torokhov, linux-input, kernel-janitors, linux-kernel
On 03/06/2020 15:09, Dan Carpenter wrote:
> On Wed, Jun 03, 2020 at 02:51:02PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The variable error is being initialized with a value that is
>> never read and the -ENOMEM error return is being returned anyhow
>> by the error exit path to label err_free_mem. The assignment is
>> redundant and can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> drivers/input/misc/ims-pcu.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
>> index d8dbfc030d0f..4ba68aa3d281 100644
>> --- a/drivers/input/misc/ims-pcu.c
>> +++ b/drivers/input/misc/ims-pcu.c
>> @@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
>> if (!gamepad || !input) {
>> dev_err(pcu->dev,
>> "Not enough memory for gamepad device\n");
>> - error = -ENOMEM;
>> goto err_free_mem;
>
> It would be better to change the return instead.
>
> regards,
> dan carpenter
>
I'm not sure about that, the err_free_mem path is used by another error
exit return path that also needs to free the device and gamepad and
returns ENOMEM, so I think this is a good enough shared error exit strategy.
Colin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
@ 2020-06-03 14:18 ` Colin Ian King
0 siblings, 0 replies; 10+ messages in thread
From: Colin Ian King @ 2020-06-03 14:18 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Dmitry Torokhov, linux-input, kernel-janitors, linux-kernel
On 03/06/2020 15:09, Dan Carpenter wrote:
> On Wed, Jun 03, 2020 at 02:51:02PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The variable error is being initialized with a value that is
>> never read and the -ENOMEM error return is being returned anyhow
>> by the error exit path to label err_free_mem. The assignment is
>> redundant and can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> drivers/input/misc/ims-pcu.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
>> index d8dbfc030d0f..4ba68aa3d281 100644
>> --- a/drivers/input/misc/ims-pcu.c
>> +++ b/drivers/input/misc/ims-pcu.c
>> @@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
>> if (!gamepad || !input) {
>> dev_err(pcu->dev,
>> "Not enough memory for gamepad device\n");
>> - error = -ENOMEM;
>> goto err_free_mem;
>
> It would be better to change the return instead.
>
> regards,
> dan carpenter
>
I'm not sure about that, the err_free_mem path is used by another error
exit return path that also needs to free the device and gamepad and
returns ENOMEM, so I think this is a good enough shared error exit strategy.
Colin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
2020-06-03 14:18 ` Colin Ian King
@ 2020-06-03 14:45 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2020-06-03 14:45 UTC (permalink / raw)
To: Colin Ian King
Cc: Dmitry Torokhov, linux-input, kernel-janitors, linux-kernel
On Wed, Jun 03, 2020 at 03:18:46PM +0100, Colin Ian King wrote:
> On 03/06/2020 15:09, Dan Carpenter wrote:
> > On Wed, Jun 03, 2020 at 02:51:02PM +0100, Colin King wrote:
> >> From: Colin Ian King <colin.king@canonical.com>
> >>
> >> The variable error is being initialized with a value that is
> >> never read and the -ENOMEM error return is being returned anyhow
> >> by the error exit path to label err_free_mem. The assignment is
> >> redundant and can be removed.
> >>
> >> Addresses-Coverity: ("Unused value")
> >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >> ---
> >> drivers/input/misc/ims-pcu.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> >> index d8dbfc030d0f..4ba68aa3d281 100644
> >> --- a/drivers/input/misc/ims-pcu.c
> >> +++ b/drivers/input/misc/ims-pcu.c
> >> @@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
> >> if (!gamepad || !input) {
> >> dev_err(pcu->dev,
> >> "Not enough memory for gamepad device\n");
> >> - error = -ENOMEM;
> >> goto err_free_mem;
> >
> > It would be better to change the return instead.
> >
> > regards,
> > dan carpenter
> >
>
> I'm not sure about that, the err_free_mem path is used by another error
> exit return path that also needs to free the device and gamepad and
> returns ENOMEM, so I think this is a good enough shared error exit strategy.
The code looks like this:
drivers/input/misc/ims-pcu.c
284 static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
285 {
286 struct ims_pcu_gamepad *gamepad;
287 struct input_dev *input;
288 int error;
289
290 gamepad = kzalloc(sizeof(struct ims_pcu_gamepad), GFP_KERNEL);
291 input = input_allocate_device();
292 if (!gamepad || !input) {
293 dev_err(pcu->dev,
294 "Not enough memory for gamepad device\n");
295 error = -ENOMEM;
296 goto err_free_mem;
The "error" is always set before all the gotos.
297 }
298
299 gamepad->input = input;
300
301 snprintf(gamepad->name, sizeof(gamepad->name),
302 "IMS PCU#%d Gamepad Interface", pcu->device_no);
303
304 usb_make_path(pcu->udev, gamepad->phys, sizeof(gamepad->phys));
305 strlcat(gamepad->phys, "/input1", sizeof(gamepad->phys));
306
307 input->name = gamepad->name;
308 input->phys = gamepad->phys;
309 usb_to_input_id(pcu->udev, &input->id);
310 input->dev.parent = &pcu->ctrl_intf->dev;
311
312 __set_bit(EV_KEY, input->evbit);
313 __set_bit(BTN_A, input->keybit);
314 __set_bit(BTN_B, input->keybit);
315 __set_bit(BTN_X, input->keybit);
316 __set_bit(BTN_Y, input->keybit);
317 __set_bit(BTN_START, input->keybit);
318 __set_bit(BTN_SELECT, input->keybit);
319
320 __set_bit(EV_ABS, input->evbit);
321 input_set_abs_params(input, ABS_X, -1, 1, 0, 0);
322 input_set_abs_params(input, ABS_Y, -1, 1, 0, 0);
323
324 error = input_register_device(input);
325 if (error) {
326 dev_err(pcu->dev,
327 "Failed to register gamepad input device: %d\n",
328 error);
329 goto err_free_mem;
The input_register_device() can return a bunch of different error codes.
Better to preserve them. "error" is set.
330 }
331
332 pcu->gamepad = gamepad;
333 return 0;
334
335 err_free_mem:
336 input_free_device(input);
337 kfree(gamepad);
338 return -ENOMEM;
We just change this from "return -ENOMEM;" to "return error;"
339 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
@ 2020-06-03 14:45 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2020-06-03 14:45 UTC (permalink / raw)
To: Colin Ian King
Cc: Dmitry Torokhov, linux-input, kernel-janitors, linux-kernel
On Wed, Jun 03, 2020 at 03:18:46PM +0100, Colin Ian King wrote:
> On 03/06/2020 15:09, Dan Carpenter wrote:
> > On Wed, Jun 03, 2020 at 02:51:02PM +0100, Colin King wrote:
> >> From: Colin Ian King <colin.king@canonical.com>
> >>
> >> The variable error is being initialized with a value that is
> >> never read and the -ENOMEM error return is being returned anyhow
> >> by the error exit path to label err_free_mem. The assignment is
> >> redundant and can be removed.
> >>
> >> Addresses-Coverity: ("Unused value")
> >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >> ---
> >> drivers/input/misc/ims-pcu.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> >> index d8dbfc030d0f..4ba68aa3d281 100644
> >> --- a/drivers/input/misc/ims-pcu.c
> >> +++ b/drivers/input/misc/ims-pcu.c
> >> @@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
> >> if (!gamepad || !input) {
> >> dev_err(pcu->dev,
> >> "Not enough memory for gamepad device\n");
> >> - error = -ENOMEM;
> >> goto err_free_mem;
> >
> > It would be better to change the return instead.
> >
> > regards,
> > dan carpenter
> >
>
> I'm not sure about that, the err_free_mem path is used by another error
> exit return path that also needs to free the device and gamepad and
> returns ENOMEM, so I think this is a good enough shared error exit strategy.
The code looks like this:
drivers/input/misc/ims-pcu.c
284 static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
285 {
286 struct ims_pcu_gamepad *gamepad;
287 struct input_dev *input;
288 int error;
289
290 gamepad = kzalloc(sizeof(struct ims_pcu_gamepad), GFP_KERNEL);
291 input = input_allocate_device();
292 if (!gamepad || !input) {
293 dev_err(pcu->dev,
294 "Not enough memory for gamepad device\n");
295 error = -ENOMEM;
296 goto err_free_mem;
The "error" is always set before all the gotos.
297 }
298
299 gamepad->input = input;
300
301 snprintf(gamepad->name, sizeof(gamepad->name),
302 "IMS PCU#%d Gamepad Interface", pcu->device_no);
303
304 usb_make_path(pcu->udev, gamepad->phys, sizeof(gamepad->phys));
305 strlcat(gamepad->phys, "/input1", sizeof(gamepad->phys));
306
307 input->name = gamepad->name;
308 input->phys = gamepad->phys;
309 usb_to_input_id(pcu->udev, &input->id);
310 input->dev.parent = &pcu->ctrl_intf->dev;
311
312 __set_bit(EV_KEY, input->evbit);
313 __set_bit(BTN_A, input->keybit);
314 __set_bit(BTN_B, input->keybit);
315 __set_bit(BTN_X, input->keybit);
316 __set_bit(BTN_Y, input->keybit);
317 __set_bit(BTN_START, input->keybit);
318 __set_bit(BTN_SELECT, input->keybit);
319
320 __set_bit(EV_ABS, input->evbit);
321 input_set_abs_params(input, ABS_X, -1, 1, 0, 0);
322 input_set_abs_params(input, ABS_Y, -1, 1, 0, 0);
323
324 error = input_register_device(input);
325 if (error) {
326 dev_err(pcu->dev,
327 "Failed to register gamepad input device: %d\n",
328 error);
329 goto err_free_mem;
The input_register_device() can return a bunch of different error codes.
Better to preserve them. "error" is set.
330 }
331
332 pcu->gamepad = gamepad;
333 return 0;
334
335 err_free_mem:
336 input_free_device(input);
337 kfree(gamepad);
338 return -ENOMEM;
We just change this from "return -ENOMEM;" to "return error;"
339 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
2020-06-03 14:45 ` Dan Carpenter
@ 2020-06-03 15:10 ` Colin Ian King
-1 siblings, 0 replies; 10+ messages in thread
From: Colin Ian King @ 2020-06-03 15:10 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Dmitry Torokhov, linux-input, kernel-janitors, linux-kernel
On 03/06/2020 15:45, Dan Carpenter wrote:
> On Wed, Jun 03, 2020 at 03:18:46PM +0100, Colin Ian King wrote:
>> On 03/06/2020 15:09, Dan Carpenter wrote:
>>> On Wed, Jun 03, 2020 at 02:51:02PM +0100, Colin King wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> The variable error is being initialized with a value that is
>>>> never read and the -ENOMEM error return is being returned anyhow
>>>> by the error exit path to label err_free_mem. The assignment is
>>>> redundant and can be removed.
>>>>
>>>> Addresses-Coverity: ("Unused value")
>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>> ---
>>>> drivers/input/misc/ims-pcu.c | 1 -
>>>> 1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
>>>> index d8dbfc030d0f..4ba68aa3d281 100644
>>>> --- a/drivers/input/misc/ims-pcu.c
>>>> +++ b/drivers/input/misc/ims-pcu.c
>>>> @@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
>>>> if (!gamepad || !input) {
>>>> dev_err(pcu->dev,
>>>> "Not enough memory for gamepad device\n");
>>>> - error = -ENOMEM;
>>>> goto err_free_mem;
>>>
>>> It would be better to change the return instead.
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>> I'm not sure about that, the err_free_mem path is used by another error
>> exit return path that also needs to free the device and gamepad and
>> returns ENOMEM, so I think this is a good enough shared error exit strategy.
>
> The code looks like this:
>
> drivers/input/misc/ims-pcu.c
> 284 static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
> 285 {
> 286 struct ims_pcu_gamepad *gamepad;
> 287 struct input_dev *input;
> 288 int error;
> 289
> 290 gamepad = kzalloc(sizeof(struct ims_pcu_gamepad), GFP_KERNEL);
> 291 input = input_allocate_device();
> 292 if (!gamepad || !input) {
> 293 dev_err(pcu->dev,
> 294 "Not enough memory for gamepad device\n");
> 295 error = -ENOMEM;
> 296 goto err_free_mem;
>
> The "error" is always set before all the gotos.
>
> 297 }
> 298
> 299 gamepad->input = input;
> 300
> 301 snprintf(gamepad->name, sizeof(gamepad->name),
> 302 "IMS PCU#%d Gamepad Interface", pcu->device_no);
> 303
> 304 usb_make_path(pcu->udev, gamepad->phys, sizeof(gamepad->phys));
> 305 strlcat(gamepad->phys, "/input1", sizeof(gamepad->phys));
> 306
> 307 input->name = gamepad->name;
> 308 input->phys = gamepad->phys;
> 309 usb_to_input_id(pcu->udev, &input->id);
> 310 input->dev.parent = &pcu->ctrl_intf->dev;
> 311
> 312 __set_bit(EV_KEY, input->evbit);
> 313 __set_bit(BTN_A, input->keybit);
> 314 __set_bit(BTN_B, input->keybit);
> 315 __set_bit(BTN_X, input->keybit);
> 316 __set_bit(BTN_Y, input->keybit);
> 317 __set_bit(BTN_START, input->keybit);
> 318 __set_bit(BTN_SELECT, input->keybit);
> 319
> 320 __set_bit(EV_ABS, input->evbit);
> 321 input_set_abs_params(input, ABS_X, -1, 1, 0, 0);
> 322 input_set_abs_params(input, ABS_Y, -1, 1, 0, 0);
> 323
> 324 error = input_register_device(input);
> 325 if (error) {
> 326 dev_err(pcu->dev,
> 327 "Failed to register gamepad input device: %d\n",
> 328 error);
> 329 goto err_free_mem;
>
> The input_register_device() can return a bunch of different error codes.
> Better to preserve them. "error" is set.
>
> 330 }
> 331
> 332 pcu->gamepad = gamepad;
> 333 return 0;
> 334
> 335 err_free_mem:
> 336 input_free_device(input);
> 337 kfree(gamepad);
> 338 return -ENOMEM;
>
> We just change this from "return -ENOMEM;" to "return error;"
>
> 339 }
>
> regards,
> dan carpenter
>
Elegantly explained, thanks Dan, I'll send a V2.
Colin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'
@ 2020-06-03 15:10 ` Colin Ian King
0 siblings, 0 replies; 10+ messages in thread
From: Colin Ian King @ 2020-06-03 15:10 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Dmitry Torokhov, linux-input, kernel-janitors, linux-kernel
On 03/06/2020 15:45, Dan Carpenter wrote:
> On Wed, Jun 03, 2020 at 03:18:46PM +0100, Colin Ian King wrote:
>> On 03/06/2020 15:09, Dan Carpenter wrote:
>>> On Wed, Jun 03, 2020 at 02:51:02PM +0100, Colin King wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> The variable error is being initialized with a value that is
>>>> never read and the -ENOMEM error return is being returned anyhow
>>>> by the error exit path to label err_free_mem. The assignment is
>>>> redundant and can be removed.
>>>>
>>>> Addresses-Coverity: ("Unused value")
>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>> ---
>>>> drivers/input/misc/ims-pcu.c | 1 -
>>>> 1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
>>>> index d8dbfc030d0f..4ba68aa3d281 100644
>>>> --- a/drivers/input/misc/ims-pcu.c
>>>> +++ b/drivers/input/misc/ims-pcu.c
>>>> @@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
>>>> if (!gamepad || !input) {
>>>> dev_err(pcu->dev,
>>>> "Not enough memory for gamepad device\n");
>>>> - error = -ENOMEM;
>>>> goto err_free_mem;
>>>
>>> It would be better to change the return instead.
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>> I'm not sure about that, the err_free_mem path is used by another error
>> exit return path that also needs to free the device and gamepad and
>> returns ENOMEM, so I think this is a good enough shared error exit strategy.
>
> The code looks like this:
>
> drivers/input/misc/ims-pcu.c
> 284 static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
> 285 {
> 286 struct ims_pcu_gamepad *gamepad;
> 287 struct input_dev *input;
> 288 int error;
> 289
> 290 gamepad = kzalloc(sizeof(struct ims_pcu_gamepad), GFP_KERNEL);
> 291 input = input_allocate_device();
> 292 if (!gamepad || !input) {
> 293 dev_err(pcu->dev,
> 294 "Not enough memory for gamepad device\n");
> 295 error = -ENOMEM;
> 296 goto err_free_mem;
>
> The "error" is always set before all the gotos.
>
> 297 }
> 298
> 299 gamepad->input = input;
> 300
> 301 snprintf(gamepad->name, sizeof(gamepad->name),
> 302 "IMS PCU#%d Gamepad Interface", pcu->device_no);
> 303
> 304 usb_make_path(pcu->udev, gamepad->phys, sizeof(gamepad->phys));
> 305 strlcat(gamepad->phys, "/input1", sizeof(gamepad->phys));
> 306
> 307 input->name = gamepad->name;
> 308 input->phys = gamepad->phys;
> 309 usb_to_input_id(pcu->udev, &input->id);
> 310 input->dev.parent = &pcu->ctrl_intf->dev;
> 311
> 312 __set_bit(EV_KEY, input->evbit);
> 313 __set_bit(BTN_A, input->keybit);
> 314 __set_bit(BTN_B, input->keybit);
> 315 __set_bit(BTN_X, input->keybit);
> 316 __set_bit(BTN_Y, input->keybit);
> 317 __set_bit(BTN_START, input->keybit);
> 318 __set_bit(BTN_SELECT, input->keybit);
> 319
> 320 __set_bit(EV_ABS, input->evbit);
> 321 input_set_abs_params(input, ABS_X, -1, 1, 0, 0);
> 322 input_set_abs_params(input, ABS_Y, -1, 1, 0, 0);
> 323
> 324 error = input_register_device(input);
> 325 if (error) {
> 326 dev_err(pcu->dev,
> 327 "Failed to register gamepad input device: %d\n",
> 328 error);
> 329 goto err_free_mem;
>
> The input_register_device() can return a bunch of different error codes.
> Better to preserve them. "error" is set.
>
> 330 }
> 331
> 332 pcu->gamepad = gamepad;
> 333 return 0;
> 334
> 335 err_free_mem:
> 336 input_free_device(input);
> 337 kfree(gamepad);
> 338 return -ENOMEM;
>
> We just change this from "return -ENOMEM;" to "return error;"
>
> 339 }
>
> regards,
> dan carpenter
>
Elegantly explained, thanks Dan, I'll send a V2.
Colin
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-06-03 15:10 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 13:51 [PATCH] input: ims-pcu: remove redundant assignment to variable 'error' Colin King
2020-06-03 13:51 ` Colin King
2020-06-03 14:09 ` Dan Carpenter
2020-06-03 14:09 ` Dan Carpenter
2020-06-03 14:18 ` Colin Ian King
2020-06-03 14:18 ` Colin Ian King
2020-06-03 14:45 ` Dan Carpenter
2020-06-03 14:45 ` Dan Carpenter
2020-06-03 15:10 ` Colin Ian King
2020-06-03 15:10 ` Colin Ian King
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.