[bug report] HID: Introduce hidpp, a module to handle Logitech hid++ devices

[bug report] HID: Introduce hidpp, a module to handle Logitech hid++ devices

[Dan Carpenter]

[ This code is really old, but I was just looking at it as an example
of some Smatch warnings around hid_device_io_start() and I don't really
understand how that function works.  ]

Hello Benjamin Tissoires,

The patch 2f31c5252910: "HID: Introduce hidpp, a module to handle
Logitech hid++ devices" from Sep 30, 2014, leads to the following
Smatch static checker warning:

	drivers/hid/hid-logitech-hidpp.c:4205 hidpp_probe()
	warn: '&hdev->driver_input_lock' both locked and unlocked.

drivers/hid/hid-logitech-hidpp.c
    4117         /*
    4118          * Plain USB connections need to actually call start and open
    4119          * on the transport driver to allow incoming data.
    4120          */
    4121         ret = hid_hw_start(hdev, 0);
    4122         if (ret) {
    4123                 hid_err(hdev, "hw start failed\n");
    4124                 goto hid_hw_start_fail;
    4125         }
    4126 
    4127         ret = hid_hw_open(hdev);
    4128         if (ret < 0) {
    4129                 dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
    4130                         __func__, ret);
    4131                 goto hid_hw_open_fail;
    4132         }
    4133 
    4134         /* Allow incoming packets */
    4135         hid_device_io_start(hdev);


IO starts here.

    4136 
    4137         if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
    4138                 hidpp_unifying_init(hidpp);
    4139 
    4140         connected = hidpp_root_get_protocol_version(hidpp) == 0;
    4141         atomic_set(&hidpp->connected, connected);
    4142         if (!(hidpp->quirks & HIDPP_QUIRK_UNIFYING)) {
    4143                 if (!connected) {
    4144                         ret = -ENODEV;
    4145                         hid_err(hdev, "Device not connected");
    4146                         goto hid_hw_init_fail;
    4147                 }
    4148 
    4149                 hidpp_overwrite_name(hdev);
    4150         }
    4151 
    4152         if (connected && hidpp->protocol_major >= 2) {
    4153                 ret = hidpp_set_wireless_feature_index(hidpp);
    4154                 if (ret == -ENOENT)
    4155                         hidpp->wireless_feature_index = 0;
    4156                 else if (ret)
    4157                         goto hid_hw_init_fail;
    4158         }
    4159 
    4160         if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
    4161                 ret = wtp_get_config(hidpp);
    4162                 if (ret)
    4163                         goto hid_hw_init_fail;
    4164         } else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
    4165                 ret = g920_get_config(hidpp, &data);
    4166                 if (ret)
    4167                         goto hid_hw_init_fail;
    4168         }
    4169 
    4170         hidpp_connect_event(hidpp);
    4171 
    4172         /* Reset the HID node state */
    4173         hid_device_io_stop(hdev);

We stop the IO here, but not if g920_get_config() fails for example.  I
would normally put a hid_device_io_stop() before the goto, I guess?

Unrelated, to your driver but in ccp_probe() it calls hid_device_io_start().
Should there be a matching hid_device_io_stop() in the ccp_remove() function?

    4174         hid_hw_close(hdev);
    4175         hid_hw_stop(hdev);
    4176 
    4177         if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
    4178                 connect_mask &= ~HID_CONNECT_HIDINPUT;
    4179 
    4180         /* Now export the actual inputs and hidraw nodes to the world */
    4181         ret = hid_hw_start(hdev, connect_mask);
    4182         if (ret) {
    4183                 hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
    4184                 goto hid_hw_start_fail;
    4185         }
    4186 
    4187         if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
    4188                 ret = hidpp_ff_init(hidpp, &data);
    4189                 if (ret)
    4190                         hid_warn(hidpp->hid_dev,
    4191                      "Unable to initialize force feedback support, errno %d\n",
    4192                                  ret);

Should this do some cleanup if hidpp_ff_init() fails?

    4193         }
    4194 
    4195         return ret;
    4196 
    4197 hid_hw_init_fail:
    4198         hid_hw_close(hdev);
    4199 hid_hw_open_fail:
    4200         hid_hw_stop(hdev);
    4201 hid_hw_start_fail:
    4202         sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
    4203         cancel_work_sync(&hidpp->work);
    4204         mutex_destroy(&hidpp->send_mutex);
--> 4205         return ret;
    4206 }

regards,
dan carpenter