Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <5ef66bdc-9645-4bbe-8182-baa7fe4c583a@quicinc.com>
Date:   Sat, 4 Nov 2023 00:15:20 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC 2/8] usb: dwc3: core: Register vendor hooks for dwc3-qcom
Content-Language: en-US
To:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        "Conor Dooley" <conor+dt@kernel.org>
CC:     <linux-usb@vger.kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        <linux-kernel@vger.kernel.org>,
        "Thinh Nguyen" <Thinh.Nguyen@synopsys.com>,
        Rob Herring <robh+dt@kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <quic_ppratap@quicinc.com>, <quic_jackp@quicinc.com>,
        <quic_wcheng@quicinc.com>, Andy Gross <agross@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Philipp Zabel <p.zabel@pengutronix.de>
References: <20231017131851.8299-1-quic_kriskura@quicinc.com>
 <20231017131851.8299-3-quic_kriskura@quicinc.com>
 <e700133b-58f7-4a4d-8e5c-0d04441b789b@linaro.org>
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <e700133b-58f7-4a4d-8e5c-0d04441b789b@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/3/2023 8:44 PM, Bryan O'Donoghue wrote:
> On 17/10/2023 14:18, Krishna Kurapati wrote:
>>
>> The following are the requirements aimed in this implementation:
>>
>> 1. When enum in device mode, Glue/core must stay active.
>>
>> 2. When cable is connected but UDC is not written yet, then glue/core
>> must be suspended.
>>
>> 3. Upon removing cable in device mode, the disconnect event must be
>> generated and unblock runtime suspend for dwc3 core.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> 

Hi Bryan,

> What happens to this code if you
> 
> static int count;
> 
> 1. sleep in dwc3_probe for 10 milliseconds
> 2. return -EPROBE_DEFER
> 3. if count++ < 5 goto 1
> 
> i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe()
> 
The vendor hooks are used in __dwc3_set_mode and role_switch_set calls 
in core and drd files respectively. These are invoked only if we are OTG 
capable. The drd_work is initialized in core_init_mode which is called 
at the end of dwc3_probe. If dwc3_probe fails and gets deferred before 
that, none of the vendor hooks will be fired and dwc3_qcom_probe is also 
deferred.

However I see that if core_init_mode fails (the cleanup is already done 
in drd to prevent set_role from getting invoked already),  I need to 
cleanup vendor hooks in error path of dwc3_probe().

> and what happens if we introduce a 100 millsecond sleep into 
> dwc3_qcom_probe() - and run a fake disconnect event from 
> dwc3_qcom_probe_core() directly ?
> 
> In other words if make it that dwc3_probe() completes and struct 
> dwc3_glue_ops->notify_cable_disconnect() fires prior to 
> dwc3_qcom_probe_core() completing ?
> 
> i.e. I don't immediately see how you've solved the probe() completion 
> race condition here.
> 
Just wanted to understand the situation clearly. Is this the sequence 
you are referring to ?

1. dwc3_probe is successful and role switch is registered properly.
2. added delay after dwc3_qcom_probe_core and before interconnect_init
3. Between this delay, we got a disconnect notificiation from glink
4. We are clearing the qscratch reg in case of device mode and 
un-registering notifier in case of host mode.

If so, firstly I don't see any issue if we process disconnect event 
before qcom probe is complete. If we reached this stage, the clocks/gdsc 
is definitely ON and register accesses are good to go.

If we are in host mode at this point, we would just unregister to 
usb-core notifier and mark last busy. If we are in device mode, we would 
just clear the hs_phy_ctrl reg of qscratch. After the 100ms delay you 
mentioned we would call dwc3_remove anyways and cleanup the vendor 
hooks. But is the concern here that, what if we enter runtime_suspend at 
this point ?

Regards,
Krishna,