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

[Guenter Roeck <linux@roeck-us.net>]

On 8/15/20 3:54 PM, Barnabás Pőcze wrote:
> Hello,
> 
> there are a couple things that I did not notice in v3, or were introduced in
> this version of the patch.
> 
> 
>> [...]
>> +
>> +#define max_fan_count 2
>> +#define max_pwm_channel_count max_fan_count
>> +
> 
> I think these should be all-caps as per
> https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
> 
> 
>> [...]
>> +
>> +#define default_curve quiet_curve
>> +
> 
> I am inclined to say that this should be all-caps as well.
> 
> 
>> [...]
>> +static int transfer_usb(struct hydro_i_pro_device *hdev,
>> +			unsigned char *send_buf, unsigned char *recv_buf,
>> +			int send_len, int recv_len)
>> +{
>> +	int retval;
>> +	int wrote;
>> +	int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
>> +	int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
>> +
>> +	retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, send_len, &wrote,
>> +			      BULK_TIMEOUT);
>> +	if (retval)
>> +		return retval;
>> +
>> +	retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
>> +			      BULK_TIMEOUT);
>> +	if (retval)
>> +		return retval;
>> +	return 0;
> 
> The preceding 5 lines could be simplified to the following:
> 
> return usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
> 		    BULK_TIMEOUT);
> 
> And if you don't use 'wrote', you can simply pass 'NULL' as the 5th argument of
> usb_bulk_msg(). Although you should either check the value or set 'recv_buf' to
> all zeroes in the calling function to avoid the possibility of a failed transaction
> being recognized as successful.
> 
> 
>> +}
>> +
>> +static int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
>> +			     struct hwmon_fan_data *fan_data,
>> +			     struct curve_point point[7])
>> +{
>> +	int retval;
>> +	unsigned char *send_buf = hdev->bulk_out_buffer;
>> +	unsigned char *recv_buf = hdev->bulk_in_buffer;
>> +
>> +	memcpy(fan_data->curve, point, sizeof(struct curve_point) * 7);
>> +
> 
> Personally, I'd use 'sizeof(fan_data->curve)' here. And consider making that
> seven a named constant.
> 
> Beware that even though the size is there in 'struct curve_point point[7]',
> you can still pass arrays that are smaller than that. Consider using
> 'struct curve_point point[static 7]'.
> 
> 
>> +	send_buf[0] = PWM_FAN_CURVE_CMD;
>> +	send_buf[1] = fan_data->fan_channel;
>> +	send_buf[2] = point[0].temp;
>> +	send_buf[3] = point[1].temp;
>> +	send_buf[4] = point[2].temp;
>> +	send_buf[5] = point[3].temp;
>> +	send_buf[6] = point[4].temp;
>> +	send_buf[7] = point[5].temp;
>> +	send_buf[8] = point[6].temp;
>> +	send_buf[9] = point[0].pwm;
>> +	send_buf[10] = point[1].pwm;
>> +	send_buf[11] = point[2].pwm;
>> +	send_buf[12] = point[3].pwm;
>> +	send_buf[13] = point[4].pwm;
>> +	send_buf[14] = point[5].pwm;
>> +	send_buf[15] = point[6].pwm;
>> +
>> +	retval = transfer_usb(hdev, send_buf, recv_buf, 16, 4);
>> +	if (retval)
>> +		return retval;
>> +
>> +	if (!check_succes(send_buf[0], recv_buf)) {
>> +		dev_warn(
>> +			&hdev->udev->dev,
>> +			"failed setting fan pwm/temp curve for %s on channel %d %d,%d,%d\n",
>> +			hdev->config->name, recv_buf[3], recv_buf[0],
>> +			recv_buf[1], recv_buf[2]);
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>> [...]
>> +
>> +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;
>> +
>> +	send_buf[0] = PWM_FAN_TARGET_CMD;
>> +	send_buf[1] = fan_data->fan_channel;
>> +	send_buf[3] = fan_data->fan_pwm_target;
>> +
>> +	retval = transfer_usb(hdev, send_buf, recv_buf, 4, 6);
>> +	if (retval)
>> +		return retval;
>> +
>> +	if (!check_succes(send_buf[0], recv_buf)) {
> 
> Any reason why you don't check recv_buf[3] as in get_fan_current_rpm()?
> (same applies to set_fan_pwm_curve() and set_fan_target_rpm())
> 
> 
>> +		dev_warn(
>> +			&hdev->udev->dev,
>> +			"failed setting fan pwm for %s on channel %d %d,%d,%d\n",
>> +			hdev->config->name, recv_buf[3], 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;
>> +
>> +			if (val < (2 ^ 16) - 2)
> 
> Did you intend to write 2 to the power of 16? Because 2^16 is not that.
> 2 to the power 16 may be written as '(1 << 16)' or 'BIT(16)'
> (you may need to include linux/bits.h for the latter)
> 
> But something like this is my suggestion:
> 
> if (val < 0 || 0xFFFF < val)
> 	return -EINVAL;
> 

I would suggest to use clamp_val; we don't expect users to know device
specific limits. Also, FTR, I won't accept any Yoda programming.

Guenter

> Though, I suspect the fans won't go up to 60000 RPM or so.
> 
> 
>> +				return -EINVAL;
>> +
>> +			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;
>> +
>> +			if (val < (2 ^ 8) - 2)
> 
> Same here, 2^8 != 2 to the power of 8.
> 
> I suggest you simply do something like the following:
> 
> if (val < 0 || 0xFF < val)
> 	return -EINVAL;
> 
> 
>> +				return -EINVAL;
>> +
>> +			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 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;
>> +				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;
>> +			case 2:
>> +				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 = 2;
>> +				break;
> 
> If I see it correctly, case 0 and case 2 are the basically the same, no?
> If so, then you could merge them.
> 
> 
>> +			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);
>> +			if (retval)
>> +				goto cleanup;
>> +
>> +			goto cleanup;
> 
> The preceding 3 lines can be replaced by a single 'break' given that the
> 'goto exit' is replaced by a 'break' after the 'switch (attr)'
> 
> 
>> +		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 = 200;
>> +			goto exit;
>> +
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +		goto exit;
>> +
> 
> I don't see why this goto is needed here. It has no effect on the control flow.
> 
> 
>> +	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;
>> +	}
>> +
>> +cleanup:
>> +	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 void hwmon_init(struct hydro_i_pro_device *hdev)
>> +{
>> +	u8 fan_id;
>> +	struct device *hwmon_dev;
>> +	struct hwmon_fan_data *fan;
>> +	struct hwmon_data *data = devm_kzalloc(
>> +		&hdev->udev->dev, sizeof(struct hwmon_data), GFP_KERNEL);
>> +	struct hwmon_chip_info *hwmon_info = devm_kzalloc(
>> +		&hdev->udev->dev, sizeof(struct hwmon_chip_info), GFP_KERNEL);
>> +
>> +	/* You did something bad!! Either adjust  max_fan_count or the fancount for the config!*/
>> +	WARN_ON(hdev->config->fancount >= max_pwm_channel_count);
> 
> If this warning is triggered, then that leads to the overflow of 'data->channel_data' later on.
> 
> 
>> +	data->channel_count = hdev->config->fancount;
>> +
>> +	/* 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);
>> +		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);
>> +	dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
>> +}
>> +
> 
> There is absolutely no error checking in hwmon_init().
> 
> 
>> +const int USB_VENDOR_ID_CORSAIR = 0x1b1c;
>> +const int USB_PRODUCT_ID_H100I_PRO = 0x0c15;
> 
> I think these should be either - preferably - #define's or 'static' at least.
> 
> 
>> +/*
>> + * Devices that work with this driver.
>> + * More devices should work, however none have been tested.
>> + */
>> +static const struct usb_device_id astk_table[] = {
>> +	{ USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO),
>> +	  .driver_info = (kernel_ulong_t)&config_table[0] },
>> +	{},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(usb, astk_table);
>> +
>> +static int init_device(struct usb_device *udev)
>> +{
>> +	int retval;
>> +
>> +	/*
>> +	 * This is needed because when running windows in a vm with proprietary driver
>> +	 * and you switch to this driver, the device will not respond unless you run this.
>> +	 */
>> +	retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
>> +				 0xffff, 0x0000, 0, 0, 0);
>> +	/*this always returns error*/
>> +	if (retval)
>> +		;
> 
> Shouldn't you check if it's the "good" kind of error?
> 
> 
>> +
>> +	retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
>> +				 0x0002, 0x0000, 0, 0, 0);
>> +	return retval;
>> +}
>> +
>> +static int deinit_device(struct usb_device *udev)
>> +{
>> +	return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
>> +			       0x0004, 0x0000, 0, 0, 0);
>> +}
>> +
>> +static void astk_delete(struct hydro_i_pro_device *hdev)
>> +{
>> +	usb_put_intf(hdev->interface);
>> +	usb_put_dev(hdev->udev);
>> +	kfree(hdev->bulk_in_buffer);
>> +	kfree(hdev->bulk_out_buffer);
>> +	kfree(hdev);
>> +}
>> +
> 
> I think you should call mutex_destroy() in astk_delete().
> 
> 
>> +static int astk_probe(struct usb_interface *interface,
>> +		      const struct usb_device_id *id)
>> +{
>> +	struct hydro_i_pro_device *hdev =
>> +		kzalloc(sizeof(struct hydro_i_pro_device), GFP_KERNEL);
> 
> I think this should be modifed to use 'sizeof(*ptr)' as per
> https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory
> (and everything else that uses the same pattern)
> 
> 
> 
>> [...]
>> +	retval = init_device(hdev->udev);
>> +	if (retval) {
>> +		dev_err(&interface->dev, "failed initialising %s\n",
>> +			hdev->config->name);
> 
> If you print the error code, that helps immensely with troubleshooting.
> 
> 
>> +		kfree(hdev);
>> +		goto exit;
>> +	}
>> +
>> +	hwmon_init(hdev);
>> +
>> +	usb_set_intfdata(interface, hdev);
>> +	mutex_init(&hdev->io_mutex);
>> +exit:
>> +	return retval;
>> +}
>> +
>> [...]
> 
> 
> Barnabás Pőcze
>