Re: [PATCH V5] hwmon: add fan/pwm driver for corsair h100i platinum

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

I added a couple notes about the code inline.


> [...]
> +#define MAX_FAN_COUNT 2
> +#define MAX_PWM_CHANNEL_COUNT MAX_FAN_COUNT
> +
> +struct hwmon_data {
> +	struct hydro_i_pro_device *hdev;
> +	int channel_count;

This is a nitpick, but the 'channel_count' value comes from the 'fancount'
of an entry of 'config_table', which has type 'u8', so I don't see any need for
it to be an 'int'.

> +	void *channel_data[MAX_PWM_CHANNEL_COUNT];
> +};
> [...]
> +struct curve_point quiet_curve[FAN_CURVE_COUNT] = {

This should be 'static' (and possibly 'const'?).


> +	{
> +		.temp = 0x1F,
> +		.pwm = 0x15,
> +	},
> +	{
> +		.temp = 0x21,
> +		.pwm = 0x1E,
> +	},
> +	{
> +		.temp = 0x24,
> +		.pwm = 0x25,
> +	},
> +	{
> +		.temp = 0x27,
> +		.pwm = 0x2D,
> +	},
> +	{
> +		.temp = 0x29,
> +		.pwm = 0x38,
> +	},
> +	{
> +		.temp = 0x2C,
> +		.pwm = 0x4A,
> +	},
> +	{
> +		.temp = 0x2F,
> +		.pwm = 0x64,
> +	},
> +};
> [...]
> +static const struct device_config config_table[] = {
> +	{
> +		.vendor_id = 0x1b1c,
> +		.product_id = 0x0c15,

If I see it correctly, you never use 'vendor_id', nor 'product_id', right?


> +		.fancount = 2,
> +		.rpm_min = 200,
> +		.rpm_max = 3000,
> +		.pwm_max = 100,
> +		.name = "corsair_H100i_pro",
> +		.hwmon_info = dual_fan,
> +	},
> +};
> [...]
> +
> +#define SUCCES_LENGTH 3
> +#define SUCCES_CODE 0x12, 0x34
> +
> +static bool check_succes(enum opcodes command, char ret[SUCCES_LENGTH])

I missed this it seems, but my suggestion about the 'static' before the size
applies here as well.


