| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 22 Aug 2016 12:09:54 -0400 (EDT)
From: Alan Stern <stern@...land.harvard.edu>
To: Peter Chen <hzpeterchen@...il.com>
cc: Peter Chen <peter.chen@....com>, <gregkh@...uxfoundation.org>,
<ulf.hansson@...aro.org>, <broonie@...nel.org>, <sre@...nel.org>,
<robh+dt@...nel.org>, <shawnguo@...nel.org>,
<dbaryshkov@...il.com>, <dwmw3@...radead.org>,
<k.kozlowski@...sung.com>, <linux-arm-kernel@...ts.infradead.org>,
<p.zabel@...gutronix.de>, <devicetree@...r.kernel.org>,
<pawel.moll@....com>, <mark.rutland@....com>,
<linux-usb@...r.kernel.org>, <arnd@...db.de>,
<s.hauer@...gutronix.de>, <mail@...iej.szmigiero.name>,
<troy.kisky@...ndarydevices.com>, <festevam@...il.com>,
<oscar@...andei.net>, <stephen.boyd@...aro.org>,
<linux-pm@...r.kernel.org>, <stillcompiling@...il.com>,
<linux-kernel@...r.kernel.org>, <mka@...omium.org>
Subject: Re: [PATCH v6 4/8] usb: core: add power sequence handling for USB
devices
On Mon, 22 Aug 2016, Peter Chen wrote:
> On Mon, Aug 15, 2016 at 05:13:14PM +0800, Peter Chen wrote:
> > Some hard-wired USB devices need to do power sequence to let the
> > device work normally, the typical power sequence like: enable USB
> > PHY clock, toggle reset pin, etc. But current Linux USB driver
> > lacks of such code to do it, it may cause some hard-wired USB devices
> > works abnormal or can't be recognized by controller at all.
> >
> > In this patch, it calls power sequence library APIs to finish
> > the power sequence events. At first, it calls pwrseq_alloc_generic
> > to create a generic power sequence instance, then it will do power
> > on sequence at hub's probe for all devices under this hub
> > (includes root hub).
> >
> > At hub_disconnect, it will do power off sequence which is at powered
> > on list.
> >
> > Signed-off-by: Peter Chen <peter.chen@....com>
> > Tested-by Joshua Clayton <stillcompiling@...il.com>
>
> Hi Alan,
>
> The power sequence library code has been reviewed several rounds, would
> you please help to review the change for USB part?
Some small problems... See below.
> > --- a/drivers/usb/core/Makefile
> > +++ b/drivers/usb/core/Makefile
> > @@ -9,5 +9,6 @@ usbcore-y += port.o of.o
> >
> > usbcore-$(CONFIG_PCI) += hcd-pci.o
> > usbcore-$(CONFIG_ACPI) += usb-acpi.o
> > +usbcore-$(CONFIG_PWRSEQ_GENERIC) += pwrseq.o
Please use a tab to align this with the lines above.
> > --- /dev/null
> > +++ b/drivers/usb/core/pwrseq.c
> > @@ -0,0 +1,100 @@
> > +/*
> > + * pwrseq.c USB device power sequence management
> > + *
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Author: Peter Chen <peter.chen@....com>
> > + *
> > + * 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/>.
> > + */
> > +
> > +#include <linux/list.h>
> > +#include <linux/of.h>
> > +#include <linux/power/pwrseq.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/hcd.h>
> > +
> > +#include "hub.h"
> > +
> > +struct usb_pwrseq_node {
> > + struct pwrseq *pwrseq;
> > + struct list_head list;
> > +};
> > +
> > +static int hub_of_pwrseq_on(struct device_node *np, struct usb_hub *hub)
> > +{
> > + struct pwrseq *pwrseq;
> > + struct usb_pwrseq_node *pwrseq_node;
> > + int ret;
> > +
> > + pwrseq = pwrseq_alloc_generic();
> > + if (IS_ERR(pwrseq))
> > + return PTR_ERR(pwrseq);
> > +
> > + ret = pwrseq_get(np, pwrseq);
> > + if (ret)
> > + goto pwr_free;
> > +
> > + ret = pwrseq_on(np, pwrseq);
> > + if (ret)
> > + goto pwr_put;
> > +
> > + pwrseq_node = kzalloc(sizeof(*pwrseq_node), GFP_KERNEL);
You need to check that the kzalloc succeeded.
> > + pwrseq_node->pwrseq = pwrseq;
> > + list_add(&pwrseq_node->list, &hub->pwrseq_on_list);
> > +
> > + return 0;
> > +
> > +pwr_put:
> > + pwrseq_put(pwrseq);
> > +pwr_free:
> > + pwrseq_free(pwrseq);
> > + return ret;
> > +}
This subroutine looks very generic. The only place where it cares that
it was called for a USB hub is the list_add(), and that could easily
become generic also (make this function take &hub->pwrseq_on_list as
its second argument, instead of hub).
It looks like this routine really belongs in the power sequence
library.
> > +
> > +int hub_pwrseq_on(struct usb_hub *hub)
> > +{
The names of this routine and the subroutine above are too similar.
After all, both functions require OF support.
> > + struct device *parent;
> > + struct usb_device *hdev = hub->hdev;
> > + struct device_node *np;
> > + int ret;
> > +
> > + if (hdev->parent)
> > + parent = &hdev->dev;
> > + else
> > + parent = bus_to_hcd(hdev->bus)->self.controller;
> > +
> > + for_each_child_of_node(parent->of_node, np) {
> > + ret = hub_of_pwrseq_on(np, hub);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void hub_pwrseq_off(struct usb_hub *hub)
> > +{
> > + struct pwrseq *pwrseq;
> > + struct usb_pwrseq_node *pwrseq_node, *tmp_node;
> > +
> > + list_for_each_entry_safe(pwrseq_node, tmp_node,
> > + &hub->pwrseq_on_list, list) {
> > + pwrseq = pwrseq_node->pwrseq;
> > + pwrseq_off(pwrseq);
> > + pwrseq_put(pwrseq);
> > + pwrseq_free(pwrseq);
> > + list_del(&pwrseq_node->list);
> > + kfree(pwrseq_node);
> > + }
> > +}
This routine also looks like it belongs in a library.
Alan Stern
Powered by blists - more mailing lists