Re: [RFC 1/2] pwrseq: Add subsystem to handle complex power sequences

[Ulf Hansson]

On 19 June 2014 16:23, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
> On 06/19/2014 04:03 PM, Hans de Goede wrote:
>> Hi,
>>
>
> <snip>
>
>> Also I'm not sold on how you're making this a devm only thing, and
>> are using devres_alloc to not only allocate memory for resource tracking,
>> but also the actual backing struct, that is not how devres_alloc is
>> intended to be used AFAIK.
>>
>> A problem with having this one always devm managed is that it makes it
>> hard to use in library functions without side effects.
>>
>> e.g. if you look at how your using this in mmc_of_parse in the next
>> patch, this gives mmc_of_parse the side-effect of having allocated
>> and bound the power-seq method, even if mmc_of_parse later
>> fails on e.g. gpio binding. If then for some reason the mmc host
>> probe method decides to not propagate the mmc_of_parse method
>> error (leading to freeing of all devm managed resources), then this is
>> undesirable.
>>
>> Where as with a non devm version, it would be clear that on error
>> mmc_of_parse would need to explicitly release the pwrseq again.
>>
>> I realize that pwrseq implementations likely will want to use
>> devm functions too, and I'm a great fan of devm. But this is something
>> to keep in mind. At a minimum the description of of_mmc_parse needs
>> to get updated with info about it having potential side-effects even
>> when it fails, and that failure should always be treated as a fatal
>> error and cause the host probe method to fail.
>
> Thinking more about this, it may be a good idea to give the pwrseq
> its own struct device, turning it into a virtual device, this way the
> pwrseq-method can use devm managed resources bound to this device,
> we can set the of_node of this device to the actual powerseq
> childnode and it gets its own sysfs dir in which we could do useful things
> such as have an attribute to query the current power state.
>
> This would also mean introducing a non devm version of devm_pwrseq_get
> + an explicit release, which would be useful to avoid the side-effects
> mentioned above when used in library functions such as mmc_of_parse.

Hans, thanks for your comments. I will try to include all your ideas in a v2.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/