> +{
> +	char success[SUCCES_LENGTH] = { command, SUCCES_CODE };
> +
> +	return memcmp(ret, success, SUCCES_LENGTH) == 0;
> +}
> +
> +static int acquire_lock(struct hydro_i_pro_device *hdev)
> +{
> +	int retval = usb_autopm_get_interface(hdev->interface);
> +
> +	if (retval)
> +		return retval;
> +
> +	mutex_lock(&hdev->io_mutex);
> +	return 0;

I think this should be

return mutex_lock_interruptible(...);

as per
https://www.kernel.org/doc/html/latest/kernel-hacking/locking.html#locking-only-in-user-context


> +}
> [...]
> +static int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
> +			      struct hwmon_fan_data *fan_data, u8 val)
> +{
> +	int retval;
> +	unsigned char *send_buf = hdev->bulk_out_buffer;
> +	unsigned char *recv_buf = hdev->bulk_in_buffer;
> +
> +	fan_data->fan_target = 0;
> +	fan_data->fan_pwm_target = val;

Personally, I'd place these two lines at the end of the function so that they
are not modified if the USB transaction fails.
(same applies to set_fan_pwm_curve() and set_fan_target_rpm())


> +
> +	send_buf[0] = PWM_FAN_TARGET_CMD;
> +	send_buf[1] = fan_data->fan_channel;
> +	send_buf[2] = fan_data->fan_pwm_target;
> +	dev_info(&hdev->udev->dev, "debug:%d,%d,%d", send_buf[0], send_buf[1],
> +		 send_buf[2]);
> +	dev_info(&hdev->udev->dev, "val:%d", fan_data->fan_pwm_target);

This should be dev_dbg() in my opinion if you intend to have such messages.


> +
> +	retval = transfer_usb(hdev, send_buf, recv_buf, 3, 3);
> +	if (retval)
> +		return retval;
> +
> +	if (!check_succes(send_buf[0], recv_buf)) {
> +		dev_warn(
> +			&hdev->udev->dev,
> +			"failed setting fan pwm for %s on channel %d %d,%d,%d\n",
> +			hdev->config->name, fan_data->fan_channel, recv_buf[0],
> +			recv_buf[1], recv_buf[2]);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> [...]
> +static int hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +		       u32 attr, int channel, long val)
> +{
> +	struct hwmon_data *data = dev_get_drvdata(dev);
> +	struct hydro_i_pro_device *hdev = data->hdev;
> +	struct hwmon_fan_data *fan_data;
> +	int retval = 0;
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_target:
> +			fan_data = data->channel_data[channel];
> +			if (fan_data->mode != 1)
> +				return -EINVAL;
> +
> +			val = clamp_val(val, hdev->config->rpm_min,
> +					hdev->config->rpm_max);
> +
> +			retval = acquire_lock(hdev);
> +			if (retval)
> +				goto exit;
> +
> +			retval = set_fan_target_rpm(hdev, fan_data, val);
> +			if (retval)
> +				goto cleanup;
> +
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			fan_data = data->channel_data[channel];
> +			if (fan_data->mode != 1)
> +				return -EINVAL;
> +
> +			val = clamp_val(val, 0, hdev->config->pwm_max);
> +			dev_info(&hdev->udev->dev, "val:%ld", val);

Same here regarding dev_dbg().


> +			retval = acquire_lock(hdev);
> +			if (retval)
> +				goto exit;
> +
> +			retval = set_fan_target_pwm(hdev, fan_data, val);
> +			if (retval)
> +				goto cleanup;
> +
> +			break;
> +		case hwmon_pwm_enable:
> +			fan_data = data->channel_data[channel];
> +
> +			switch (val) {
> +			case 2:
> +			case 0:
> +				retval = acquire_lock(hdev);
> +				if (retval)
> +					goto exit;
> +
> +				retval = set_fan_pwm_curve(hdev, fan_data,
> +							   DEFAULT_CURVE);
> +				if (retval)
> +					goto cleanup;
> +				fan_data->mode = 0;

This could be 'fan_data->mode = val', no?


> +				break;
> +			case 1:
> +				retval = acquire_lock(hdev);
> +				if (retval)
> +					goto exit;
> +
> +				if (fan_data->fan_target != 0) {
> +					retval = set_fan_target_rpm(
> +						hdev, fan_data,
> +						fan_data->fan_target);
> +					if (retval)
> +						goto cleanup;
> +				} else if (fan_data->fan_pwm_target != 0) {
> +					retval = set_fan_target_pwm(
> +						hdev, fan_data,
> +						fan_data->fan_pwm_target);
> +					if (retval)
> +						goto cleanup;
> +				}
> +				fan_data->mode = 1;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +cleanup:
> +	mutex_unlock(&hdev->io_mutex);
> +	usb_autopm_put_interface(hdev->interface);
> +exit:
> +	return retval;
> +}
> +
> +static int hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +		      u32 attr, int channel, long *val)
> +{
> +	struct hwmon_data *data = dev_get_drvdata(dev);
> +	struct hydro_i_pro_device *hdev = data->hdev;
> +	struct hwmon_fan_data *fan_data;
> +	int retval = 0;
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			fan_data = data->channel_data[channel];
> +
> +			retval = acquire_lock(hdev);
> +			if (retval)
> +				goto exit;
> +
> +			retval = get_fan_current_rpm(hdev, fan_data, val);
> +			break;
> +		case hwmon_fan_target:
> +			fan_data = data->channel_data[channel];
> +			if (fan_data->mode != 1) {
> +				*val = 0;
> +				goto exit;
> +			}
> +			*val = fan_data->fan_target;
> +			goto exit;
> +		case hwmon_fan_min:
> +			*val = hdev->config->rpm_min;
> +			goto exit;
> +

Any reason why you don't handle 'hwmon_fan_max' as well?


> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_enable:
> +			fan_data = data->channel_data[channel];
> +			*val = fan_data->mode;
> +			goto exit;
> +		default:
> +			return -EINVAL;
> +		}
> +		goto exit;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_unlock(&hdev->io_mutex);
> +	usb_autopm_put_interface(hdev->interface);
> +exit:
> +	return retval;
> +}
> +
> +static const struct hwmon_ops i_pro_ops = {
> +	.is_visible = hwmon_is_visible,
> +	.read = hwmon_read,
> +	.write = hwmon_write,
> +};
> +
> +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);
> +	hwmon_info = devm_kzalloc(&hdev->udev->dev,
> +				  sizeof(struct hwmon_chip_info), GFP_KERNEL);

I think you missed this 'sizeof' when coverting to 'sizeof(*ptr)'.


> +
> +	/* 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(struct hwmon_fan_data), GFP_KERNEL);

And this one as well.


> +		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;
> +}

It is still possible that hwmon_init() leaks memory on failure.


> [...]
> +static int astk_probe(struct usb_interface *interface,
> +		      const struct usb_device_id *id)
> +{
> +	struct hydro_i_pro_device *hdev;
> +	int retval;
> +	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> +
> +	hdev = kzalloc(sizeof(*hdev), GFP_KERNEL);
> +
> +	if (!hdev) {
> +		kfree(hdev);

If this 'if' is taken, then hdev == NULL, so no need to free it.


> +		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 = usb_get_dev(interface_to_usbdev(interface));
> +	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 = kmalloc(hdev->bulk_in_size, GFP_KERNEL);
> +	hdev->bulk_in_endpointAddr = bulk_in->bEndpointAddress;
> +	hdev->bulk_out_size = usb_endpoint_maxp(bulk_out);
> +	hdev->bulk_out_buffer = kmalloc(hdev->bulk_out_size, GFP_KERNEL);

I think both allocations should be checked.


> +	hdev->bulk_out_endpointAddr = bulk_out->bEndpointAddress;
> +
> +	retval = init_device(hdev->udev);
> +	if (retval) {
> +		dev_err(&interface->dev, "failed initialising %s:%d\n",
> +			hdev->config->name, retval);
> +		kfree(hdev);
> +		goto exit;
> +	}
> +
> +	retval = hwmon_init(hdev);
> +	if (retval) {
> +		dev_err(&interface->dev, "failed initialising hwmon%s:%d\n",
                                                                  ^^^^^^
I think you need more spaces here.


> +			hdev->config->name, retval);
> +		kfree(hdev);
> +		goto exit;
> +	}
> +
> +	usb_set_intfdata(interface, hdev);
> +	mutex_init(&hdev->io_mutex);
> +exit:
> +	return retval;
> +}
> [...]


Barnabás Pőcze




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux