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

[Peter Chen]

On Thu, Dec 01, 2016 at 10:57:24PM +0100, Rafael J. Wysocki wrote:
> 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.

ok.

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

Ok, I will change it to debug message.

> >> > + */
> >> > +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?

I will add mutex for it, thanks.

> >
> >> > + */
> >> > +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?
> 

Not any problems, I will follow your comments.

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

Thanks, yes, we need to consider PM.

The initial idea for this library is only for power on/off. It does not
take power management into consideration. As an enhancement, we need
to consider PM, and implement pwrseq_suspend/resume accordingly (will
consider concurrent issue). I will add related APIs at pwrseq_generic.c
at next version, and only call clock operations at it (reset gpio
is not needed for PM).

> 
> > 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?
> 

For system PM, the list is still existed. It calls of_pwrseq_suspend/resume
accordingly. The first user for this pwrseq library is USB HUB.
(drivers/usb/core/hub.c).

-- 

Best Regards,
Peter Chen