Looks OK, just some minor comments below.
On Thu, 03 Nov, at 11:18:48AM, Lukas Wunner wrote:
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index cc69e37..ff01637 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -537,6 +537,63 @@ static void setup_efi_pci(struct boot_params *params)
> efi_call_early(free_pool, pci_handle);
> }
>
> +static void retrieve_apple_device_properties(struct boot_params *params)
> +{
> + efi_guid_t guid = APPLE_PROPERTIES_PROTOCOL_GUID;
> + struct setup_data *data, *new;
> + efi_status_t status;
> + void *properties;
> + u32 size = 0;
> +
> + status = efi_call_early(locate_protocol, &guid, NULL, &properties);
> + if (status != EFI_SUCCESS)
> + return;
> +
> + if ((efi_is_64bit() ?
> + ((apple_properties_protocol_64 *)properties)->version :
> + ((apple_properties_protocol_32 *)properties)->version)
> + != 0x10000) {
> + efi_printk(sys_table, "Unsupported properties proto version\n");
> + return;
> + }
It's a minor point, but please don't write the 32/64 code like this.
Either use two separate functions, like __file_size32() and
__file_size64(), or if that's two much code duplication stash the
version field and get_all pointer in stack-local variables at the
start of the function.
> + efi_early->call(efi_is_64bit() ?
> + ((apple_properties_protocol_64 *)properties)->get_all :
> + ((apple_properties_protocol_32 *)properties)->get_all,
> + properties, NULL, &size);
> + if (!size)
> + return;
> +
> + do {
> + status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> + size + sizeof(struct setup_data), &new);
> + if (status != EFI_SUCCESS) {
> + efi_printk(sys_table,
> + "Failed to alloc mem for properties\n");
> + return;
> + }
Newline here please.
> + status = efi_early->call(efi_is_64bit() ?
> + ((apple_properties_protocol_64 *)properties)->get_all :
> + ((apple_properties_protocol_32 *)properties)->get_all,
> + properties, new->data, &size);
And a newline here.
> + if (status == EFI_BUFFER_TOO_SMALL)
> + efi_call_early(free_pool, new);
> + } while (status == EFI_BUFFER_TOO_SMALL);
> +
> + new->type = SETUP_APPLE_PROPERTIES;
> + new->len = size;
> + new->next = 0;
> +
> + data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
> + if (!data)
> + params->hdr.setup_data = (unsigned long)new;
> + else {
> + while (data->next)
> + data = (struct setup_data *)(unsigned long)data->next;
> + data->next = (unsigned long)new;
> + }
> +}
> +
> static efi_status_t
> setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
> {
> @@ -1068,6 +1125,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
> struct boot_params *efi_main(struct efi_config *c,
> struct boot_params *boot_params)
> {
> + efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> struct desc_ptr *gdt = NULL;
> efi_loaded_image_t *image;
> struct setup_header *hdr = &boot_params->hdr;
> @@ -1098,6 +1156,11 @@ struct boot_params *efi_main(struct efi_config *c,
>
> setup_efi_pci(boot_params);
>
> + if (!memcmp((void *)sys_table->fw_vendor, apple, sizeof(apple))) {
> + if (IS_ENABLED(CONFIG_APPLE_PROPERTIES))
> + retrieve_apple_device_properties(boot_params);
> + }
> +
> status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> sizeof(*gdt), (void **)&gdt);
> if (status != EFI_SUCCESS) {
Could you split this off into a setup_apple_properties() function to
mirror setup_efi_pci()?
> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> index c18ce67..b10bf31 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -7,6 +7,7 @@
> #define SETUP_DTB 2
> #define SETUP_PCI 3
> #define SETUP_EFI 4
> +#define SETUP_APPLE_PROPERTIES 5
>
> /* ram_size flags */
> #define RAMDISK_IMAGE_START_MASK 0x07FF
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 893fda4..2e78b0b 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -129,6 +129,19 @@ config EFI_TEST
> Say Y here to enable the runtime services support via /dev/efi_test.
> If unsure, say N.
>
> +config APPLE_PROPERTIES
> + bool "Apple Device Properties"
> + depends on EFI_STUB && X86
> + select EFI_DEV_PATH_PARSER
> + select UCS2_STRING
> + help
> + Retrieve properties from EFI on Apple Macs and assign them to
> + devices, allowing for improved support of Apple hardware.
> + Properties that would otherwise be missing include the
> + Thunderbolt Device ROM and GPU configuration data.
> +
> + If unsure, say Y if you have a Mac. Otherwise N.
> +
> endmenu
>
> config UEFI_CPER
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 3e91ae3..ad67342 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_EFI_FAKE_MEMMAP) += fake_mem.o
> obj-$(CONFIG_EFI_BOOTLOADER_CONTROL) += efibc.o
> obj-$(CONFIG_EFI_TEST) += test/
> obj-$(CONFIG_EFI_DEV_PATH_PARSER) += dev-path-parser.o
> +obj-$(CONFIG_APPLE_PROPERTIES) += apple-properties.o
>
> arm-obj-$(CONFIG_EFI) := arm-init.o arm-runtime.o
> obj-$(CONFIG_ARM) += $(arm-obj-y)
> diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
> new file mode 100644
> index 0000000..3b4d6a2
> --- /dev/null
> +++ b/drivers/firmware/efi/apple-properties.c
> @@ -0,0 +1,239 @@
> +/*
> + * apple-properties.c - EFI device properties on Macs
> + * Copyright (C) 2016 Lukas Wunner <lukas@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License (version 2) as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#define pr_fmt(fmt) "apple-properties: " fmt
> +
> +#include <linux/bootmem.h>
> +#include <linux/dmi.h>
> +#include <linux/efi.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/ucs2_string.h>
> +#include <asm/setup.h>
> +
> +static bool dump_properties __initdata;
> +
> +static int __init dump_properties_enable(char *arg)
> +{
> + dump_properties = true;
> + return 0;
> +}
> +
> +__setup("dump_apple_properties", dump_properties_enable);
> +
> +struct dev_header {
> + u32 len;
> + u32 prop_count;
> + struct efi_dev_path path[0];
> + /*
> + * followed by key/value pairs, each key and value preceded by u32 len,
> + * len includes itself, value may be empty (in which case its len is 4)
> + */
> +};
> +
> +struct properties_header {
> + u32 len;
> + u32 version;
> + u32 dev_count;
> + struct dev_header dev_header[0];
> +};
> +
> +static u8 one __initdata = 1;
> +
> +static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
> + struct device *dev, void *ptr,
> + struct property_entry entry[])
> +{
> + int i;
> +
> + for (i = 0; i < dev_header->prop_count; i++) {
> + int remaining = dev_header->len - (ptr - (void *)dev_header);
> + u32 key_len, val_len;
> + char *key;
> +
> + if (sizeof(key_len) > remaining)
> + break;
> + key_len = *(typeof(key_len) *)ptr;
> + if (key_len + sizeof(val_len) > remaining ||
> + key_len < sizeof(key_len) + sizeof(efi_char16_t) ||
> + *(efi_char16_t *)(ptr + sizeof(key_len)) == 0) {
> + dev_err(dev, "invalid property name len at %#zx\n",
> + ptr - (void *)dev_header);
> + break;
> + }
> + val_len = *(typeof(val_len) *)(ptr + key_len);
> + if (key_len + val_len > remaining ||
> + val_len < sizeof(val_len)) {
> + dev_err(dev, "invalid property val len at %#zx\n",
> + ptr - (void *)dev_header + key_len);
> + break;
> + }
Please sprinkle a few newlines to make this more readable.
> +
> + key = kzalloc((key_len - sizeof(key_len)) * 4 + 1, GFP_KERNEL);
> + if (!key) {
> + dev_err(dev, "cannot allocate property name\n");
> + break;
> + }
Could you add a comment to document the kzalloc() size argument above?
> + ucs2_as_utf8(key, ptr + sizeof(key_len),
> + key_len - sizeof(key_len));
> +
> + entry[i].name = key;
> + entry[i].is_array = true;
> + entry[i].length = val_len - sizeof(val_len);
> + entry[i].pointer.raw_data = ptr + key_len + sizeof(val_len);
> + if (!entry[i].length) {
> + /* driver core doesn't accept empty properties */
> + entry[i].length = 1;
> + entry[i].pointer.raw_data = &one;
> + }
> +
> + if (dump_properties) {
> + dev_info(dev, "property: %s\n", entry[i].name);
> + print_hex_dump(KERN_INFO, pr_fmt(), DUMP_PREFIX_OFFSET,
> + 16, 1, entry[i].pointer.raw_data,
> + entry[i].length, true);
> + }
> +
> + ptr += key_len + val_len;
> + }
> +
> + if (i != dev_header->prop_count) {
> + dev_err(dev, "got %d device properties, expected %u\n", i,
> + dev_header->prop_count);
> + print_hex_dump(KERN_ERR, pr_fmt(), DUMP_PREFIX_OFFSET,
> + 16, 1, dev_header, dev_header->len, true);
> + } else
> + dev_info(dev, "assigning %d device properties\n", i);
Mismatched braces are frowned upon in the CodingStyle guide. Please
use braces for both the 'if' and 'else'. Alternatively, return early
for one of the conditions (and save a level of indentation too).
> +}
> +
> +static int __init unmarshal_devices(struct properties_header *properties)
> +{
> + size_t offset = offsetof(struct properties_header, dev_header[0]);
> +
> + while (offset + sizeof(struct dev_header) < properties->len) {
> + struct dev_header *dev_header = (void *)properties + offset;
> + struct property_entry *entry = NULL;
> + struct device *dev;
> + size_t len;
> + int ret, i;
> + void *ptr;
> +
> + if (offset + dev_header->len > properties->len ||
> + dev_header->len <= sizeof(*dev_header)) {
> + pr_err("invalid len in dev_header at %#zx\n", offset);
> + return -EINVAL;
> + }
> +
> + ptr = dev_header->path;
> + len = dev_header->len - sizeof(*dev_header);
Newline please.
> + dev = efi_get_device_by_path((struct efi_dev_path **)&ptr, &len);
> + if (IS_ERR(dev)) {
> + pr_err("device path parse error %ld at %#zx:\n",
> + PTR_ERR(dev), ptr - (void *)dev_header);
> + print_hex_dump(KERN_ERR, pr_fmt(), DUMP_PREFIX_OFFSET,
> + 16, 1, dev_header, dev_header->len, true);
> + dev = NULL;
> + goto skip_device;
> + }
> +
> + entry = kcalloc(dev_header->prop_count + 1, sizeof(*entry),
> + GFP_KERNEL);
> + if (!entry) {
> + dev_err(dev, "cannot allocate properties\n");
> + goto skip_device;
> + }
> +
> + unmarshal_key_value_pairs(dev_header, dev, ptr, entry);
> + if (!entry[0].name)
> + goto skip_device;
> +
> + ret = device_add_properties(dev, entry); /* makes deep copy */
> + if (ret)
> + dev_err(dev, "error %d assigning properties\n", ret);
Newline.
> + for (i = 0; entry[i].name; i++)
> + kfree(entry[i].name);
> +
> +skip_device:
> + kfree(entry);
> + put_device(dev);
> + offset += dev_header->len;
> + }
> +
> + return 0;
> +}
> +
> +static int __init map_properties(void)
> +{
> + struct properties_header *properties;
> + struct setup_data *data;
> + u32 data_len;
> + u64 pa_data;
> + int ret;
> +
> + if (!dmi_match(DMI_SYS_VENDOR, "Apple Inc.") &&
> + !dmi_match(DMI_SYS_VENDOR, "Apple Computer, Inc."))
> + return 0;
> +
> + pa_data = boot_params.hdr.setup_data;
> + while (pa_data) {
> + data = ioremap(pa_data, sizeof(*data));
> + if (!data) {
> + pr_err("cannot map setup_data header\n");
> + return -ENOMEM;
> + }
> +
> + if (data->type != SETUP_APPLE_PROPERTIES) {
> + pa_data = data->next;
> + iounmap(data);
> + continue;
> + }
> +
> + data_len = data->len;
> + iounmap(data);
Newline.
> + data = ioremap(pa_data, sizeof(*data) + data_len);
> + if (!data) {
> + pr_err("cannot map setup_data payload\n");
> + return -ENOMEM;
> + }
> +
> + properties = (struct properties_header *)data->data;
> + if (properties->version != 1) {
> + pr_err("unsupported version:\n");
> + print_hex_dump(KERN_ERR, pr_fmt(), DUMP_PREFIX_OFFSET,
> + 16, 1, properties, data_len, true);
> + ret = -ENOTSUPP;
> + } else if (properties->len != data_len) {
> + pr_err("length mismatch, expected %u\n", data_len);
> + print_hex_dump(KERN_ERR, pr_fmt(), DUMP_PREFIX_OFFSET,
> + 16, 1, properties, data_len, true);
> + ret = -EINVAL;
> + } else
> + ret = unmarshal_devices(properties);
> +
> + /*
> + * can only free the setup_data payload but not its header
> + * to avoid breaking the chain of ->next pointers
> + */
Multi-line comments should begin with capital letters and end with
full-stops.
> + data->len = 0;
> + iounmap(data);
> + free_bootmem_late(pa_data + sizeof(*data), data_len);
Newline.
> + return ret;
> + }
> + return 0;
> +}
> +
> +fs_initcall(map_properties);
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 2617672..88f343f 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -443,6 +443,22 @@ typedef struct {
> #define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO_16 0x20000
> #define EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 0x40000
>
> +typedef struct {
> + u32 version;
> + u32 get;
> + u32 set;
> + u32 del;
> + u32 get_all;
> +} apple_properties_protocol_32;
> +
> +typedef struct {
> + u64 version;
> + u64 get;
> + u64 set;
> + u64 del;
> + u64 get_all;
> +} apple_properties_protocol_64;
> +
Please append a '_t' to both of these so it's obvious they're types.
> /*
> * Types and defines for EFI ResetSystem
> */
> @@ -592,6 +608,7 @@ void efi_native_runtime_setup(void);
> #define EFI_RNG_ALGORITHM_RAW EFI_GUID(0xe43176d7, 0xb6e8, 0x4827, 0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61)
> #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID EFI_GUID(0xdcfa911d, 0x26eb, 0x469f, 0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
> #define EFI_CONSOLE_OUT_DEVICE_GUID EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4, 0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
> +#define APPLE_PROPERTIES_PROTOCOL_GUID EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb, 0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
>
> /*
> * This GUID is used to pass to the kernel proper the struct screen_info
> --
> 2.10.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
