Re: [PATCH v9 04/10] usb: dwc3: core: Skip setting event buffers for host only controllers

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

On 6/27/2023 5:16 AM, Thinh Nguyen wrote:
> On Mon, Jun 26, 2023, Thinh Nguyen wrote:
>> On Sat, Jun 24, 2023, Krishna Kurapati PSSNV wrote:
>>>
>>>
>>> On 6/24/2023 3:57 AM, Thinh Nguyen wrote:
>>>> On Wed, Jun 21, 2023, Krishna Kurapati wrote:
>>>>> On some SoC's like SA8295P where the tertiary controller is host-only
>>>>> capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible.
>>>>> Trying to access them leads to a crash.
>>>>>
>>>>> For DRD/Peripheral supported controllers, event buffer setup is done
>>>>> again in gadget_pullup. Skip setup or cleanup of event buffers if
>>>>> controller is host-only capable.
>>>>>
>>>>> Suggested-by: Johan Hovold <johan@kernel.org>
>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>> ---
>>>>>    drivers/usb/dwc3/core.c | 11 +++++++++++
>>>>>    1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 32ec05fc242b..e1ebae54454f 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc)
>>>>>    static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length)
>>>>>    {
>>>>>    	struct dwc3_event_buffer *evt;
>>>>> +	unsigned int hw_mode;
>>>>> +
>>>>> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>>>>> +	if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST)
>>>>> +		return 0;
>>>>
>>>> This is a little awkward. Returning 0 here indicates that this function
>>>> was successful, and the event buffers were allocated based on the
>>>> function name. Do this check outside of dwc3_alloc_one_event_buffer()
>>>> and specifically set dwc->ev_buf = NULL if that's the case.
>>>>
>>> Hi Thinh,
>>>
>>>    Apologies, I didn't understand the comment properly.
>>>
>>>    I thought we were supposed to return 0 here if we fulfill the goal of this
>>> function (allocate if we are drd/gadget and don't allocate if we are host
>>> mode only).
>>
>> The name of the function implies that it returns 0 if it allocated the
>> event buffer. If there are multiple conditions to the function returning
>> 0 here, then we should document it.
>>
>>>
>>>    If we return a non zero error here, probe would fail as this call will be
>>> done only for host only controllers during probe and nowhere else.
>>>
>>>    Are you suggesting we move this check to dwc3_alloc_one_event_buffer call
>>> ?
>>>
>>> Regards,
>>> Krishna,
>>>
>>
>> I'm suggesting to move the check to the caller of
>> dwc3_alloc_one_event_buffer(). Something like this so that it's
>> self-documented:
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 0beaab932e7d..bba82792bf6f 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1773,6 +1773,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>   	struct resource		*res, dwc_res;
>>   	void __iomem		*regs;
>>   	struct dwc3		*dwc;
>> +	unsigned int		hw_mode;
>>   	int			ret;
>>   
>>   	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
>> @@ -1854,11 +1855,16 @@ static int dwc3_probe(struct platform_device *pdev)
>>   
>>   	pm_runtime_forbid(dev);
>>   
>> -	ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
>> -	if (ret) {
>> -		dev_err(dwc->dev, "failed to allocate event buffers\n");
>> -		ret = -ENOMEM;
>> -		goto err_allow_rpm;
>> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>> +	if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
>> +		dwc->ev_buf = NULL;
>> +	} else {
>> +		ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
>> +		if (ret) {
>> +			dev_err(dwc->dev, "failed to allocate event buffers\n");
>> +			ret = -ENOMEM;
>> +			goto err_allow_rpm;
>> +		}
>>   	}
>>   
>>   	dwc->edev = dwc3_get_extcon(dwc);
>>
> 
> Actually, please ignore. there's already a document there, I missed that
> for some reason. What you did is fine. Though, I don't see the condition
> for ev_buf = NULL anywhere. Can you add that for clarity?
> 
> Thanks,
> Thinh

Hi Thinh,

  Did you mean adding "dwc->ev_buf = NULL" specifically ?

Regards,
Krishna,