Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <3be5e95f-85d2-4abf-a8b4-18b019341602@quicinc.com>
Date:   Sat, 4 Nov 2023 00:19:08 +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
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
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>
 <5ef66bdc-9645-4bbe-8182-baa7fe4c583a@quicinc.com>
In-Reply-To: <5ef66bdc-9645-4bbe-8182-baa7fe4c583a@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 11/4/2023 12:15 AM, Krishna Kurapati PSSNV wrote:
> 
> 
> 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 ?
> 

Just to clarify one more thing. The probe completion requirement came in 
because, before the device tree was flattened, dwc3-qcom and core are 
two different platform devices. And if the dwc3 core device probe got 
deferred, dwc3-qcom probe still gets successfully completed. The glue 
would never know when to register vendor hook callbacks to dwc3-core as 
it would never know when the core probe was completed.

That is the reason we wanted to find out accurate point where core probe 
is done to ensure we can properly register these callbacks.

Regards,
Krishna,