Hi Thomas, On Mon, Apr 24, 2023 at 4:04 PM Thomas Weißschuh <thomas@xxxxxxxx> wrote: > > On 2023-04-24 15:33:26-0500, Jorge Lopez wrote: > > Hi Thomas, > > > > Please see my comments below. > > > > On Sat, Apr 22, 2023 at 4:30 PM Thomas Weißschuh <thomas@xxxxxxxx> wrote: > > > > > > Hi Jorge, > > > > > > checkpatch.pl finds some issues on your patches. > > > Please make sure checkpath.pl --strict is happy. > > > > > I wasn't aware of the '--strict' parameter. It is not part of the > > help information for checkpath.pl tool. > > Nonetheless, I will use it forward. > > Thanks > > It's an alias to --subjective. But indeed, it's hard to see in the help > output. Thanks > > > > On 2023-04-20 11:54:42-0500, Jorge Lopez wrote: > > > > --- > > > > Based on the latest platform-drivers-x86.git/for-next > > > No need to initialize auth_token_choice and start. > > > Consider coalescing variable declaration to avoid wasting vertical > > > space. > > > > > Done! > > Please note that this affects many parts of the driver, > try to fix it everywhere. It will be done across all files > > > > > +{ > > > > + struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL }; > > > > + struct bios_return *bios_return; > > > > + union acpi_object *obj = NULL; > > > > + struct bios_args *args = NULL; > > > > + int mid, actual_outsize; > > > > + size_t bios_args_size; > > > > + int ret; > > > > + > > > > + mid = encode_outsize_for_pvsz(outsize); > > > > + if (WARN_ON(mid < 0)) > > > > + return mid; > > > > + > > > > + bios_args_size = struct_size(args, data, insize); > > > > + args = kmalloc(bios_args_size, GFP_KERNEL); > > > > + if (!args) > > > > + return -ENOMEM; > > > > + > > > > + input.length = bios_args_size; > > > > + input.pointer = args; > > > > + > > > > + args->signature = 0x55434553; > > > > > > What does this number mean? > > This is a HEX representation of the word 'SECU' expected by BIOS as a signa. > > Sounds like a good thing to comment or put into a #define. I will add a comment since it is only used here. > > > > > > > > + args->command = command; > > > > + args->commandtype = query; > > > > + args->datasize = insize; > > > > + memcpy(args->data, buffer, flex_array_size(args, data, insize)); > > > > + > > > > + ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output); > > > > > > The driver is mixing calls to the UUID based APIs and the wmi_device > > > ones. > > > wmi_devices is newer and preferred. > > > > The driver calls wmi_evaluate_method when initiating an WMI call. > > Where is the driver mixing calls to the UUID based APIs and the > > wmi_device one? > > WMI calls are made by calling hp_wmi_perform_query() which invokes > > wmi_evaluate_method(). > > Did I miss something? > > wmi_evaluate_method() is UUID-based. > struct wmi_driver is wmi_device based. > > The wmi_driver/wmi_device code essentially does nothing and is only used > to validate that a device is present. > The same can be done more easily wmi_has_guid(). > Thank you for the clarification. > > > > > > > + bioscfg_wmi_error_and_message(ret); > > > > + > > > > + if (ret) > > > > + goto out_free; > > > > + > > > > + obj = output.pointer; > > > > + if (!obj) { > > > > + ret = -EINVAL; > > > > + goto out_free; > > > > + } > > > > + if (query != HPWMI_SECUREPLATFORM_GET_STATE && > > > > + command != HPWMI_SECUREPLATFORM) > > > > + if (obj->type != ACPI_TYPE_BUFFER || > > > > + obj->buffer.length < sizeof(*bios_return)) { > > > > + pr_warn("query 0x%x returned wrong type or too small buffer\n", query); > > > > + ret = -EINVAL; > > > > + goto out_free; > > > > + } > > > > + > > > > + > > > > + bios_return = (struct bios_return *)obj->buffer.pointer; > > > > > > For query == HPWMI_SECUREPLATFORM_GET_STATE && command == HPWMI_SECUREPLATFORM > > > this is not guaranteed to be a buffer. > > > > BIOS ensures the output is of buffer type and buffer of 1024 bytes in > > size. This check also help us validate that BIOS only returns a > > buffer type for this query/command type. > > The kernel does not trust the BIOS :-) > It trusts nothing and nobody. > > All cases should be validated. Additional validation will be added to cover all cases. > > > > > > > > + */ > > > > +void *ascii_to_utf16_unicode(u16 *p, const u8 *str) > > > > +{ > > > > + int len = strlen(str); > > > > + int ret; > > > > + > > > > + /* > > > > + * Add null character when reading an empty string > > > > + * "02 00 00 00" > > > > + */ > > > > + if (len == 0) > > > > + return utf16_empty_string(p); > > > > + > > > > + /* Move pointer len * 2 number of bytes */ > > > > + *p++ = len * 2; > > > > + ret = utf8s_to_utf16s(str, strlen(str), UTF16_HOST_ENDIAN, p, len); > > > > + if (ret < 0) { > > > > + dev_err(bioscfg_drv.class_dev, "UTF16 conversion failed\n"); > > > > + goto ascii_to_utf16_unicode_out; > > > > + } > > > > > > What if ret != len ? > > > > only in conditions where utf8s_to_utf16s an error, we can state ret != len. > > ret == len when utf8s_to_utf16s() is successful. > > > > > > > + > > > > + if ((ret * sizeof(u16)) > U16_MAX) { > > > > + dev_err(bioscfg_drv.class_dev, "Error string too long\n"); > > > > + goto ascii_to_utf16_unicode_out; > > > > + } > > > > + > > > > +ascii_to_utf16_unicode_out: > > > > + p += len; > > > > > > In cases of errors this will still point to the end of the data that > > > should have been written but was not actually written. > > > The caller has no way to recognize the error case. > > > > > That is correct. If an error occurs, we only provide an error message > > for those conditions. > > But the caller has no idea that this error occurred and will try to use > the garbage buffer. > The error should be communicated to the caller, and the caller has to > validate the result. > Maybe return NULL? returning NULL will be a good option so I will review what the impact will be across the driver > > > > > > > + return p; > > > > +} > > > > + > > > > +/* > > > > > > kernel-doc comments start with "/**". Note the two asterisks. > > Done > > This also needs to be done all over the driver. It will be done across all files.