Re: [PATCH 2/2] platform: x86: add ACPI driver for ChromeOS.

[Enric Balletbo Serra]

Hi Andy,

2017-08-01 17:17 GMT+02:00 Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:
> On Mon, Jul 31, 2017 at 4:03 PM, Enric Balletbo i Serra
> <enric.balletbo@xxxxxxxxxxxxx> wrote:
>
>> This driver attaches to the Chromeos ACPI device and the exports the values
>> reported by the ACPI in a sysfs directory. All ACPI values are presented in
>> the string form (numbers as decimal values) and can be accessed as the
>> contents of the appropriate read only files in the sysfs directory tree
>> originating in /sys/devices/platform/chromeos_acpi.
>
> Besides what Olof mentioned in reply to cover letter this patch
> requires more clean up work.
>
> I would suggest to look how to make code looks better. Try to get what
> is done under lib/, what new %p extensions are and so on.
> Also some minor typos (comments says -1, function returns -ERRNO).
>
> Few comments below. (Not a complete review)
>
> (Tired to look to this. For PDx86 the quality of this beyond good)
>
>> @@ -0,0 +1,792 @@
>> +/*
>> + * chromeos_acpi.c - ChromeOS specific ACPI support
>
> No need to put filename here. It makes more useless effort in the
> future if, for example, file would be renamed.
>
>> + *
>
>> +static struct chromeos_acpi_dev chromeos_acpi = { };
>
> {} is redundant for global static variables.
>
>> +/*
>
> /** ?
>
> If so, read how to create a correct looking kernel doc descriptions.
>
> Otherwise "@flag - " part (if you not imply kernel doc) looks a bit confusing.
>
>> + * This function operates on legacy BIOSes which do not export VBNV element
>> + * through ACPI. These BIOSes use a fixed location in NVRAM to contain a
>> + * bitmask of known flags.
>> + *
>> + * @flag - the bitmask to set, it is the responsibility of the caller to set
>> + *         the proper bits.
>> + *
>> + * returns 0 on success (is running in legacy mode and chnv is initialized) or
>
>> + *         -1 otherwise.
>
> Not true.
>
>> + */
>
>
>> +static int chromeos_set_nvram_flag(u8 flag)
>> +{
>
>> +       u8 cur;
>> +       unsigned int index = chromeos_acpi_if_data.chnv.cad_value;
>
> Reveresed X-mas tree order.
>
>> +
>> +       if (!chromeos_on_legacy_firmware())
>> +               return -ENODEV;
>
> ^^^^
>
>> +
>> +       cur = nvram_read_byte(index);
>> +
>
>> +       if ((cur & flag) != flag)
>> +               nvram_write_byte(cur | flag, index);
>
> This looks suspicious. Is flag always a one bit set?
>
>> +
>> +       return 0;
>> +}
>
>> +/*
>> + * Read the nvram buffer contents into the user provided space.
>> + *
>> + * returns the number of bytes copied,
>
>> or -1 on any error.
>
> Not true!
>
>> + */
>
>> +static ssize_t chromeos_vbc_nvram_read(void *buf, size_t count)
>> +{
>> +       int base, size, i;
>> +
>> +       if (!chromeos_acpi_if_data.nv_base.cad_is_set ||
>> +           !chromeos_acpi_if_data.nv_size.cad_is_set) {
>> +               pr_err("NVRAM not configured\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       base = chromeos_acpi_if_data.nv_base.cad_value;
>> +       size = chromeos_acpi_if_data.nv_size.cad_value;
>> +
>> +       if (count < size) {
>> +               pr_err("not enough room to read nvram (%zd < %d)\n",
>> +                      count, size);
>> +               return -EINVAL;
>> +       }
>> +
>
>> +       for (i = 0; i < size; i++)
>> +               ((u8 *)buf)[i] = nvram_read_byte(base++);
>
> Perhaps provide
> nvram_read_array() ?
>
>> +
>> +       return size;
>> +}
>> +
>> +static ssize_t chromeos_vbc_nvram_write(const void *buf, size_t count)
>> +{
>> +       unsigned int base, size, i;
>> +
>> +       if (!chromeos_acpi_if_data.nv_base.cad_is_set ||
>> +           !chromeos_acpi_if_data.nv_size.cad_is_set) {
>> +               pr_err("NVRAM not configured\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       size = chromeos_acpi_if_data.nv_size.cad_value;
>> +       base = chromeos_acpi_if_data.nv_base.cad_value;
>> +
>> +       if (count != size) {
>> +               pr_err("wrong buffer size (%zd != %d)\n", count, size);
>> +               return -EINVAL;
>> +       }
>> +
>
>> +       for (i = 0; i < size; i++) {
>> +               u8 c;
>> +
>> +               c = nvram_read_byte(base + i);
>> +               if (c == ((u8 *)buf)[i])
>> +                       continue;
>> +               nvram_write_byte(((u8 *)buf)[i], base + i);
>> +       }
>
> nvram_write_array() ?
>
>> +
>> +       return size;
>> +}
>> +
>> +/*
>> + * To show attribute value just access the container structure's `value'
>> + * field.
>> + */
>> +static ssize_t show_acpi_attribute(struct device *dev,
>> +                                  struct device_attribute *attr, char *buf)
>> +{
>> +       struct acpi_attribute *paa;
>> +
>> +       paa = container_of(attr, struct acpi_attribute, dev_attr);
>> +
>
>> +       return snprintf(buf, PAGE_SIZE, paa->value);
>
> sprintf();
>
> How did you test this? Where is the format string?
>
>> +}
>
>> +/*
>
> /** ?
>
>> + * create_sysfs_attribute() create and initialize an ACPI sys fs attribute
>> + *                         structure.
>> + * @value: attribute value
>
>> + */
>
>> +static struct acpi_attribute *create_sysfs_attribute(char *value, char *name,
>
> Function name needs prefix, otherwise too broad.
>
>> +                                                    int count, int instance)
>
>> +{
>> +       struct acpi_attribute *paa;
>> +       int total_size, room_left;
>> +       int value_len = strlen(value);
>> +
>> +       if (!value_len)
>> +               return NULL;
>> +
>
>> +       value_len++; /* include the terminating zero */
>
>
>
>> +       total_size = value_len + sizeof(struct acpi_attribute) +
>> +                       strlen(name) + 1;
>
> One line?
>
>> +
>> +       if (count != 1) {
>
>> +               if (count >= 1000) {
>> +                       pr_err("too many (%d) instances of %s.\n", count, name);
>> +                       return NULL;
>> +               }
>
> Move this outside out the parent condition.
>
>> +               /* allow up to three digits and the dot */
>> +               total_size += 4;
>> +       }
>
>> +
>> +       paa = kzalloc(total_size, GFP_KERNEL);
>> +       if (!paa)
>> +               return NULL;
>> +
>> +       sysfs_attr_init(&paa->dev_attr.attr);
>> +       paa->dev_attr.attr.mode = 0444;  /* read only */
>> +       paa->dev_attr.show = show_acpi_attribute;
>
>> +       paa->value = (char *)(paa + 1);
>
>> +       strcpy(paa->value, value);
>> +       paa->dev_attr.attr.name = paa->value + value_len;
>> +
>> +       room_left = total_size - value_len -
>> +                       offsetof(struct acpi_attribute, value);
>> +
>
>> +       if (count == 1) {
>> +               snprintf((char *)paa->dev_attr.attr.name, room_left, name);
>
> "%s", name
>
>> +       } else {
>> +               snprintf((char *)paa->dev_attr.attr.name, room_left,
>> +                        "%s.%d", name, instance);
>> +       }
>> +
>> +       paa->next_acpi_attr = chromeos_acpi.attributes;
>> +       chromeos_acpi.attributes = paa;
>> +
>> +       return paa;
>> +}
>
>> +/*
>
> /** ?
>
>> + * add_sysfs_attribute() create and initialize an ACPI sys fs attribute
>> + *                         structure and create the attribute.
>> + * @value: attribute value
>
>> + */
>
>> +static void handle_nested_acpi_package(union acpi_object *po, char *pm,
>> +                                      int total, int instance)
>> +{
>> +       int i, size, count, j;
>> +       struct acpi_attribute_group *aag;
>> +
>> +       count = po->package.count;
>> +
>> +       size = strlen(pm) + 1 + sizeof(struct acpi_attribute_group) +
>> +           sizeof(struct attribute *) * (count + 1);
>> +
>> +       if (total != 1) {
>
>> +               if (total >= 1000) {
>> +                       pr_err("too many (%d) instances of %s\n", total, pm);
>> +                       return;
>> +               }
>
> Move it out.
>
>> +               /* allow up to three digits and the dot */
>> +               size += 4;
>> +       }
>
>> +       aag->next_acpi_attr_group = chromeos_acpi.groups;
>> +       chromeos_acpi.groups = aag->next_acpi_attr_group;
>
>> +       aag->ag.attrs = (struct attribute **)(aag + 1);
>
>> +       aag->ag.name = (const char *)&aag->ag.attrs[count + 1];
>> +
>> +       /* room left in the buffer */
>> +       size = size - (aag->ag.name - (char *)aag);
>
>> +       if (total != 1)
>
> Perhaps
>
> if (total == 1)
>
> to be consistent with above code for some other stuff?
>
>> +               snprintf((char *)aag->ag.name, size, "%s.%d", pm, instance);
>> +       else
>> +               snprintf((char *)aag->ag.name, size, "%s", pm);
>
>> +       j = 0;                  /* attribute index */
>> +       for (i = 0; i < count; i++) {
>> +               union acpi_object *element = po->package.elements + i;
>
>> +               int copy_size = 0;
>
> Redundant assignment.
>
>> +               char attr_value[40];    /* 40 chars be enough for names */
>> +               struct acpi_attribute *paa;
>> +
>> +               switch (element->type) {
>> +               case ACPI_TYPE_INTEGER:
>> +                       copy_size = snprintf(attr_value, sizeof(attr_value),
>> +                                            "%d", (int)element->integer.value);
>> +                       paa = create_sysfs_attribute(attr_value, pm, count, i);
>> +                       break;
>> +
>> +               case ACPI_TYPE_STRING:
>
>> +                       copy_size = min(element->string.length,
>> +                                       (u32)(sizeof(attr_value)) - 1);
>> +                       memcpy(attr_value, element->string.pointer, copy_size);
>> +                       attr_value[copy_size] = '\0';
>
> What the problem to supply atrribute value and its length?
>
>> +                       paa = create_sysfs_attribute(attr_value, pm, count, i);
>> +                       break;
>> +
>> +               default:
>> +                       pr_err("ignoring nested type %d\n", element->type);
>> +                       continue;
>> +               }
>> +               aag->ag.attrs[j++] = &paa->dev_attr.attr;
>> +       }
>> +
>> +       if (sysfs_create_group(&chromeos_acpi.p_dev->dev.kobj, &aag->ag))
>> +               pr_err("failed to create group %s.%d\n", pm, instance);
>> +}
>
>> +static void maybe_export_acpi_int(const char *pm, int index,
>> +                                 unsigned int value)
>> +{
>> +       int i;
>
>
>> +       struct chromeos_acpi_exported_ints {
>> +               const char *acpi_name;
>> +               int acpi_index;
>> +               struct chromeos_acpi_datum *cad;
>> +       } exported_ints[] = {
>> +               { "VBNV", 0, &chromeos_acpi_if_data.nv_base },
>> +               { "VBNV", 1, &chromeos_acpi_if_data.nv_size },
>> +               { "CHSW", 0, &chromeos_acpi_if_data.switch_state },
>> +               { "CHNV", 0, &chromeos_acpi_if_data.chnv }
>> +       };
>
> Shouldn't be outside and marked as static const ?
>
>> +
>> +       for (i = 0; i < ARRAY_SIZE(exported_ints); i++) {
>> +               struct chromeos_acpi_exported_ints *exported_int;
>> +
>
>> +               exported_int = exported_ints + i;
>
> &exported_ints[i]; ?
>
>> +
>> +               if (!strncmp(pm, exported_int->acpi_name, 4) &&
>> +                   (exported_int->acpi_index == index)) {
>> +                       pr_notice("registering %s %d\n", pm, index);
>> +                       exported_int->cad->cad_value = value;
>> +                       exported_int->cad->cad_is_set = true;
>> +                       return;
>> +               }
>> +       }
>> +}
>
>> +static char *acpi_buffer_to_string(union acpi_object *element)
>
> C'mon, acpi_ is for ACPI glue layer, not for some chrome custom stuff.
>
>> +{
>> +       char *base, *p;
>> +       int i;
>> +       unsigned int room_left;
>> +       /* Include this many characters per line */
>> +       unsigned int char_per_line = 16;
>> +       unsigned int blob_size;
>> +       unsigned int string_buffer_size;
>
> Reversed X-mas tree.
>
>> +
>> +       /*
>> +        * As of now the VDAT structure can supply as much as 3700 bytes. When
>> +        * expressed as a hex dump it becomes 3700 * 3 + 3700/16 + .. which
>> +        * clearly exceeds the maximum allowed sys fs buffer size of one page
>> +        * (4k).
>> +        *
>> +        * What this means is that we can't keep the entire blob in one sysfs
>> +        * file. Currently verified boot (the consumer of the VDAT contents)
>> +        * does not care about the most of the data, so as a quick fix we will
>> +        * truncate it here. Once the blob data beyond the 4K boundary is
>> +        * required this approach will have to be reworked.
>> +        *
>> +        * TODO: Split the data into multiple VDAT instances, each
>> +        * not exceeding 4K or consider exporting as a binary using
>> +        * sysfs_create_bin_file().
>> +        */
>> +
>> +       /*
>> +        * X, the maximum number of bytes which will fit into a sysfs file
>> +        * (one memory page) can be derived from the following equation (where
>> +        * N is number of bytes included in every hex string):
>> +        *
>> +        * 3X + X/N + 4 <= PAGE_SIZE.
>> +        *
>> +        * Solving this for X gives the following
>> +        */
>> +       blob_size = ((PAGE_SIZE - 4) * char_per_line) / (char_per_line * 3 + 1);
>
> Oy vey!
>
>> +
>> +       if (element->buffer.length > blob_size)
>> +               pr_info("truncating buffer from %d to %d\n",
>> +                       element->buffer.length, blob_size);
>> +       else
>> +               blob_size = element->buffer.length;
>> +
>> +       /*
>> +        * Three characters to display one byte, one newline per line, all
>> +        * rounded up, plus extra newline in the end, plus terminating
>> +        * zero, hence + 4
>> +        */
>> +       string_buffer_size = blob_size * 3 + blob_size / char_per_line + 4;
>> +
>> +       p = kzalloc(string_buffer_size, GFP_KERNEL);
>> +       if (!p)
>> +               return NULL;
>> +
>> +       base = p;
>> +       room_left = string_buffer_size;
>> +       for (i = 0; i < blob_size; i++) {
>> +               int printed;
>> +
>> +               printed = snprintf(p, room_left, " %2.2x",
>> +                                  element->buffer.pointer[i]);
>> +               room_left -= printed;
>> +               p += printed;
>> +               if (((i + 1) % char_per_line) == 0) {
>> +                       if (!room_left)
>> +                               break;
>> +                       room_left--;
>> +                       *p++ = '\n';
>> +               }
>> +       }
>> +       if (room_left < 2) {
>> +               pr_err("no room in the buffer\n");
>> +               *p = '\0';
>> +       } else {
>> +               *p++ = '\n';
>> +               *p++ = '\0';
>> +       }
>
> hex_dump_to_buffer();
> And remove this custom crap.
>
>> +       return base;
>> +}
>
>> +static void handle_acpi_package(union acpi_object *po, char *pm)
>> +{
>> +       int j;
>> +       int count = po->package.count;
>> +
>> +       for (j = 0; j < count; j++) {
>> +               union acpi_object *element = po->package.elements + j;
>> +               int copy_size = 0;
>> +               char attr_value[256];   /* strings could be this long */
>> +
>> +               switch (element->type) {
>> +               case ACPI_TYPE_INTEGER:
>> +                       copy_size = snprintf(attr_value, sizeof(attr_value),
>> +                                            "%d", (int)element->integer.value);
>> +                       add_sysfs_attribute(attr_value, pm, count, j);
>> +                       maybe_export_acpi_int(pm, j, (unsigned int)
>> +                                             element->integer.value);
>> +                       break;
>> +
>> +               case ACPI_TYPE_STRING:
>> +                       copy_size = min(element->string.length,
>> +                                       (u32)(sizeof(attr_value)) - 1);
>> +                       memcpy(attr_value, element->string.pointer, copy_size);
>> +                       attr_value[copy_size] = '\0';
>> +                       add_sysfs_attribute(attr_value, pm, count, j);
>> +                       break;
>> +
>> +               case ACPI_TYPE_BUFFER: {
>> +                       char *buf_str;
>> +
>> +                       buf_str = acpi_buffer_to_string(element);
>> +                       if (buf_str) {
>> +                               add_sysfs_attribute(buf_str, pm, count, j);
>> +                               kfree(buf_str);
>> +                       }
>> +                       break;
>> +               }
>> +               case ACPI_TYPE_PACKAGE:
>> +                       handle_nested_acpi_package(element, pm, count, j);
>> +                       break;
>> +
>> +               default:
>> +                       pr_err("ignoring type %d (%s)\n", element->type, pm);
>> +                       break;
>> +               }
>> +       }
>> +}
>> +
>> +/*
>> + * add_acpi_method() evaluate an ACPI method and create sysfs attributes.
>> + *
>> + * @device: ACPI device
>> + * @pm: name of the method to evaluate
>> + */
>> +static void add_acpi_method(struct acpi_device *device, char *pm)
>> +{
>> +       acpi_status status;
>> +       struct acpi_buffer output;
>> +       union acpi_object *po;
>> +
>> +       output.length = ACPI_ALLOCATE_BUFFER;
>> +       output.pointer = NULL;
>> +
>> +       status = acpi_evaluate_object(device->handle, pm, NULL, &output);
>> +
>> +       if (!ACPI_SUCCESS(status)) {
>> +               pr_err("failed to retrieve %s (%d)\n", pm, status);
>> +               return;
>> +       }
>> +
>> +       po = output.pointer;
>> +
>> +       if (po->type != ACPI_TYPE_PACKAGE)
>> +               pr_err("%s is not a package, ignored\n", pm);
>> +       else
>> +               handle_acpi_package(po, pm);
>> +
>> +       kfree(output.pointer);
>> +}
>> +
>
>> +static int chromeos_process_mlst(struct acpi_device *device)
>> +{
>> +       acpi_status status;
>> +       struct acpi_buffer output;
>> +       union acpi_object *po;
>> +       int j;
>> +
>> +       output.length = ACPI_ALLOCATE_BUFFER;
>> +       output.pointer = NULL;
>> +
>> +       status = acpi_evaluate_object(device->handle, MLST_METHOD, NULL,
>> +                                     &output);
>
>> +       if (!ACPI_SUCCESS(status)) {
>
> ACPI_FAILURE()
>
>> +               pr_debug("failed to retrieve MLST (%d)\n", status);
>
>> +               return 1;
>
> -ENOENT?
>
>> +       }
>
>> +       for (j = 0; j < po->package.count; j++) {
>> +               union acpi_object *element = po->package.elements + j;
>
>> +               int copy_size = 0;
>
> Redundant assignment
>
>> +               char method[ACPI_NAME_SIZE + 1];
>> +
>> +               if (element->type == ACPI_TYPE_STRING) {
>
>> +                       copy_size = min_t(u32, element->string.length,
>> +                                         ACPI_NAME_SIZE);
>> +                       memcpy(method, element->string.pointer, copy_size);
>> +                       method[copy_size] = '\0';
>
> I'm not sure I understand why do you need a copy (same to the previous
> similar piece(s))?
> Can't you supply method name + length?
>
>> +                       add_acpi_method(device, method);
>
> Btw, name is too broad.
>
>> +               }
>> +       }
>> +
>> +       kfree(output.pointer);
>> +
>> +       return 0;
>> +}
>
>> +static int chromeos_acpi_remove(struct acpi_device *device)
>> +{
>> +       return 0;
>> +}
>
> Redundant.
>
>> +static struct acpi_driver chromeos_acpi_driver = {
>> +       .name   = "ChromeOS ACPI driver",
>
>> +       .owner  = THIS_MODULE,
>
> Do we need this?
>
>> +       .ids    = chromeos_device_ids,
>> +       .ops    = {
>> +               .add            = chromeos_acpi_add,
>
>> +               .remove         = chromeos_acpi_remove,
>
> Redundant.
>
>> +       },
>> +};
>
>> +static int __init chromeos_acpi_init(void)
>> +{
>
>> +       int ret = 0;
>> +       acpi_status status;
>
> Should be
>
>       acpi_status status;
>       int ret;
>
>> +       chromeos_acpi.p_dev = platform_device_register_simple("chromeos_acpi",
>> +                                                             -1, NULL, 0);
>
> -1 has a definition.
>
>> +       if (IS_ERR(chromeos_acpi.p_dev)) {
>> +               pr_err("unable to register platform device\n");
>> +               return PTR_ERR(chromeos_acpi.p_dev);
>> +       }
>> +
>> +       ret = acpi_bus_register_driver(&chromeos_acpi_driver);
>> +       if (ret < 0) {
>> +               pr_err("failed to register driver (%d)\n", ret);
>> +
>> +               platform_device_unregister(chromeos_acpi.p_dev);
>> +               chromeos_acpi.p_dev = NULL;
>> +               return ret;
>> +       }
>> +
>
>> +       pr_info("installed%s\n", chromeos_on_legacy_firmware() ?strscpy(); ?
>
>> +               " (legacy mode)" : "");
>
>> +
>> +       pr_info("enabling S3 USB wake\n");
>> +       status = acpi_evaluate_object(NULL, "\\S3UE", NULL, NULL);
>
>> +       if (!ACPI_SUCCESS(status))
>
> ACPI_FAILURE()
>
>> +               pr_info("failed to enable S3 USB wake\n");
>> +
>> +       return 0;
>> +}
>
> --
> With Best Regards,
> Andy Shevchenko

I'll do the cleanups you suggested, look under lib to catch more
cleanups and send a second version. Many thanks for the feedback and
your patience.

Regards,
 Enric