linux-kernel - Re: [PATCH 1/2] usb: cdns3: fix cdns3_core_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <715c8f74-2790-6546-66ae-c0aea53946ed@ti.com>
Date:   Wed, 16 Oct 2019 12:20:49 +0300
From:   Roger Quadros <rogerq@...com>
To:     Pawel Laszczak <pawell@...ence.com>,
        "felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "peter.chen@....com" <peter.chen@....com>,
        "nsekhar@...com" <nsekhar@...com>,
        Rahul Kumar <kurahul@...ence.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] usb: cdns3: fix cdns3_core_init_role()

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

> 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@...ence.com>
> Tested-by: Pawel Laszczak <pawell@...ence.com>
> 
> --
> Regards,
> Pawel
> 

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