Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 1 Aug 2017 18:17:09 +0300 | Subject | Re: [PATCH 2/2] platform: x86: add ACPI driver for ChromeOS. |
| |
On Mon, Jul 31, 2017 at 4:03 PM, Enric Balletbo i Serra <enric.balletbo@collabora.com> 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
| |