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@xxxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxxx>
>>>> ---
>>>> 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
