> [...]
> +static int hwmon_init(struct hydro_i_pro_device *hdev)
> +{
> + u8 fan_id;
> + struct device *hwmon_dev;
> + struct hwmon_fan_data *fan;
> + struct hwmon_data *data;
> + struct hwmon_chip_info *hwmon_info;
> +
> + data = devm_kzalloc(&hdev->udev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + hwmon_info =
> + devm_kzalloc(&hdev->udev->dev, sizeof(hwmon_info), GFP_KERNEL);
> + if (!hwmon_info) {
> + free(data);
> + return -ENOMEM;
> + }
If you use devm_kzalloc() there is no need to free it. (But see my comments below.)
And you missed a '*' in the sizeof.
> +
> + /* You did something bad!! Either adjust MAX_FAN_COUNT or the fancount for the config!*/
> + WARN_ON(hdev->config->fancount >= MAX_PWM_CHANNEL_COUNT);
> + data->channel_count =
> + clamp_val(hdev->config->fancount, 0, MAX_PWM_CHANNEL_COUNT);
> +
> + /* For each fan create a data channel a fan config entry and a pwm config entry */
> + for (fan_id = 0; fan_id < data->channel_count; fan_id++) {
> + fan = devm_kzalloc(&hdev->udev->dev, sizeof(fan), GFP_KERNEL);
> + if (!fan)
> + return -ENOMEM;
> +
> + fan->fan_channel = fan_id;
> + fan->mode = 0;
> + data->channel_data[fan_id] = fan;
> + }
> +
> + hwmon_info->ops = &i_pro_ops;
> + hwmon_info->info = hdev->config->hwmon_info;
> +
> + data->hdev = hdev;
> + hwmon_dev = devm_hwmon_device_register_with_info(
> + &hdev->udev->dev, hdev->config->name, data, hwmon_info, NULL);
> + if (IS_ERR(hwmon_dev))
> + return PTR_ERR(hwmon_dev);
> +
> + dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
> + return 0;
> +}
> [...]
> +static void astk_delete(struct hydro_i_pro_device *hdev)
> +{
> + usb_put_intf(hdev->interface);
> + usb_put_dev(hdev->udev);
> + mutex_destroy(&hdev->io_mutex);
Now that you changed 'hdev' to be devm allocated, this may lead to a problem here,
since "putting" the usb device may cause all associated allocations to be freed,
which means that 'hdev' may be freed by the time you want to destroy the mutex.
> +}
> +
> +static int astk_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *udev = usb_get_dev(interface_to_usbdev(interface));
> + struct hydro_i_pro_device *hdev;
> + int retval;
> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> +
> + hdev = devm_kzalloc(&udev->dev, sizeof(*hdev), GFP_KERNEL);
> +
> + if (!hdev) {
> + retval = -ENOMEM;
> + goto exit;
> + }
> +
> + hdev->config = (const struct device_config *)id->driver_info;
> + /* You should set the driver_info to a pointer to the correct configuration!!*/
> + WARN_ON(!hdev->config);
> +
> + retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
> + &bulk_out, NULL, NULL);
> + if (retval) {
> + kfree(hdev);
> + goto exit;
> + }
> +
> + hdev->udev = ;
> + hdev->interface = usb_get_intf(interface);
> +
> + /*
> + * set up the endpoint information
> + * use only the first bulk-in and bulk-out endpoints
> + */
> + hdev->bulk_in_size = usb_endpoint_maxp(bulk_in);
> + hdev->bulk_in_buffer =
> + devm_kzalloc(&hdev->udev->dev, hdev->bulk_in_size, GFP_KERNEL);
> + hdev->bulk_in_endpointAddr = bulk_in->bEndpointAddress;
> + if (!hdev->bulk_in_buffer) {
> + kfree(hdev);
> + goto exit;
> + }
> +
> + hdev->bulk_out_size = usb_endpoint_maxp(bulk_out);
> + hdev->bulk_out_buffer =
> + devm_kzalloc(&hdev->udev->dev, hdev->bulk_out_size, GFP_KERNEL);
> + hdev->bulk_out_endpointAddr = bulk_out->bEndpointAddress;
> + if (!hdev->bulk_out_buffer) {
> + kfree(hdev);
> + kfree(hdev->bulk_in_buffer);
> + goto exit;
> + }
> +
> + retval = init_device(hdev->udev);
> + if (retval) {
> + dev_err(&interface->dev, "failed initialising %s:%d\n",
> + hdev->config->name, retval);
> + kfree(hdev);
> + kfree(hdev->bulk_in_buffer);
> + kfree(hdev->bulk_out_buffer);
> + goto exit;
> + }
> +
> + retval = hwmon_init(hdev);
> + if (retval) {
> + dev_err(&interface->dev, "failed initialising hwmon %s:%d\n",
> + hdev->config->name, retval);
> + kfree(hdev);
> + kfree(hdev->bulk_in_buffer);
> + kfree(hdev->bulk_out_buffer);
I hope what I said wasn't confusing, but - like you said - if you use
devm_* you don't need to explicitly free it. Furthermore, you access a pointer
that has already been freed here.
devm_k.alloc() and devm_kfree() OR k.alloc() and kfree(), do not mix the two.
In case of failure I think it makes sense to free the resources either way, but
it is not strictly necessary since devm resources will be freed when the device
is gone.
> + goto exit;
> + }
> +
> + usb_set_intfdata(interface, hdev);
> + mutex_init(&hdev->io_mutex);
> +exit:
> + return retval;
> +}
> [...]
Please think more about memory management and how it should work in this module.
Only manage devm resources between "getting" and "putting" the device.
Don't do it after "putting" or before "getting" the device.
And please compile and test your code (among other things) before submitting as per
https://www.kernel.org/doc/html/latest/hwmon/submitting-patches.html
Barnabás Pőcze
