| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4E16DCFA.5050903@arm.com>
Date: Fri, 08 Jul 2011 11:33:30 +0100
From: Marc Zyngier <marc.zyngier@....com>
To: Michał Mirosław <mirqus@...il.com>
CC: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Grant Likely <grant.likely@...retlab.ca>
Subject: Re: [RFC PATCH v2 2/4] Core device subsystem implementation
On 08/07/11 11:18, Michał Mirosław wrote:
> 2011/7/8 Marc Zyngier <marc.zyngier@....com>:
>> There is a small number of devices that the core kernel needs very
>> early in the boot process, namely an interrupt controller and a timer,
>> long before the device model is up and running.
>>
>> The "core device subsystem" offers a class based device/driver
>> matching model, doesn't rely on any other subsystem, is very (too?)
>> simple, and support getting information both from DT as well as from
>> static data provided by the platform. It also gives the opportunity to
>> define the probing order by offering a sorting hook at runtime.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@....com>
> [...]
>> --- /dev/null
>> +++ b/drivers/base/core_device.c
>> @@ -0,0 +1,108 @@
> [...]
>> +static int __init core_device_match(struct core_device *dev,
>> + struct core_device_id *ids)
>> +{
>> + int i;
>> +#ifdef CONFIG_OF
>> + if (dev->of_node)
>> + for (i = 0; ids[i].name != NULL; i++)
>> + if (of_device_is_compatible(dev->of_node,
>> + ids[i].name))
>> + return 1;
>
> Add an else here? I assume DT devices shouldn't be matched by name.
Good point. I'll update that.
>> +#endif
>> + for (i = 0; ids[i].name != NULL; i++)
>> + if (!strcmp(dev->name, ids[i].name))
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
> [...]
>> --- /dev/null
>> +++ b/include/linux/core_device.h
>> @@ -0,0 +1,69 @@
>> +/*
>> + * Copyright (C) 2011 ARM Ltd
>> + * Written by Marc Zyngier <marc.zyngier@....com>
>> + *
>> + * 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.
>> + *
>> + * Core device init support
>> + */
>> +
>> +#ifndef _CORE_DEVICE_H
>> +#define _CORE_DEVICE_H
>> +
>> +#include <linux/list.h>
>> +#include <linux/ioport.h>
>> +#include <linux/of.h>
>> +
>> +struct core_device_id {
>> + const char *name;
>> +};
>> +
>> +enum core_device_class {
>> + CORE_DEV_CLASS_IRQ,
>> + CORE_DEV_CLASS_TIMER,
>> + CORE_DEV_CLASS_MAX /* Do not use as a class */
>> +};
>
> CORE_DEV_CLASS_MAX -> CORE_DEV_CLASS_COUNT
>
> _MAX suggests that this is the largest value, but in this case it is a count.
_MAX seem to be the established usage in the kernel (I just grep-ed for
"_MAX," in include/linux, and found a number of similar uses, while only
ACPI seem to be using _COUNT).
>> +
>> +struct core_device {
>> + const char *name;
>> + u32 num_resources;
>> + struct resource *resource;
>> +#ifdef CONFIG_OF
>> + struct device_node *of_node;
>> +#endif
>> + struct list_head entry;
>> +};
>> +
>> +struct core_driver {
>> + int (*init)(struct core_device *);
>> + struct core_device_id *ids;
>> +};
>> +
>> +void core_device_register(enum core_device_class class,
>> + struct core_device *dev);
>> +void core_driver_init_class(enum core_device_class class,
>> + void (*sort)(struct list_head *));
>> +#ifdef CONFIG_OF
>> +void of_core_device_populate(enum core_device_class class,
>> + struct of_device_id *matches);
>> +#else
>> +static inline void of_core_device_populate(enum core_device_class class,
>> + struct of_device_id *matches)
>> +{
>> +}
>> +#endif
>> +
>> +struct core_driver_setup_block {
>> + enum core_device_class class;
>> + struct core_driver *drv;
>> +};
>> +
>> +#define core_driver_register(cls, drv) \
>> +static struct core_driver_setup_block __core_driver_block_##cls##_##drv \
>> + __used __section(.init.core_driver) \
>> + __attribute__((aligned((sizeof(long))))) \
>> + = { cls, &drv }
>> +
>> +#endif
>
> Since core_driver_register() is not a function it shouldn't look like
> one. Something like DEFINE_CORE_DRIVER_ENTRY() would be better. I
> would make cls to be only the last word of CORE_DEV_CLASS_* values so
> its less typing in the board files (unless you don't like CPP string
> concatenation).
>
> +#define DEFINE_CORE_DRIVER_ENTRY(cls, drv) \
> +static struct core_driver_setup_block __core_driver_block_##cls##_##drv \
> + __used __section(.init.core_driver) \
> + = { CORE_DEV_CLASS_##cls, &drv }
I fully agree with the "not a function" approach. I'm more cautious with
the "CORE_DEV_CLASS_##cls" bit. I feel like it's hiding a bit too much
of what's going on, but it's only my own feeling, and I'd be happy to
change it if that's what people prefer.
Thanks for reviewing,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists