| 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: <4C649C25.5090808@codeaurora.org>
Date: Thu, 12 Aug 2010 18:13:09 -0700
From: Patrick Pannuto <ppannuto@...eaurora.org>
To: Patrick Pannuto <ppannuto@...eaurora.org>
CC: linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
magnus.damm@...il.com, grant.likely@...retlab.ca, gregkh@...e.de,
Kevin Hilman <khilman@...prootsystems.com>,
Paul Mundt <lethal@...ux-sh.org>,
Magnus Damm <damm@...nsource.se>,
"Rafael J. Wysocki" <rjw@...k.pl>,
Eric Miao <eric.y.miao@...il.com>,
Dmitry Torokhov <dtor@...l.ru>, netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform
buses
On 08/10/2010 04:49 PM, Patrick Pannuto wrote:
> (lkml.org seems to have lost August 3rd...)
> RFC: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01342.html
> v1: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01942.html
>
> Based on the idea and code originally proposed by Kevin Hilman:
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html
>
> Changes from v1:
>
> * "Pseduo" buses are no longer init'd, they are [un]registered by:
> - pseudo_platform_bus_register(struct bus_type *)
> - pseudo_platform_bus_unregister(struct bus_type *)
> * These registrations [de]allocate a dev_pm_ops structure for the
> pseudo bus_type
> * Do not overwrite the parent if .bus is already set (that is, allow
> pseudo bus devices to be root devices)
>
> * Split into 2 patches:
> - 1/2: Already sent separately, but included here for clarity
> - 2/2: The real meat of the patch (this patch)
>
> INTRO
>
> As SOCs become more popular, the desire to quickly define a simple,
> but functional, bus type with only a few unique properties becomes
> desirable. As they become more complicated, the ability to nest these
> simple busses and otherwise orchestrate them to match the actual
> topology also becomes desirable.
>
> EXAMPLE USAGE
>
> /arch/ARCH/MY_ARCH/my_bus.c:
>
> #include <linux/device.h>
> #include <linux/platform_device.h>
>
> struct bus_type SOC_bus_type = {
> .name = "SOC-bus-type",
> };
> EXPORT_SYMBOL_GPL(SOC_bus_type);
>
> struct platform_device SOC_bus1 = {
> .name = "SOC-bus1",
> .id = -1,
> .dev.bus = &SOC_bus_type;
> };
> EXPORT_SYMBOL_GPL(SOC_bus1);
>
> struct platform_device SOC_bus2 = {
> .name = "SOC-bus2",
> .id = -2,
> .dev.bus = &SOC_bus_type;
> };
> EXPORT_SYMBOL_GPL(SOC_bus2);
>
> static int __init SOC_bus_init(void)
> {
> int error;
>
> error = pseudo_platform_bus_register(&SOC_bus_type);
> if (error)
> return error;
>
> error = platform_device_register(&SOC_bus1);
> if (error)
> goto fail_bus1;
>
> error = platform_device_register(&SOC_bus2);
> if (error)
> goto fail_bus2;
>
> return error;
>
> /* platform_device_unregister(&SOC_bus2); */
> fail_bus2:
> platform_device_unregister(&SOC_bus1);
> fail_bus1:
> pseudo_platform_bus_unregister(&SOC_bus_type);
>
> return error;
> }
>
> /drivers/my_driver.c:
> static struct platform_driver my_driver = {
> .driver = {
> .name = "my-driver",
> .owner = THIS_MODULE,
> .bus = &SOC_bus_type,
> },
> };
>
> /somewhere/my_device.c:
> static struct platform_device my_device = {
> .name = "my-device",
> .id = -1,
> .dev.bus = &my_bus_type,
> .dev.parent = &SOC_bus1.dev,
> };
>
> This will build a device tree that mirrors the actual system:
>
> /sys/bus
> |-- SOC-bus-type
> | |-- devices
> | | |-- SOC_bus1 -> ../../../devices/SOC_bus1
> | | |-- SOC_bus2 -> ../../../devices/SOC_bus2
> | | |-- my-device -> ../../../devices/SOC_bus1/my-device
> | |-- drivers
> | | |-- my-driver
>
> /sys/devices
> |-- SOC_bus1
> | |-- my-device
> |-- SOC_bus2
>
> Driver can drive any device on the SOC, which is logical, without
> actually being registered on multiple /bus_types/, even though the
> devices may be on different /physical buses/ (which are actually
> just devices).
>
> THOUGHTS:
>
> 1.
> Notice that for a device / driver, only 3 lines were added to
> switch from the platform bus to the new SOC_bus. This is
> especially valuable if we consider supporting a legacy SOCs
> and new SOCs where the same driver is used, but may need to
> be on either the platform bus or the new SOC_bus. The above
> code then becomes:
>
> (possibly in a header)
> #ifdef CONFIG_MY_BUS
> #define MY_BUS_TYPE &SOC_bus_type
> #else
> #define MY_BUS_TYPE NULL
> #endif
>
> /drivers/my_driver.c
> static struct platform_driver my_driver = {
> .driver = {
> .name = "my-driver",
> .owner = THIS_MODULE,
> .bus = MY_BUS_TYPE,
> },
> };
>
> Which will allow the same driver to easily to used on either
> the platform bus or the newly defined bus type.
>
> 2.
> Implementations wishing to make dynamic / run-time decisions on where
> devices are placed could easily create wrapper functions, that is
>
> int SOC_device_register(struct platform_device *pdev)
> {
> if (pdev->archdata->flag)
> pdev->dev.parent = &SOC_bus1.dev;
> else
> pdev->dev.parent = &SOC_bus2.dev;
>
> return platform_device_register(pdev);
> }
>
> A similar solution also would allow for run-time determination of dev.bus,
> if that were necessary for your platform.
>
> 3.
> I'm not convinced that dynamically allocating a new copy of dev_pm_ops is
> the best solution. I *AM*, however, convinced that removing const from
> struct bus_type {
> ...
> const struct dev_pm_ops *pm;
> ...
> };
> would be a mistake; it is far too easy to overwrite one of the function
> pointers on accident, and the const serves a very useful purpose here.
>
> Cc: Kevin Hilman <khilman@...prootsystems.com>
> Signed-off-by: Patrick Pannuto <ppannuto@...eaurora.org>
> ---
> drivers/base/platform.c | 92 +++++++++++++++++++++++++++++++++++++-
> include/linux/platform_device.h | 3 +
> 2 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index b69ccb4..933e0c1 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -238,8 +238,12 @@ int platform_device_add(struct platform_device *pdev)
> if (!pdev)
> return -EINVAL;
>
> - if (!pdev->dev.parent)
> - pdev->dev.parent = &platform_bus;
> + if (!pdev->dev.bus) {
> + pdev->dev.bus = &platform_bus_type;
> +
> + if (!pdev->dev.parent)
> + pdev->dev.parent = &platform_bus;
> + }
>
> pdev->dev.bus = &platform_bus_type;
^^^ small bug here, this line should be deleted; any other comments though?
>
> @@ -482,7 +486,8 @@ static void platform_drv_shutdown(struct device *_dev)
> */
> int platform_driver_register(struct platform_driver *drv)
> {
> - drv->driver.bus = &platform_bus_type;
> + if (!drv->driver.bus)
> + drv->driver.bus = &platform_bus_type;
> if (drv->probe)
> drv->driver.probe = platform_drv_probe;
> if (drv->remove)
> @@ -1017,6 +1022,87 @@ struct bus_type platform_bus_type = {
> };
> EXPORT_SYMBOL_GPL(platform_bus_type);
>
> +/** pseudo_platform_bus_register - register an "almost platform bus"
> + * @bus: partially complete bus type to register
> + *
> + * This init will fill in any ommitted fields in @bus, however, it
> + * also allocates memory and replaces the pm field in @bus, since
> + * pm is const, but some of its pointers may need this one-time
> + * initialziation overwrite.
> + *
> + * @bus's registered this way should be released with
> + * pseudo_platform_bus_unregister
> + */
> +int pseudo_platform_bus_register(struct bus_type *bus)
> +{
> + int error;
> + struct dev_pm_ops* heap_pm;
> + heap_pm = kmalloc(sizeof (*heap_pm), GFP_KERNEL);
> +
> + if (!heap_pm)
> + return -ENOMEM;
> +
> + if (!bus->dev_attrs)
> + bus->dev_attrs = platform_bus_type.dev_attrs;
> + if (!bus->match)
> + bus->match = platform_bus_type.match;
> + if (!bus->uevent)
> + bus->uevent = platform_bus_type.uevent;
> + if (!bus->pm)
> + memcpy(heap_pm, &platform_bus_type.pm, sizeof(*heap_pm));
> + else {
> + heap_pm->prepare = (bus->pm->prepare) ?
> + bus->pm->prepare : platform_pm_prepare;
> + heap_pm->complete = (bus->pm->complete) ?
> + bus->pm->complete : platform_pm_complete;
> + heap_pm->suspend = (bus->pm->suspend) ?
> + bus->pm->suspend : platform_pm_suspend;
> + heap_pm->resume = (bus->pm->resume) ?
> + bus->pm->resume : platform_pm_resume;
> + heap_pm->freeze = (bus->pm->freeze) ?
> + bus->pm->freeze : platform_pm_freeze;
> + heap_pm->thaw = (bus->pm->thaw) ?
> + bus->pm->thaw : platform_pm_thaw;
> + heap_pm->poweroff = (bus->pm->poweroff) ?
> + bus->pm->poweroff : platform_pm_poweroff;
> + heap_pm->restore = (bus->pm->restore) ?
> + bus->pm->restore : platform_pm_restore;
> + heap_pm->suspend_noirq = (bus->pm->suspend_noirq) ?
> + bus->pm->suspend_noirq : platform_pm_suspend_noirq;
> + heap_pm->resume_noirq = (bus->pm->resume_noirq) ?
> + bus->pm->resume_noirq : platform_pm_resume_noirq;
> + heap_pm->freeze_noirq = (bus->pm->freeze_noirq) ?
> + bus->pm->freeze_noirq : platform_pm_freeze_noirq;
> + heap_pm->thaw_noirq = (bus->pm->thaw_noirq) ?
> + bus->pm->thaw_noirq : platform_pm_thaw_noirq;
> + heap_pm->poweroff_noirq = (bus->pm->poweroff_noirq) ?
> + bus->pm->poweroff_noirq : platform_pm_poweroff_noirq;
> + heap_pm->restore_noirq = (bus->pm->restore_noirq) ?
> + bus->pm->restore_noirq : platform_pm_restore_noirq;
> + heap_pm->runtime_suspend = (bus->pm->runtime_suspend) ?
> + bus->pm->runtime_suspend : platform_pm_runtime_suspend;
> + heap_pm->runtime_resume = (bus->pm->runtime_resume) ?
> + bus->pm->runtime_resume : platform_pm_runtime_resume;
> + heap_pm->runtime_idle = (bus->pm->runtime_idle) ?
> + bus->pm->runtime_idle : platform_pm_runtime_idle;
> + }
> + bus->pm = heap_pm;
> +
> + error = bus_register(bus);
> + if (error)
> + kfree(bus->pm);
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_register);
> +
> +void pseudo_platform_bus_unregister(struct bus_type *bus)
> +{
> + bus_unregister(bus);
> + kfree(bus->pm);
> +}
> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_unregister);
> +
> int __init platform_bus_init(void)
> {
> int error;
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 5417944..5ec827c 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -79,6 +79,9 @@ extern int platform_driver_probe(struct platform_driver *driver,
> #define platform_get_drvdata(_dev) dev_get_drvdata(&(_dev)->dev)
> #define platform_set_drvdata(_dev,data) dev_set_drvdata(&(_dev)->dev, (data))
>
> +extern int pseudo_platform_bus_register(struct bus_type *);
> +extern void pseudo_platform_bus_unregister(struct bus_type *);
> +
> extern struct platform_device *platform_create_bundle(struct platform_driver *driver,
> int (*probe)(struct platform_device *),
> struct resource *res, unsigned int n_res,
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--
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