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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 28 Apr 2016 11:01:16 +0300
From:	Roger Quadros <rogerq@...com>
To:	Peter Chen <hzpeterchen@...il.com>
CC:	Jun Li <jun.li@....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 28/04/16 04:54, Peter Chen wrote:
> On Wed, Apr 27, 2016 at 01:59:44PM +0300, Roger Quadros wrote:
>> Hi,
>>
>> On 27/04/16 06:15, Peter Chen wrote:
>>> On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
>>>> 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.
>>>>
>>>
>>> Roger, after discussing with Jun off line, we think usb_otg_register
>>> should return -ENOTSUPP if platform is OTG capabilities (HNP || SRP ||
>>> ADP), since this patch set does not cover fully otg features, the users
>>
>> But this series isn't preventing full otg implementation. You can
>> still do that via config->otg_work.
>>
>> I can modify the following condition to return -ENOTSUPP instead of
>> defaulting to dual-role
>>
>> struct usb_otg *usb_otg_register(...)
>> {
>> ...
>>         if ((otg->caps->hnp_support || otg->caps->srp_support ||
>>              otg->caps->adp_support) && !config->otg_work) {
>> 		dev_err(dev, "otg: otg_work must be provided for OTG support\n");
>> 		return -ENOTSUPP;
>> 	}
>> ...
>> }
>>
> 
> According to Jun, this framework still needs to improve (maybe small) even
> using user work item for fully OTG, so the first step is let the drd
> work well:)
> 
Agreed. Until one OTG implementation is tested we can't really say it works :).

cheers,
-roger