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

[Pawel Laszczak]

>
>
>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@xxxxxx>
>>> Signed-off-by: Sekhar Nori <nsekhar@xxxxxx>
>>> ---
>>> 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;

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@xxxxxxxxxxx>
>> Tested-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
>>
>> --
>> Regards,
>> Pawel
>>
>
>--
>cheers,
>-roger
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki