Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Grant Likely <grant.likely@secretlab.ca>]

On Mon, Aug 16, 2010 at 12:47 PM, Patrick Pannuto
<ppannuto@codeaurora.org> wrote:
> On 08/13/2010 03:05 PM, Grant Likely wrote:
>>> @@ -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"
>>
>
> Fixed.

Actually, I also made a kerneldoc style error here because the braces
are missing.  This should be:

+/**
+ * pseudo_platform_bus_register() - register an "almost platform bus"

See Documentation/kernel-doc-nano-HOWTO.txt for details.

I'm also not fond of the "pseudo" name.  It isn't really a pseudo bus,
but rather a different bus type that happens to inherit most of the
platform_bus infrastructure.  It may be better just to expose the
platform_bus helper functions without any reference to pseudo, and let
the users be responsible for any naming differentiation.  This is
particularly true if the custom bus becomes responsible for
registering itself and only the initialization functions are exposed.

[...]
>>> +       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.
>>
>
> No, this won't work. (Unless, every pseudo_bus_type author did this same
> kludge to work around const - better to do once IMHO)
>
> struct bus_type {
>        ...
>        const struct dev_pm_ops *pm;
>        ...
> };
>
> That const is there to *prevent* device/driver writers from overriding
> pm ops on accident, and to that end, it has value. What we would really
> like here is 'const after registered' semantics, where the bus author
> can fill in half the structure, and the platform code can fill in the
> rest. However, allowing subclasses to modify private data elements isn't
> possible in C :)

Okay then, here is a better approach:  Don't do any allocation in this
function.  Just initialize the bus_type exactly the way it is
initialized for the platform bus and assign the default dev_pm_ops.
If the custom bus needs to override them, then it should be
responsible to kmemdup the default dev_pm_ops structure and modify the
copy.  The code will be a lot simpler that way.

> I believe the 'const' here provides valuable safety. My original attempt
> at doing this removed the const, and I overwrote one of the pointers in
> platform_dev_pm_ops on my first try at implementing it!

Yes, overriding the const is a bad idea and you'd be wrong to override it.  :-)

Cheers,
g.