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

[Peter Chen]

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.

> > +
> > +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.

> 
> > +
> > +       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.

> 
> > +       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.


> > +       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)

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.

-- 

Best Regards,
Peter Chen