Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* RE: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.
       [not found] ` <20200108142250.GA2383861@kroah.com>
@ 2020-01-09  4:16   ` Pawel Laszczak
  0 siblings, 0 replies; 8+ messages in thread
From: Pawel Laszczak @ 2020-01-09  4:16 UTC (permalink / raw)
  To: Greg KH
  Cc: felipe.balbi, linux-usb, rogerq, jbergsagel, nsekhar, nm,
	linux-kernel, Jayshri Dajiram Pawar, Rahul Kumar, Sanket Parmar,
	Peter Chan

Hi,

>
>
>On Wed, Jan 08, 2020 at 12:37:18PM +0100, Pawel Laszczak wrote:
>> The ARM core hang when access USB register after tens of thousands
>> connect/disconnect operation.
>>
>> The issue was observed on platform with android system and is not easy
>> to reproduce. During test controller works at HS device mode with host
>> connected.
>>
>> The test is based on continuous disabling/enabling USB device function
>> what cause continuous setting DEVDS/DEVEN bit in USB_CONF register.
>>
>> For testing was used composite device consisting from ADP and RNDIS
>> function.
>>
>> Presumably the problem was caused by DMA transfer made after setting
>> DEVDS bit. To resolve this issue fix stops all DMA transfer before
>> setting DEVDS bit.
>>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> Signed-off-by: Peter Chan <peter.chan@nxp.com>
>> Reported-by: Peter Chan <peter.chan@nxp.com>
>> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>> ---
>>  drivers/usb/cdns3/gadget.c | 84 ++++++++++++++++++++++++++------------
>>  drivers/usb/cdns3/gadget.h |  1 +
>>  2 files changed, 58 insertions(+), 27 deletions(-)
>
>Any reason to forget linux-usb@vger.kernel.org for usb patches?

No reason. I missed it. 
+ linux-usb@vger.kernel.org

Thanks,
Pawell

>
>thanks,
>
>greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.
       [not found] ` <20200108142829.GB2383861@kroah.com>
@ 2020-01-09  6:27   ` Pawel Laszczak
  2020-01-09  6:38     ` Greg KH
  0 siblings, 1 reply; 8+ messages in thread
From: Pawel Laszczak @ 2020-01-09  6:27 UTC (permalink / raw)
  To: Greg KH
  Cc: felipe.balbi, linux-usb, rogerq, jbergsagel, nsekhar, nm,
	linux-kernel, Jayshri Dajiram Pawar, Rahul Kumar, Sanket Parmar,
	Peter Chan

Hi Greg,

+ linux-usb@vger.kernel.org

>
>On Wed, Jan 08, 2020 at 12:37:18PM +0100, Pawel Laszczak wrote:
>> The ARM core hang when access USB register after tens of thousands
>> connect/disconnect operation.
>>
>> The issue was observed on platform with android system and is not easy
>> to reproduce. During test controller works at HS device mode with host
>> connected.
>>
>> The test is based on continuous disabling/enabling USB device function
>> what cause continuous setting DEVDS/DEVEN bit in USB_CONF register.
>>
>> For testing was used composite device consisting from ADP and RNDIS
>> function.
>>
>> Presumably the problem was caused by DMA transfer made after setting
>> DEVDS bit. To resolve this issue fix stops all DMA transfer before
>> setting DEVDS bit.
>>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> Signed-off-by: Peter Chan <peter.chan@nxp.com>
>> Reported-by: Peter Chan <peter.chan@nxp.com>
>> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>
>As this is in the 5.4 kernel release, you should have a "Cc: stable..."
>line in here, right?

Ok,

>
>> ---
>>  drivers/usb/cdns3/gadget.c | 84 ++++++++++++++++++++++++++------------
>>  drivers/usb/cdns3/gadget.h |  1 +
>>  2 files changed, 58 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>> index 4c1e75509303..277ed8484032 100644
>> --- a/drivers/usb/cdns3/gadget.c
>> +++ b/drivers/usb/cdns3/gadget.c
>> @@ -1516,6 +1516,49 @@ static int cdns3_ep_onchip_buffer_reserve(struct cdns3_device *priv_dev,
>>  	return 0;
>>  }
>>
>> +static int cdns3_disable_reset_ep(struct cdns3_device *priv_dev,
>> +				  struct cdns3_endpoint *priv_ep)
>> +{
>> +	unsigned long flags;
>> +	u32 val;
>> +	int ret;
>> +
>> +	spin_lock_irqsave(&priv_dev->lock, flags);
>> +
>> +	if (priv_ep->flags & EP_HW_RESETED) {
>
>Is "reseted" a word?  :)
>
>> +		spin_unlock_irqrestore(&priv_dev->lock, flags);
>> +		return 0;
>> +	}
>> +
>> +	cdns3_select_ep(priv_dev, priv_ep->endpoint.desc->bEndpointAddress);
>> +
>> +	val = readl(&priv_dev->regs->ep_cfg);
>> +	val &= ~EP_CFG_ENABLE;
>> +	writel(val, &priv_dev->regs->ep_cfg);
>> +
>> +	/**
>
>No need for kernel-doc comment style please.
>
>> +	 * Driver needs some time before resetting endpoint.
>> +	 * It need waits for clearing DBUSY bit or for timeout expired.
>> +	 * 10us is enough time for controller to stop transfer.
>> +	 */
>> +	readl_poll_timeout_atomic(&priv_dev->regs->ep_sts, val,
>> +				  !(val & EP_STS_DBUSY), 1, 10);
>
>You don't care if you time out?
I don't need care of it. The next timeout is critical.
>
>> +	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. 
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. 

>
>Yes, I know you copied this from other code in this driver, but it
>doesn't look right to me.

>
>> +	spin_unlock_irqrestore(&priv_dev->lock, flags);
>
>Why print while a spinlock is held?  That's mean, you could have printed
>here, right?
>

Yes, it's true. 

>> +
>> +	return ret;
>> +}
>> +
>>  void cdns3_configure_dmult(struct cdns3_device *priv_dev,
>>  			   struct cdns3_endpoint *priv_ep)
>>  {
>> @@ -1893,8 +1936,6 @@ static int cdns3_gadget_ep_disable(struct usb_ep *ep)
>>  	struct usb_request *request;
>>  	unsigned long flags;
>>  	int ret = 0;
>> -	u32 ep_cfg;
>> -	int val;
>>
>>  	if (!ep) {
>>  		pr_err("usbss: invalid parameters\n");
>> @@ -1908,32 +1949,11 @@ static int cdns3_gadget_ep_disable(struct usb_ep *ep)
>>  			  "%s is already disabled\n", priv_ep->name))
>>  		return 0;
>>
>> -	spin_lock_irqsave(&priv_dev->lock, flags);
>> -
>>  	trace_cdns3_gadget_ep_disable(priv_ep);
>>
>> -	cdns3_select_ep(priv_dev, ep->desc->bEndpointAddress);
>> -
>> -	ep_cfg = readl(&priv_dev->regs->ep_cfg);
>> -	ep_cfg &= ~EP_CFG_ENABLE;
>> -	writel(ep_cfg, &priv_dev->regs->ep_cfg);
>> -
>> -	/**
>> -	 * Driver needs some time before resetting endpoint.
>> -	 * It need waits for clearing DBUSY bit or for timeout expired.
>> -	 * 10us is enough time for controller to stop transfer.
>> -	 */
>> -	readl_poll_timeout_atomic(&priv_dev->regs->ep_sts, val,
>> -				  !(val & EP_STS_DBUSY), 1, 10);
>> -	writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd);
>> -
>> -	readl_poll_timeout_atomic(&priv_dev->regs->ep_cmd, val,
>> -				  !(val & (EP_CMD_CSTALL | EP_CMD_EPRST)),
>> -				  1, 1000);
>> -	if (unlikely(ret))
>> -		dev_err(priv_dev->dev, "Timeout: %s resetting failed.\n",
>> -			priv_ep->name);
>> +	cdns3_disable_reset_ep(priv_dev, priv_ep);
>>
>> +	spin_lock_irqsave(&priv_dev->lock, flags);
>>  	while (!list_empty(&priv_ep->pending_req_list)) {
>>  		request = cdns3_next_request(&priv_ep->pending_req_list);
>>
>> @@ -1962,6 +1982,7 @@ static int cdns3_gadget_ep_disable(struct usb_ep *ep)
>>
>>  	ep->desc = NULL;
>>  	priv_ep->flags &= ~EP_ENABLED;
>> +	priv_ep->flags |= EP_CLAIMED | EP_HW_RESETED;
>
>Why do you now set EP_CLAIMED here when you never set that before?  Is
>this a different type of change?

My mistake. Thanks for letting me know. I merged two different patches fixing the same issue and 
I missed it. 

>
>thanks,
>
>greg k-h

Thanks,
Pawell 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.
  2020-01-09  6:27   ` Pawel Laszczak
