linux-kernel - RE: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.

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]

Date:   Thu, 9 Jan 2020 08:34:12 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "rogerq@...com" <rogerq@...com>,
        "jbergsagel@...com" <jbergsagel@...com>,
        "nsekhar@...com" <nsekhar@...com>, "nm@...com" <nm@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jayshri Dajiram Pawar <jpawar@...ence.com>,
        Rahul Kumar <kurahul@...ence.com>,
        Sanket Parmar <sparmar@...ence.com>,
        Peter Chan <peter.chan@....com>
Subject: RE: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect
 operation.

>
>On Thu, Jan 09, 2020 at 06:27:02AM +0000, Pawel Laszczak wrote:
>> >> +	writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd);
>> >> +
>> >> +	ret = readl_poll_timeout_atomic(&priv_dev->regs->ep_cmd, val,
>> >> +					!(val & (EP_CMD_CSTALL | EP_CMD_EPRST)),
>> >> +					1, 1000);
>> >> +
>> >> +	if (unlikely(ret))
>> >
>> >Unless you can measure the difference of using/not using a
>> >unlikely/likely mark, NEVER use it.  The compiler and cpu can almost
>> >always do better than you can, we have the tests to prove it.
>> >
>>
>> The both of the above timeout should never occur. If they occurred it would be a
>> critical controller bug. In this case driver can only inform  about this event.
>
>"Should never occur" is a fun thing to say :)
>
>If it can never occur, then don't even check for it.

Yes, on existing platforms it can never occur. 

>
>If it can, then check for it and handle it properly.
>
>What about this controller in systems with removable busses (like PCI?)
>What happens then (hint, I bet this could occur...)

It's good question.  Nobody from our customer currently use such system. 
The only platform with PCI is used by me for testing purpose.  
I will talk with  HW team about such cases. Maybe in the feature such improvement 
will be necessary.    
It's important fix for our customer so I will delete this checking for now. 

>
>> For timeouts used in driver I've never see an errors. Because debugging these
>> kind of errors is very hard I decided to leave dev_err in such case to inform that
>> probably something is wrong in HW project.
>>
>> I will remove unlikely.
>>
>> >> +		dev_err(priv_dev->dev, "Timeout: %s resetting failed.\n",
>> >> +			priv_ep->name);
>> >> +
>> >> +	priv_ep->flags |= EP_HW_RESETED;
>> >
>> >So an error happens, but you still claim the device is reset?  What can
>> >a user do about this error?
>>
>> The error should never occur.
>
>Then no need to warn anyone, just wait and move on.
>
>Or properly handle it.
>
>thanks,
>
>greg k-h

Thanks,

Pawell