| 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: <AANLkTi=GduvsmkXpxsVoN7k3gPDjjRgqe0E_Z2YeEVS5@mail.gmail.com>
Date: Fri, 13 Aug 2010 16:05:28 -0600
From: Grant Likely <grant.likely@...retlab.ca>
To: Patrick Pannuto <ppannuto@...eaurora.org>
Cc: linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
magnus.damm@...il.com, 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 Tue, Aug 10, 2010 at 5:49 PM, Patrick Pannuto
<ppannuto@...eaurora.org> 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
Hi Patrick,
Before acking this as something that should be merged, I'd like to see
some real device drivers converted to use this interface so I can
better judge whether or not it is a good idea. More comments below.
> 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;
>
For safety, I think you'd want to have a separate add function for
each new platform bus type, and change the name of this function to
explicitly pass the bus type:
int __platform_device_add(struct platform_device *pdev)
{
if (!pdev->dev.bus)
return -EINVAL;
[... existing code ...]
}
int platform_device_add(struct platform_device *pdev)
{
if (!pdev->dev.parent)
pdev->dev.parent = &platform_bus;
pdev->dev.bus = &platform_bus_type;
__platform_device_add(pdev);
}
And then for each bus type (in this example, I'll call it
foo_bus_type, and assume foo_device wraps platform_device):
int foo_device_add(struct foo_device *foo)
{
foo->pdev.dev.bus = &foo_bus_type;
__platform_device_add(&foo->pdev);
}
That will allow the new bus_type code to keep foo_bus_type as an
unexported static and will provide a bit more type safety. For
example, it avoids accidentally registering a plain (unwrapped)
platform device on the foo_bus_type.
> @@ -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)
Ditto here; better to have a separate foo_driver_register() for each
new bus type.
> @@ -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"
Kerneldoc style error. Should be:
+/**
+ * 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;
Nit: heap_pm is an odd name. Typically Linux local vars are named
according to what they are, not where the memory is allocated from.
How about simply 'ops'
> + 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);
Ick, this is a nasty list of conditional statements. :-) Instead it
could have an unconditional initialize function which sets it up just
like the platform bus without registering. Then allow the
foo_bus_type initialization code override the ones it cares about, and
then register directly.
> +
> +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,
> --
> 1.7.2
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists