Re: [PATCH 1/2] usb: cdns3: fix cdns3_core_init_role()

From: Roger Quadros <rogerq@ti.com>
To: Pawel Laszczak <pawell@cadence.com>,
	"felipe.balbi@linux.intel.com" <felipe.balbi@linux.intel.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: "peter.chen@nxp.com" <peter.chen@nxp.com>,
	"nsekhar@ti.com" <nsekhar@ti.com>,
	Rahul Kumar <kurahul@cadence.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] usb: cdns3: fix cdns3_core_init_role()
Date: Wed, 16 Oct 2019 12:54:38 +0300	[thread overview]
Message-ID: <3d2e7110-448f-d847-7668-bb20d7e03d22@ti.com> (raw)
In-Reply-To: <BYAPR07MB470923F80C4B49D26291B268DD920@BYAPR07MB4709.namprd07.prod.outlook.com>

On 16/10/2019 12:38, Pawel Laszczak wrote:
>>
>>
>> Hi Pawel,
>>
>> On 16/10/2019 07:32, Pawel Laszczak wrote:
>>> Hi Roger
>>>
>>>>
>>>> At startup we should trigger the HW state machine
>>>> only if it is OTG mode. Otherwise we should just
>>>> start the respective role.
>>>>
>>>> Initialize idle role by default. If we don't do this then
>>>> cdns3_idle_role_stop() is not called when switching to
>>>> host/device role and so lane switch mechanism
>>>> doesn't work. This results to super-speed device not working
>>>> in one orientation if it was plugged before driver probe.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>>> ---
>>>> drivers/usb/cdns3/core.c | 20 +++++++++++++++++++-
>>>> 1 file changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>> index 06f1e105be4e..1109dc5a4c39 100644
>>>> --- a/drivers/usb/cdns3/core.c
>>>> +++ b/drivers/usb/cdns3/core.c
>>>> @@ -160,10 +160,28 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>>> 	if (ret)
>>>> 		goto err;
>>>>
>>>> -	if (cdns->dr_mode != USB_DR_MODE_OTG) {
>>>> +	/* Initialize idle role to start with */
>>>> +	ret = cdns3_role_start(cdns, USB_ROLE_NONE);
>>>> +	if (ret)
>>>> +		goto err;
>>>> +
>>>> +	switch (cdns->dr_mode) {
>>>> +	case USB_DR_MODE_UNKNOWN:
>>>
>>> One note in this place. USB_DR_MODE_UNKNOWN is not possible in this place.
>>> If cdns->dr_mode will be USB_DR_MODE_UNKNOWN then driver returns -EINVAL
>>
>> At which place? I could not find.
> 
> In this patch we can't see this line:
> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/usb/cdns3/core.c#L159
> There is called  cdns3_drd_update_mode(cdns);
> 
> int cdns3_drd_update_mode(struct cdns3 *cdns)
> {
> 	int ret = 0;
> 
> 	switch (cdns->dr_mode) {
> 	case USB_DR_MODE_PERIPHERAL:
> 		ret = cdns3_set_mode(cdns, USB_DR_MODE_PERIPHERAL);
> 		break;
> 	case USB_DR_MODE_HOST:
> 		ret = cdns3_set_mode(cdns, USB_DR_MODE_HOST);
> 		break;
> 	case USB_DR_MODE_OTG:
> 		ret = cdns3_init_otg_mode(cdns);
> 		break;
> 	default:
> 		dev_err(cdns->dev, "Unsupported mode of operation %d\n",
> 			cdns->dr_mode);
> 		return -EINVAL;
> 	}
> 
> 	return ret;
> }
> 
> After calling cdns3_drd_update_mode  we have 2 first lines from this patch
> 	if (ret)
> 		goto err;

I see now. I will update this patch to error out if dr_mode is USB_DR_MODE_UNKNOWN.
Thanks.

cheers,
-roger

> 
> Pawel
>>
>>> some line before after returning form cdns3_drd_update_mode and in consequence
>>> it jump to err label.
>>>
>>> Maybe for better readability it this condition should be treated here also as error.
>>>
>>>> +	case USB_DR_MODE_OTG:
>>>> 		ret = cdns3_hw_role_switch(cdns);
>>>> 		if (ret)
>>>> 			goto err;
>>>> +		break;
>>>> +	case USB_DR_MODE_PERIPHERAL:
>>>> +		ret = cdns3_role_start(cdns, USB_ROLE_DEVICE);
>>>> +		if (ret)
>>>> +			goto err;
>>>> +		break;
>>>> +	case USB_DR_MODE_HOST:
>>>> +		ret = cdns3_role_start(cdns, USB_ROLE_HOST);
>>>> +		if (ret)
>>>> +			goto err;
>>>> +		break;
>>>> 	}
>>>>
>>>> 	return ret;
>>>
>>> Reviewed-by: Pawel Laszczak <pawell@cadence.com>
>>> Tested-by: Pawel Laszczak <pawell@cadence.com>
>>>
>>> --
>>> Regards,
>>> Pawel
>>>
>>
>> --
>> cheers,
>> -roger
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki