On Sat, 15 Aug 2020 at 01:30, Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> wrote:
Thanks for the quick response, I implemented most of the
items you talked about.
> > + *
> > + * Note: fan curve control has not been implemented
> > + */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +
> > +struct device_config {
> > + u16 vendor_id;
> > + u16 product_id;
> > + u8 fancount;
>
> I think you could be more consistent. Later in the code you use 'uin8_t'.
> I'd choose one type of fixed-width integer (either {u,s}N or [u]intN_t),
> and stick with it.
Changed all the int values to be the correct size and using uN.
> > + const struct hwmon_channel_info **hwmon_info;
> > +};
> > +
> > +struct hydro_i_pro_device {
> > + struct usb_device *udev;
> > +
> > + const struct device_config *config;
> > +
> > + unsigned char *bulk_out_buffer;
> > + char *bulk_in_buffer;
>
> Any reason why these two have different sizes? You cast both to
> 'unsigned char*' in set_fan_target_rpm(), set_fan_pwm_curve(), etc.
I am not sure what you mean by this, I should probably make the type
consistent between them, is that what you mean by "size"?
> As far as I see every device has two buffers for bulk in/out operations.
> Isn't it possible that - when multiple processes - try to interact with
> the sysfs attributes, more than one USB transactions use the same IO
> buffers? I suspect that could lead to data corruption.
Yes, I switched to using just a mutex, I saw this being used in the
usb_skel driver. I was told earlier to remove the mutex and keep the
semaphore, but a mutex makes more sense to me as well.
Fixed 2 bugs along the way!
> > +};
> > +
> > +struct hwmon_data {
> > + struct hydro_i_pro_device *hdev;
> > + int channel_count;
> > + void **channel_data;
>
> Why is this 'void**'? As far as I see this could be 'struct hwmon_fan_data**'.
No, I will have to add something like `hwmon_pump_data` for pump information.
This will also have to use fan/rpm hwmon interfaces so will share the same
channel range as `hwmon_fan_data`. This way I can just use channelnr as index.
If I made pump a separate array/value(it's almost always one pump) I could no
longer just use channelnr as index.
If you are against this then I can change it, but it is a deliberate
choice to make it
void and not an accident.
> But personally I'd use the "flexible array member" feature of C to deal with this,
> or even just a static array that's just big enough.
The "flexible array member" seems to give little benefit over what I
am doing right
now. Since there are at most 3 fans and 1 pump in a realistic
configuration a static
array doesn't seem that bad actually.
> > +};
> > +
> > +struct curve_point {
> > + uint8_t temp;
> > + uint8_t pwm;
> > +};
> > +
> > +struct hwmon_fan_data {
> > + char fan_channel;
> > + long fan_target;
> > + unsigned char fan_pwm_target;
>
> This is very nitpicky, but any reason why is it not 'uint8_t' like above?
> This applies to other variables as well, I think some variables are too big for what they store,
> or have a sign, when they could be unsigned.
I moved all the pwm values to u8, and a;ll rpm values to u16(as they
can't be any bigger)
I usually don't really write c code, all the different types for the
same thing confuse me in
which one to use.
> > + long mode;
>
> If the only valid modes are 0, 1, and 2, why the 'long'?
Also moved to u8.
> > +}
> > +
> > +static const struct device_config *get_device_configuration(u16 vendor_id,
> > + u16 product_id)
> > +{
> > + const struct device_config *config;
> > + int i = 0;
> > + int n = ARRAY_SIZE(config_table);
> > + for (i = 0; i < n; i++) {
> > + config = &config_table[i];
> > + if (config->vendor_id == vendor_id &&
> > + config->product_id == product_id) {
> > + return config;
> > + }
> > + }
> > + return config;
> > +}
> > +
>
> I think this can be gotten rid of by utilizing the "driver_info" field of the
> usb_device_id struct as seen in drivers/usb/serial/visor.c. That way you could
> store a pointer or an index, and there would be no need to do any lookup. Like this:
>
> { USB_DEVICE(VID, PID), .driver_info = (kernel_ulong_t) &config_table[0] }
That is nice!
> > +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;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> > + unsigned char *send_buf = hdev->bulk_out_buffer;
> > + unsigned char *recv_buf = hdev->bulk_in_buffer;
> > +
> > + memcpy(fan_data->curve, point, sizeof(fan_data->curve));
> > +
> > + 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 = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 16, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > +
> > + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > +
> > + if (!check_succes(send_buf[0], recv_buf)) {
> > + dev_info(&hdev->udev->dev,
> > + "[*] failed setting fan curve %d,%d,%d/%d\n",
> > + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +static int set_fan_target_rpm(struct hydro_i_pro_device *hdev,
> > + struct hwmon_fan_data *fan_data, long val)
> > +{
> > + int retval;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> > +
> > + unsigned char *send_buf = hdev->bulk_out_buffer;
> > + unsigned char *recv_buf = hdev->bulk_in_buffer;
> > +
> > + fan_data->fan_target = val;
> > + fan_data->fan_pwm_target = 0;
> > +
> > + send_buf[0] = RPM_FAN_TARGET_CMD;
> > + send_buf[1] = fan_data->fan_channel;
> > + send_buf[2] = (fan_data->fan_target >> 8);
> > + send_buf[3] = fan_data->fan_target;
>
> As far as I see 'fan_data->fan_target' is an unsigned 16 bit number, so anything that is
> out of range should be rejected - preferably in hwmon_write() - with -EINVAL in my opinion,
> since 'fan_data->fan_target' may not accurately represent the state of the hardware
> if other values are allowed.
>
> The lack of range checking applies to other parts of the code as well.
I put range checks around all input -> fan/pwm target conversions, if
you found any other
places let me know.
> > +
> > + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > +
> > + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > +
> > + if (!check_succes(send_buf[0], recv_buf)) {
> > + dev_info(&hdev->udev->dev,
> > + "[*] failed setting fan rpm %d,%d,%d/%d\n",
> > + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +static int get_fan_current_rpm(struct hydro_i_pro_device *hdev,
> > + struct hwmon_fan_data *fan_data, long *val)
> > +{
> > + int retval;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> > +
> > + unsigned char *send_buf = hdev->bulk_out_buffer;
> > + unsigned char *recv_buf = hdev->bulk_in_buffer;
> > +
> > + send_buf[0] = PWM_GET_CURRENT_CMD;
> > + send_buf[1] = fan_data->fan_channel;
> > +
> > + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 2, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > +
> > + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > +
> > + if (!check_succes(send_buf[0], recv_buf) ||
> > + recv_buf[3] != fan_data->fan_channel) {
> > + dev_info(&hdev->udev->dev,
> > + "[*] failed retrieving fan rmp %d,%d,%d/%d\n",
> > + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> > + return -EINVAL;
> > + }
> > +
> > + *val = ((recv_buf[4]) << 8) + recv_buf[5];
> > + return 0;
> > +}
> > +
> > +static int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
> > + struct hwmon_fan_data *fan_data, long val)
> > +{
> > + int retval;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> > +
> > + unsigned char *send_buf = hdev->bulk_out_buffer;
> > + unsigned char *recv_buf = hdev->bulk_in_buffer;
> > +
> > + fan_data->fan_pwm_target = val;
> > + fan_data->fan_target = 0;
> > +
> > + send_buf[0] = PWM_FAN_TARGET_CMD;
> > + send_buf[1] = fan_data->fan_channel;
> > + send_buf[3] = fan_data->fan_pwm_target;
> > +
> > + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > +
> > + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > +
> > + if (!check_succes(send_buf[0], recv_buf)) {
> > + dev_info(&hdev->udev->dev,
> > + "[*] failed setting fan pwm %d,%d,%d/%d\n",
> > + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
>
> The previous four functions are structurally more or less the same,
> I think the USB related parts could be placed into their own dedicated function.
I could, but the only part that could actually be split up is the usb_bulk_msg.
If I would remove the error code that part could also be split up.
Are there any conventions around what to log/not to log? Maybe it is best to
remove those log messages anyways.
> > +
> > + data->channel_count = hdev->config->fancount;
> > + data->channel_data =
> > + devm_kzalloc(&hdev->udev->dev,
> > + sizeof(char *) * data->channel_count, GFP_KERNEL);
>
> This should be 'sizeof(fan)' or 'sizeof(struct hwmon_fan_data*)', no?
Irrelevant since I moved to a fixed-size array.
> > +
> > + /* 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 = 2;
>
> Can't it be queried what the mode actually is?
Maybe?? I don't actually know, would take some debugging/wireshark to
figure out.
(there were some problems with wireshark and starting the windows
driver from what I
remember, so might not be possible to find out) by default it uses the
dafault profile
present in the driver. Only if another program interferes with the
device would this not
match.
> > +
> > + 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, "driver_fan", data, hwmon_info, NULL);
>
> Personally, I'd choose a different name, since "driver_fan" is not really
> descriptive.
I just made the name be part of the device config, this way its device
specific.
> > + {},
> > +};
> > +
> > +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.
> ^
> Very nitpicky, but a space is missing there.
I knew I missed one of them! :(
> > + }
> > +
> > + hwmon_init(hdev);
> > +
> > + usb_set_intfdata(interface, hdev);
> > + sema_init(&hdev->limit_sem, 8);
> > +exit:
> > + return retval;
> > +}
> > +
> > +static void astk_disconnect(struct usb_interface *interface)
> > +{
> > + struct hydro_i_pro_device *hdev = usb_get_intfdata(interface);
> > +
> > + dev_info(&hdev->udev->dev, "[*] DEINIT DEVICE\n");
>
> In my opinion the style of the log messages is not consistent,
> sometimes you use all-caps, sometimes it's all lowercase, sometimes
> it has punctuation.
Again I couldn't find anything on logging style within the kernel, I am leaning
towards just removing them all together. If you can find any logging style
guide link me to it, if not I will just remove all the logs,
> > [...]
> > +static int __init hydro_i_pro_init(void)
> > +{
> > + return usb_register(&hydro_i_pro_driver);
> > +}
> > +
> > +static void __exit hydro_i_pro_exit(void)
> > +{
> > + usb_deregister(&hydro_i_pro_driver);
> > +}
> > +
> > +module_init(hydro_i_pro_init);
> > +module_exit(hydro_i_pro_exit);
>
> Any reason for not using the module_usb_driver() macro?
Just didn't know it existed.
Once you reply to my open comments I will send in V4,
if it's prefered to just add V4 not and address those comments later
let me know and I will just post them.
Kind regards,
Jaap Aarts
