Re: [PATCH v10 2/8] power: add power sequence library

[Rafael J. Wysocki]

On Tue, Nov 22, 2016 at 4:53 AM, Peter Chen <hzpeterchen@xxxxxxxxx> wrote:
> On Tue, Nov 22, 2016 at 03:23:12AM +0100, Rafael J. Wysocki wrote:
>> > @@ -0,0 +1,237 @@
>> > +/*
>> > + * core.c      power sequence core file
>> > + *
>> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
>> > + * Author: Peter Chen <peter.chen@xxxxxxx>
>> > + *
>> > + * This program is free software: you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2  of
>> > + * the License as published by the Free Software Foundation.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + * You should have received a copy of the GNU General Public License
>> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>
>> The last paragraph is not necessary AFAICS.
>
> I just copy it from:
>
> https://www.gnu.org/licenses/gpl-howto.en.html
>
> If you are concerns about it, I can delete it.

It is redundant, so yes, please.

>> > +
>> > +static struct pwrseq *pwrseq_find_available_instance(struct device_node *np)
>> > +{
>> > +       struct pwrseq *pwrseq;
>> > +
>> > +       list_for_each_entry(pwrseq, &pwrseq_list, node) {
>> > +               if (pwrseq->used)
>> > +                       continue;
>> > +
>> > +               /* compare compatible string for pwrseq node */
>> > +               if (of_match_node(pwrseq->pwrseq_of_match_table, np)) {
>> > +                       pwrseq->used = true;
>> > +                       return pwrseq;
>> > +               }
>> > +
>> > +               /* return generic pwrseq instance */
>> > +               if (!strcmp(pwrseq->pwrseq_of_match_table->compatible,
>> > +                               "generic")) {
>> > +                       pr_debug("using generic pwrseq instance for %s\n",
>> > +                               np->full_name);
>> > +                       pwrseq->used = true;
>> > +                       return pwrseq;
>> > +               }
>> > +       }
>> > +       pr_warn("Can't find any pwrseq instances for %s\n", np->full_name);
>>
>> pr_debug() ?
>
> If there is no pwrseq instance for that node, the power sequence on routine will
> return fail, so I think an warning message is useful for user.

Useful in what way?  How is the user supposed to know what happened
from this message?

>>
>> > +
>> > +       return NULL;
>> > +}
>> > +
>> > +/**
>> > + * of_pwrseq_on: do power sequence on for device node
>>
>> of_pwrseq_on - Carry out power sequence on for device node
>>
>> Argument description should follow this line.
>>
>> > + *
>> > + * This API is used to power on single device, if the host
>> > + * controller only needs to handle one child device (this device
>> > + * node points to), use this API. If multiply devices are needed
>> > + * to handle on bus, use of_pwrseq_on_list.
>>
>> That's unclear.
>>
>> What about "Carry out a single device power on.  If multiple devices
>> need to be handled, use of_pwrseq_on_list() instead."
>>
>> > + *
>> > + * @np: the device node would like to power on
>> > + *
>> > + * On successful, it returns pwrseq instance, otherwise an error value.
>>
>> "Return a pointer to the power sequence instance on success, or an
>> error code otherwise."
>>
>
> Ok, will change.
>
>> > + */
>> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
>> > +{
>> > +       struct pwrseq *pwrseq;
>> > +       int ret;
>> > +
>> > +       pwrseq = pwrseq_find_available_instance(np);
>>
>> What does guarantee the integrity of ths list at this point?
>
> Once the use selects the specific pwrseq library, the library will
> create an empty one instance during the initialization, and it
> will be called at postcore_initcall, the device driver has not
> probed yet.

Which doesn't matter really, because the list is global and some other
driver using it might have been probed already.

You have a mutex here and it is used for add/remove.  Why isn't it
used for list browsing?

>
>>
>> > +       if (!pwrseq)
>> > +               return ERR_PTR(-ENONET);
>>
>> ENOENT I suppose?
>>
>
> Good catch, thanks.
>
>> > +/**
>> > + * of_pwrseq_off: do power sequence off for this pwrseq instance
>> > + *
>> > + * This API is used to power off single device, it is the opposite
>> > + * operation for of_pwrseq_on.
>> > + *
>> > + * @pwrseq: the pwrseq instance which related device would like to be off
>> > + */
>> > +void of_pwrseq_off(struct pwrseq *pwrseq)
>> > +{
>> > +       pwrseq_off(pwrseq);
>> > +       pwrseq_put(pwrseq);
>> > +}
>> > +EXPORT_SYMBOL_GPL(of_pwrseq_off);
>>
>> What happens if two code paths attempt to turn the same power sequence
>> off in parallel?  Can it ever happen?  If not, then why not?
>>
>
> I don't think the same pwrseq instance off will be called at the same
> time, the of_pwrseq_off is supposed to be only called at error path
> during power-on and at device power-off routine, and only the power-on is
> successful, the device can be created, if the device is not created,
> its power-off routine is not supposed to be called.
>
>> > +
>> > +/**
>> > + * of_pwrseq_on_list: do power sequence on for list
>> > + *
>> > + * This API is used to power on multiple devices at single bus.
>> > + * If there are several devices on bus (eg, USB bus), uses this
>> > + * this API. Otherwise, use of_pwrseq_on. After the device
>> > + * is powered on successfully, it will be added to pwrseq list for
>> > + * this bus.
>> > + *
>> > + * @np: the device node would like to power on
>> > + * @head: the list head for pwrseq list on this bus
>> > + *
>> > + * On successful, it returns 0, otherwise an error value.
>>
>> Please format the kerneldoc comment in a usual way.
>>
>
> Ok.
>
>> > + */
>> > +int of_pwrseq_on_list(struct device_node *np, struct list_head *head)
>> > +{
>> > +       struct pwrseq *pwrseq;
>> > +       struct pwrseq_list_per_dev *pwrseq_list_node;
>> > +
>> > +       pwrseq = of_pwrseq_on(np);
>> > +       if (IS_ERR(pwrseq))
>> > +               return PTR_ERR(pwrseq);
>> > +
>> > +       pwrseq_list_node = kzalloc(sizeof(*pwrseq_list_node), GFP_KERNEL);
>>
>> Why don't you allocate memory before turning the power sequence on?
>>
>
> This list is only for power sequence on instance, if I allocate memory before
> power sequence on, I need to free it if power sequence on is failed.

So why is that a problem?

>> > +       if (!pwrseq_list_node) {
>> > +               of_pwrseq_off(pwrseq);
>> > +               return -ENOMEM;
>> > +       }
>> > +       pwrseq_list_node->pwrseq = pwrseq;
>> > +       list_add(&pwrseq_list_node->list, head);
>> > +
>> > +       return 0;
>> > +}
>> > +EXPORT_SYMBOL_GPL(of_pwrseq_on_list);
>>
>> So the caller is supposed to provide a list head of the list to put
>> the power sequence object into on success, right?
>
> Yes
>
>>
>> Can you explain to me what the idea here is, please?
>>
>
> Taking USB devices as an example, there is one power sequence on list
> per bus, and there are several USB devices on the bus. Using a list,
> we can record which device is powered sequence on, and only powers
> sequence off which has already powered sequence on at error path, and
> power sequence off all devices on the bus when the bus (eg, USB HUB)
> is removed. (eg, when the bus driver is removed)

Well, I'm not sure I understand this correctly.

What about system suspend/resume and such, for instance?

> Usually, the power sequence is only needed for hard-wired devices,
> the power sequence on is carried out during the bus driver probed,
> and off if carried out during the bus driver is removed,
> of_pwrseq_on_list/of_powerseq_off_list is not supposed to be
> called during the other bus driver life cycles.
>
>> Also, what's the protection of the list against concurrent access?
>>
>
> I will add comment that the list creator needs to take consideration
> of concurrent access if exists.
>
>> > +
>> > +/**
>> > + * of_pwrseq_off_list: do power sequence off for the list
>> > + *
>> > + * This API is used to power off all devices on this bus, it is
>> > + * the opposite operation for of_pwrseq_on_list.
>> > + *
>> > + * @head: the list head for pwrseq instance list on this bus
>> > + */
>> > +void of_pwrseq_off_list(struct list_head *head)
>> > +{
>> > +       struct pwrseq *pwrseq;
>> > +       struct pwrseq_list_per_dev *pwrseq_list_node, *tmp_node;
>> > +
>> > +       list_for_each_entry_safe(pwrseq_list_node, tmp_node, head, list) {
>> > +               pwrseq = pwrseq_list_node->pwrseq;
>> > +               of_pwrseq_off(pwrseq);
>> > +               list_del(&pwrseq_list_node->list);
>> > +               kfree(pwrseq_list_node);
>> > +       }
>> > +}
>> > +EXPORT_SYMBOL_GPL(of_pwrseq_off_list);
>>
>> This looks horribly inefficient.
>>
>> Is the user expected to create the list from scratch every time things
>> are turned on?
>>
>
> Like I explained above, the power sequence is for hard-wired device on
> board, the list creation and remove are only carried out on driver's
> probe and remove.

Which driver exactly are you referring to?

Thanks,
Rafael