@ 2020-01-09  6:38     ` Greg KH
  2020-01-09  8:34       ` Pawel Laszczak
  0 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2020-01-09  6:38 UTC (permalink / raw)
  To: Pawel Laszczak
  Cc: felipe.balbi, linux-usb, rogerq, jbergsagel, nsekhar, nm,
	linux-kernel, Jayshri Dajiram Pawar, Rahul Kumar, Sanket Parmar,
	Peter Chan

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.

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...)

> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.
  2020-01-09  6:38     ` Greg KH
@ 2020-01-09  8:34       ` Pawel Laszczak
  2020-01-09  9:38         ` Greg KH
  0 siblings, 1 reply; 8+ messages in thread
From: Pawel Laszczak @ 2020-01-09  8:34 UTC (permalink / raw)
  To: Greg KH
  Cc: felipe.balbi, linux-usb, rogerq, jbergsagel, nsekhar, nm,
	linux-kernel, Jayshri Dajiram Pawar, Rahul Kumar, Sanket Parmar,
	Peter Chan

>
>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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.
  2020-01-09  8:34       ` Pawel Laszczak
@ 2020-01-09  9:38         ` Greg KH
  2020-01-14  8:57           ` Peter Chen
  0 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2020-01-09  9:38 UTC (permalink / raw)
  To: Pawel Laszczak
  Cc: felipe.balbi, linux-usb, rogerq, jbergsagel, nsekhar, nm,
	linux-kernel, Jayshri Dajiram Pawar, Rahul Kumar, Sanket Parmar,
	Peter Chan

On Thu, Jan 09, 2020 at 08:34:12AM +0000, Pawel Laszczak wrote:
> >
> >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.  

So if you do have a PCI device, then you need to handle PCI reads
failing and returning all 1s.  Hopefully you can gracefully handle this :)

Adding timeout handling here, where it is totally obvious to do so,
would be a good thing.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.
  2020-01-09  9:38         ` Greg KH
@ 2020-01-14  8:57           ` Peter Chen
  2020-01-14  9:06             ` Pawel Laszczak
  2020-01-14  9:09             ` Pawel Laszczak
  0 siblings, 2 replies; 8+ messages in thread
From: Peter Chen @ 2020-01-14  8:57 UTC (permalink / raw)
  To: Greg KH
  Cc: Pawel Laszczak, felipe.balbi, linux-usb, rogerq, jbergsagel,
	nsekhar, nm, linux-kernel, Jayshri Dajiram Pawar, Rahul Kumar,
	Sanket Parmar, Peter Chan

On Thu, Jan 9, 2020 at 5:39 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 09, 2020 at 08:34:12AM +0000, Pawel Laszczak wrote:
> > >
> > >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.
>
> So if you do have a PCI device, then you need to handle PCI reads
> failing and returning all 1s.  Hopefully you can gracefully handle this :)
>
> Adding timeout handling here, where it is totally obvious to do so,
> would be a good thing.
>
> thanks,
>
> greg k-h

