Hi,
Thanks for reviewing.
>> + switch (element->type) {
>> + case ACPI_TYPE_BUFFER:
>> + length = element->buffer.length;
>> + info->data = kmemdup(element->buffer.pointer,
>> + length, GFP_KERNEL);
>> + break;
>> + case ACPI_TYPE_INTEGER:
>> + length = snprintf(buffer, sizeof(buffer), "%d",
>> + (int)element->integer.value);
>> + info->data = kmemdup(buffer, length, GFP_KERNEL);
>
> You can use `kasprintf()` here, no?
>
Choosing kmemdup vs k*printf depends on what is being achieved. Usage of
kmemdup indicates that only the memory is being duplicated here. While
in case of k*printf, some transformation is done. Thus in normal memory
duplication cases like this, the usage of kmemdup makes code more
readable and seems preferable to me.
>> +static int chromeos_acpi_handle_package(struct platform_device *pdev,
>> + union acpi_object *obj, char *name)
>> +{
>> + struct device *dev = &pdev->dev;
>> + int count = obj->package.count;
>> + union acpi_object *element;
>> + int i, ret;
>> +
>> + element = obj->package.elements;
>> + for (i = 0; i < count; i++, element++) {
>> + if (element->type == ACPI_TYPE_BUFFER ||
>> + element->type == ACPI_TYPE_STRING ||
>> + element->type == ACPI_TYPE_INTEGER) {
>> + /* Create a single attribute in the root directory */
>> + ret = chromeos_acpi_add_attr(chromeos_acpi.root,
>> + element, name,
>> + count, i);
>> + if (ret) {
>> + dev_err(dev, "error adding attributes (%d)\n",
>> + ret);
>> + return ret;
>> + }
>> + chromeos_acpi.num_attrs++;
>> + } else if (element->type == ACPI_TYPE_PACKAGE) {
>> + /* Create a group of attributes */
>> + ret = chromeos_acpi_add_group(element, name, count, i);
>> + if (ret) {
>> + dev_err(dev, "error adding a group (%d)\n",
>> + ret);
>> + return ret;
>> + }
>> + } else {
>> + if (ret) {
>
> `ret` can be potentially uninitialized here, no?
>
>
This driver is written in this way that the element->type must be from
these 4 types always. Having a second look, it seems a bit illogical to
check the value of ret if some other element->type happen to be present.
I'll remove this `if (ret)` condition entirely.
>> + dev_err(dev, "error on element type (%d)\n",
>> + ret);
>> + return -EINVAL;
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * chromeos_acpi_add_method() - Evaluate an ACPI method and create sysfs
>> + * attributes
>> + *
>> + * @pdev: Platform device
>> + * @name: Name of the method to evaluate
>> + *
>> + * Return: 0 on success, non-zero on failure
>> + */
>> +static int chromeos_acpi_add_method(struct platform_device *pdev, char *name)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>> + acpi_status status;
>> + int ret = 0;
>> +
>> + status = acpi_evaluate_object(ACPI_COMPANION(&pdev->dev)->handle, name, NULL, &output);
>> + if (ACPI_FAILURE(status)) {
>> + dev_err(dev, "failed to retrieve %s (%d)\n", name, status);
>
> (maybe `acpi_format_exception(status)` would be more meaningful than the numeric value)
>
Yeah, it'll be more better. I'll use this macro.
>
>> + return status;
>
> This return value is potentially propagated to become the return value of
> the probe function. The problem is that it is not a negative errno that the probe
> method should return but rather an ACPI status code.
>
Good catch. I'll return -EINVAL here instead.
>
>> + }
>> +
>> + if (((union acpi_object *)output.pointer)->type == ACPI_TYPE_PACKAGE)
>> + ret = chromeos_acpi_handle_package(pdev, output.pointer, name);
>> +
>> + kfree(output.pointer);
>> + return ret;
>> +}
>> +
>> +/**
>> + * chromeos_acpi_process_mlst() - Evaluate the MLST method and add methods
>> + * listed in the response
>> + *
>> + * @pdev: Platform device
>> + *
>> + * Returns: 0 if successful, non-zero if error.
>> + */
>> +static int chromeos_acpi_process_mlst(struct platform_device *pdev)
>> +{
>> + struct chromeos_acpi_attribute_group *aag;
>> + char name[ACPI_NAMESEG_SIZE + 1];
>> + union acpi_object *element, *obj;
>> + struct device *dev = &pdev->dev;
>> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>> + acpi_status status;
>> + int ret = 0;
>> + int size;
>> + int i;
>> +
>> + status = acpi_evaluate_object(ACPI_COMPANION(&pdev->dev)->handle, MLST, NULL,
>> + &output);
>> + if (ACPI_FAILURE(status))
>> + return status;
>> +
>> + obj = output.pointer;
>> + if (obj->type != ACPI_TYPE_PACKAGE) {
>> + ret = -EINVAL;
>> + goto free_acpi_buffer;
>> + }
>> +
>> + element = obj->package.elements;
>> + for (i = 0; i < obj->package.count; i++, element++) {
>> + if (element->type == ACPI_TYPE_STRING) {
>> + size = min(element->string.length + 1,
>> + (u32)ACPI_NAMESEG_SIZE + 1);
>
> Is truncation a real possibility? Shouldn't it abort/etc. in that case?
> And `min()` "returns" a u32 here but `size` is an `int`.
>
There is not likely, but can still happen. We want to not abort and
carry on with truncated string. I'll update the type of size to u32.
>
>> + strscpy(name, element->string.pointer, size);
>> + ret = chromeos_acpi_add_method(pdev, name);
>> + if (ret) {
>> + chromeos_acpi_remove_groups();
>> + break;
>
> Is just a `break` is enough here to handle the error? If this is not fatal,
> then why is a `dev_warn()` not sufficient? If this is fatal, why continue
> with the rest of the function?
>
I'll have a look.
>
> Excuse me if I have missed previous discussions about it, but I am confused by
> the design. Why is a global variable needed here? The global struct's members
> are overwritten in the probe method in any case.
>
The global variable is needed in probe and remove functions. Researching
this more, I think dev->driver_data can be used instead of global
variable. I'll test it. Thanks for mentioning it.
> And checkpatch reports that no MAINTAINERS entry has been added for the new file.
> (And it appears to be right if I have not missed anything.)
>
I thought there will be default maintainer of this directory. But there
isn't. I'll send a separate email to discuss this.
>
> Regards,
> Barnabás Pőcze
--
Muhammad Usama Anjum
