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
