linux-kernel - Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160426082106.GE2978@peterchendt>
Date:	Tue, 26 Apr 2016 16:21:07 +0800
From:	Peter Chen <hzpeterchen@...il.com>
To:	Jun Li <jun.li@....com>
Cc:	Roger Quadros <rogerq@...com>,
	"stern@...land.harvard.edu" <stern@...land.harvard.edu>,
	"balbi@...nel.org" <balbi@...nel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"peter.chen@...escale.com" <peter.chen@...escale.com>,
	"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	"jun.li@...escale.com" <jun.li@...escale.com>,
	"mathias.nyman@...ux.intel.com" <mathias.nyman@...ux.intel.com>,
	"tony@...mide.com" <tony@...mide.com>,
	"Joao.Pinto@...opsys.com" <Joao.Pinto@...opsys.com>,
	"abrestic@...omium.org" <abrestic@...omium.org>,
	"r.baldyga@...sung.com" <r.baldyga@...sung.com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>
Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

On Tue, Apr 26, 2016 at 07:00:22AM +0000, Jun Li wrote:
> Hi
> 
> > -----Original Message-----
> > From: Peter Chen [mailto:hzpeterchen@...il.com]
> > Sent: Tuesday, April 26, 2016 2:28 PM
> > To: Jun Li <jun.li@....com>
> > Cc: Roger Quadros <rogerq@...com>; stern@...land.harvard.edu;
> > balbi@...nel.org; gregkh@...uxfoundation.org; peter.chen@...escale.com;
> > dan.j.williams@...el.com; jun.li@...escale.com;
> > mathias.nyman@...ux.intel.com; tony@...mide.com; Joao.Pinto@...opsys.com;
> > abrestic@...omium.org; r.baldyga@...sung.com; linux-usb@...r.kernel.org;
> > linux-kernel@...r.kernel.org; linux-omap@...r.kernel.org
> > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > 
> > On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
> > > Hi
> > >
> > > > -----Original Message-----
> > > > From: Peter Chen [mailto:hzpeterchen@...il.com]
> > > > Sent: Tuesday, April 26, 2016 11:47 AM
> > > > To: Jun Li <jun.li@....com>
> > > > Cc: Roger Quadros <rogerq@...com>; stern@...land.harvard.edu;
> > > > balbi@...nel.org; gregkh@...uxfoundation.org;
> > > > peter.chen@...escale.com; dan.j.williams@...el.com;
> > > > jun.li@...escale.com; mathias.nyman@...ux.intel.com;
> > > > tony@...mide.com; Joao.Pinto@...opsys.com; abrestic@...omium.org;
> > > > r.baldyga@...sung.com; linux-usb@...r.kernel.org;
> > > > linux-kernel@...r.kernel.org; linux-omap@...r.kernel.org
> > > > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > > >
> > > > On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
> > > > > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > > > > +				 struct usb_otg_config *config) {
> > > > > > +	struct usb_otg *otg;
> > > > > > +	struct otg_wait_data *wait;
> > > > > > +	int ret = 0;
> > > > > > +
> > > > > > +	if (!dev || !config || !config->fsm_ops)
> > > > > > +		return ERR_PTR(-EINVAL);
> > > > > > +
> > > > > > +	/* already in list? */
> > > > > > +	mutex_lock(&otg_list_mutex);
> > > > > > +	if (usb_otg_get_data(dev)) {
> > > > > > +		dev_err(dev, "otg: %s: device already in otg list\n",
> > > > > > +			__func__);
> > > > > > +		ret = -EINVAL;
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	/* allocate and add to list */
> > > > > > +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > > > > +	if (!otg) {
> > > > > > +		ret = -ENOMEM;
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	otg->dev = dev;
> > > > > > +	otg->caps = config->otg_caps;
> > > > > > +
> > > > > > +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > > > > +	     otg->caps->adp_support) && !config->otg_work)
> > > > > > +		dev_info(dev, "otg: limiting to dual-role\n");
> > > > >
> > > > > dev_err, this should be an error.
> > > >
> > > > The condition may be wrong, but it is an information to show that
> > > > current OTG is dual-role.
> > >
> > > This should not happen in any correct design, I even doubt if we
> > > should try to continue by "downgrade" it to be duel role, currently
> > > the only example user is dual role, so doing like this can't be tested
> > > by real case, this downgrade is not so easy like we image, at least
> > > for chipidea otg driver, simply replace a queue worker may not work,
> > > as we have much more difference between the 2 configs.
> > >
> > 
> > Would you show more why chipidea can't work just replace the work item,
> > and see if anything we still can improve for this framework?
> 
> In real OTG, we need enable AVV irq,

Enable and Handling AVV is platform stuff. In this framework, we are
focus on how otg device manages host and gadget together, and the state
machine when the related otg event occurs.

> but for duel role, nobody care/handle,
> there are much more resource required for OTG: timers, hnp polling,
> otg test device handling... 

They are common things for fully OTG fsm, you can move them
to common code (In fact, hnp polling handling is already common code).

> 
> with current design, chipidea driver can support real OTG with its own
> queue worker, or DRD with Roger's drd work item if config is correct.
> 
> But improve something to work on a *wrong* config will make it complicated
> and does not make much sense IMO.
> 

What does above "config" you mean?

If the configure is fully OTG, you can choose different state machine,
eg otg_statemachine, if you find it is hard for chipidea to use this
framework, just list the reason, and see if we can improve.

-- 
Best Regards,
Peter Chen