Hi Pawel,

My email is: peter.chen@nxp.com, please change it when your send next version.

Peter

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.
  2020-01-14  8:57           ` Peter Chen
@ 2020-01-14  9:06             ` Pawel Laszczak
  2020-01-14  9:09             ` Pawel Laszczak
  1 sibling, 0 replies; 8+ messages in thread
From: Pawel Laszczak @ 2020-01-14  9:06 UTC (permalink / raw)
  To: Peter Chen, Greg KH
  Cc: felipe.balbi, linux-usb, rogerq, jbergsagel, nsekhar, nm,
	linux-kernel, Jayshri Dajiram Pawar, Rahul Kumar, Sanket Parmar,
	Peter Chan

>
>On Thu, Jan 9, 2020 at 5:39 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> On Thu, Jan 09, 2020 at 08:34:12AM +0000, Pawel Laszczak wrote:
>> > >
>> > >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.
>>
>> So if you do have a PCI device, then you need to handle PCI reads
>> failing and returning all 1s.  Hopefully you can gracefully handle this :)
>>
>> Adding timeout handling here, where it is totally obvious to do so,
>> would be a good thing.
>>
>> thanks,
>>
>> greg k-h
>
>Hi Pawel,
>
>My email is: peter.chen@nxp.com, please change it when your send next version.

Hi Peter, 

I'm sorry for that.

BTW. I need postpone sending the next version of this patch because there is some issue with synchronization of repositories  regarding the patch "usb: cdns3: Add streams support to cadence USB3 DRD driver"
I don't want generate new conflicts. 

Regards,
Pawell
>
>Peter

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.
  2020-01-14  8:57           ` Peter Chen
  2020-01-14  9:06             ` Pawel Laszczak
@ 2020-01-14  9:09             ` Pawel Laszczak
  1 sibling, 0 replies; 8+ messages in thread
From: Pawel Laszczak @ 2020-01-14  9:09 UTC (permalink / raw)
  To: Peter Chen, Greg KH
  Cc: felipe.balbi, linux-usb, rogerq, jbergsagel, nsekhar, nm,
	linux-kernel, Jayshri Dajiram Pawar, Rahul Kumar, Sanket Parmar,
	peter.chen

>
>On Thu, Jan 9, 2020 at 5:39 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> On Thu, Jan 09, 2020 at 08:34:12AM +0000, Pawel Laszczak wrote:
>> > >
>> > >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.
>>
>> So if you do have a PCI device, then you need to handle PCI reads
>> failing and returning all 1s.  Hopefully you can gracefully handle this :)
>>
>> Adding timeout handling here, where it is totally obvious to do so,
>> would be a good thing.
>>
>> thanks,
>>
>> greg k-h
>
>Hi Pawel,
>
>My email is: peter.chen@nxp.com, please change it when your send next version.

Hi Peter, 

I'm sorry for that.

BTW. I need postpone sending the next version of this patch because there is some issue with synchronization of repositories  regarding the patch "usb: cdns3: Add streams support to cadence USB3 DRD driver"
I don't want generate new conflicts. 

Regards,
Pawell

>
>Peter



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200108113719.21551-1-pawell@cadence.com>
     [not found] ` <20200108142250.GA2383861@kroah.com>
2020-01-09  4:16   ` [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation Pawel Laszczak
     [not found] ` <20200108142829.GB2383861@kroah.com>
2020-01-09  6:27   ` Pawel Laszczak
2020-01-09  6:38     ` Greg KH
2020-01-09  8:34       ` Pawel Laszczak
2020-01-09  9:38         ` Greg KH
2020-01-14  8:57           ` Peter Chen
2020-01-14  9:06             ` Pawel Laszczak
2020-01-14  9:09             ` Pawel Laszczak

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git