Subject: Re: [PATCH v2 2/3] usb: chipidea: Hook into mux framework to toggle
 usb switch
To: Stephen Boyd <stephen.boyd@linaro.org>,
        Peter Chen <Peter.Chen@nxp.com>
Cc: linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, devicetree@vger.infradead.org,
        Rob Herring <robh+dt@kernel.org>, Rob Clark <robdclark@gmail.com>,
        Andy Gross <andy.gross@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        devicetree@vger.kernel.org
References: <20170714214005.14967-1-stephen.boyd@linaro.org>
 <20170714214005.14967-3-stephen.boyd@linaro.org>
 <7080030c-c9c2-657d-aad4-aef636438cb1@axentia.se>
 <150215709134.18242.599725161375995991@sboyd-linaro>
 <c5c411fe-2f5a-88cc-73e8-28ab61f5de04@axentia.se>
 <150249037091.15411.10074988823155268663@sboyd-linaro>
From: Peter Rosin <peda@axentia.se>
Organization: Axentia Technologies AB
Message-ID: <8bbd90f2-a80f-1bf7-e5a8-400cebf6dc89@axentia.se>
Date: Tue, 15 Aug 2017 13:36:42 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <150249037091.15411.10074988823155268663@sboyd-linaro>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Aug 2017 11:36:46.7739 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0202MB2547
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3272
Lines: 66

On 2017-08-12 00:26, Stephen Boyd wrote:
> Quoting Peter Rosin (2017-08-08 05:46:30)
>> On 2017-08-08 03:51, Stephen Boyd wrote:
>>
>>>                      It looked like we paired the start/stop ops with
>>> each other so that the mux is properly managed across these ops.
>>
>> Yes, it *looks* ok...
>>
>>>                                                                  My
>>> testing hasn't shown a problem, but maybe there's some corner case
>>> you're thinking of? I'll double check the code.
>>
>> ...but since I do not know the usb code, I can't tell. What I worry about
>> is the usb core calling udc_id_switch_for_host or udc_id_switch_for_device
>> more than once without any call to the other in between. Maybe that is a
>> guarantee that the usb core makes? Or maybe it isn't? If e.g. there is a
>> third mode (or if one is added in the future), then the calls to
>> mux_control_select and mux_control_deselect would not be paired correctly.
>> Ok, sure, a third mode probably doesn't exist and will probably not be
>> added, but but but...
>>
>> Also, what happens if udc_id_switch_for_device fails? Is it certain that
>> it will be called again before udc_id_switch_for_host is called, or is
>> the failure simply logged? If the latter, you might have a call to
>> mux_control_deselect without a preceding (and successful) call to
>> mux_control_select. That's fatal.
> 
> The only thing that could fail right now is the mux selection, so we
> wouldn't get into some sort of situation where that's locked in and
> unchangeable. We do rollback the role if it fails to switch, so we also
> wouldn't go into a half-way state of being in one role but not actually
> switching all the way over to it.

What do you roll back to if the initial setting of the role fails?
Hopefully that causes a probe failure?

Cheers,
Peter

>> I have similar worries for host_start/host_stop, but for that case
>> host_stop is not allowed to fail, and it seems like a safe bet that
>> host_stop will only be called if host_start succeeds. So, I'm not as
>> worried there.
>>
>> In other words, the question is if the usb core is designed to allow
>> this kind of "raw" resource administration in udc_id_switch_for_host and
>> udc_id_switch_for_device, or if you need to keep a local record of the
>> state so that you do not do double resource acquisition or attempt to
>> free resources you don't have?
>>
>> I think I would feel better if the muxing for the device mode could
>> be done in a start/stop pair of function just like the host mode is
>> doing. Again, I don't know the usb code and don't know if such hooks
>> exist or not?
>>
> 
> The host_start/host_stop functions are assigned to the same struct
> ci_role_driver ops that udc_idc_switch_for_{device,host} are for the
> gadget role. Really, these things are called from the same place by the
> chipidea driver so not much is different between the two files I modify
> to make the mux calls. Furthermore, we don't want to do this if we have
> HNP or "true" OTG support so I've put it behind the ci_otg_is_fsm_mode()
> check to make sure we don't do any muxing stuff based on fsm state
> changes. It doesn't really make any sense here anyway because this
> device I have doesn't support OTG, just role switching.