* [RFC PATCH] USB: xhci: Enable HCE event reset function @ 2020-11-18 3:36 Longfang Liu 2020-11-18 7:02 ` Greg KH 0 siblings, 1 reply; 8+ messages in thread From: Longfang Liu @ 2020-11-18 3:36 UTC (permalink / raw) To: gregkh; +Cc: linux-usb, yisen.zhuang, tanxiaofei The HCE(Host Controller Error) event has been defined in the XHCI driver but has not been used. If we want to use the HCE event to reset the controller, can we implement it in the interrupt function as follows: xhci_irq() |----xhci_halt() |----xhci_shutdown() |----xhci_start() |----xhci_run() Signed-off-by: Longfang Liu <liulongfang@huawei.com> --- drivers/usb/host/xhci-ring.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 2c255d0..87b3a40 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -2857,6 +2857,16 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) goto out; } + if (status & STS_HCE) { + xhci_warn(xhci, "WARNING: Host Controller Error\n"); + xhci_halt(xhci); + xhci_shutdown(hcd); + xhci_start(xhci); + xhci_run(hcd); + ret = IRQ_HANDLED; + goto out; + } + /* * Clear the op reg interrupt status first, * so we can receive interrupts from other MSI-X interrupters. -- 2.8.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] USB: xhci: Enable HCE event reset function 2020-11-18 3:36 [RFC PATCH] USB: xhci: Enable HCE event reset function Longfang Liu @ 2020-11-18 7:02 ` Greg KH 2020-11-18 9:04 ` liulongfang 0 siblings, 1 reply; 8+ messages in thread From: Greg KH @ 2020-11-18 7:02 UTC (permalink / raw) To: Longfang Liu; +Cc: linux-usb, yisen.zhuang, tanxiaofei On Wed, Nov 18, 2020 at 11:36:13AM +0800, Longfang Liu wrote: > The HCE(Host Controller Error) event has been defined in > the XHCI driver but has not been used. If we want to use > the HCE event to reset the controller, can we implement > it in the interrupt function as follows: > > xhci_irq() > |----xhci_halt() > |----xhci_shutdown() > |----xhci_start() > |----xhci_run() > > Signed-off-by: Longfang Liu <liulongfang@huawei.com> > --- > drivers/usb/host/xhci-ring.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) $ ./scripts/get_maintainer.pl --file drivers/usb/host/xhci-ring.c Mathias Nyman <mathias.nyman@intel.com> (supporter:USB XHCI DRIVER) Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) linux-usb@vger.kernel.org (open list:USB XHCI DRIVER) linux-kernel@vger.kernel.org (open list) Any reason to not include the maintainer of the xhci driver here? > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 2c255d0..87b3a40 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -2857,6 +2857,16 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) > goto out; > } > > + if (status & STS_HCE) { > + xhci_warn(xhci, "WARNING: Host Controller Error\n"); > + xhci_halt(xhci); > + xhci_shutdown(hcd); > + xhci_start(xhci); > + xhci_run(hcd); > + ret = IRQ_HANDLED; > + goto out; > + } > + Does this fix a real problem for you? Are you sure we will not suddenly start resetting devices that were working properly and sending this error incorrectly? How did you test this? thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] USB: xhci: Enable HCE event reset function 2020-11-18 7:02 ` Greg KH @ 2020-11-18 9:04 ` liulongfang 2020-11-18 9:14 ` Greg KH 0 siblings, 1 reply; 8+ messages in thread From: liulongfang @ 2020-11-18 9:04 UTC (permalink / raw) To: Greg KH; +Cc: linux-usb, yisen.zhuang, tanxiaofei, mathias.nyman On 2020/11/18 15:02, Greg KH Wrote: > On Wed, Nov 18, 2020 at 11:36:13AM +0800, Longfang Liu wrote: >> The HCE(Host Controller Error) event has been defined in >> the XHCI driver but has not been used. If we want to use >> the HCE event to reset the controller, can we implement >> it in the interrupt function as follows: >> >> xhci_irq() >> |----xhci_halt() >> |----xhci_shutdown() >> |----xhci_start() >> |----xhci_run() >> >> Signed-off-by: Longfang Liu <liulongfang@huawei.com> >> --- >> drivers/usb/host/xhci-ring.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) > > $ ./scripts/get_maintainer.pl --file drivers/usb/host/xhci-ring.c > Mathias Nyman <mathias.nyman@intel.com> (supporter:USB XHCI DRIVER) > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) > linux-usb@vger.kernel.org (open list:USB XHCI DRIVER) > linux-kernel@vger.kernel.org (open list) > > Any reason to not include the maintainer of the xhci driver here? OK, I will include the maintainer in the next patch. > >> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >> index 2c255d0..87b3a40 100644 >> --- a/drivers/usb/host/xhci-ring.c >> +++ b/drivers/usb/host/xhci-ring.c >> @@ -2857,6 +2857,16 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) >> goto out; >> } >> >> + if (status & STS_HCE) { >> + xhci_warn(xhci, "WARNING: Host Controller Error\n"); >> + xhci_halt(xhci); >> + xhci_shutdown(hcd); >> + xhci_start(xhci); >> + xhci_run(hcd); >> + ret = IRQ_HANDLED; >> + goto out; >> + } >> + > > Does this fix a real problem for you? Are you sure we will not suddenly > start resetting devices that were working properly and sending this > error incorrectly? How did you test this? > > thanks, > > greg k-h > . > Yes, we want to add a RAS feature to the USB, Use the HCE event to trigger the reset operation of the USB controller. By searching for the current xhci driver, the driver did not handle HCE event. In fact, I am not sure if other operations will cause HCE, The HCE event is used to reset the USB controller according to the definition of the event. I test this reset operation through the Sysfs file, but have not tested all usage scenarios. Thanks. Longfang ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] USB: xhci: Enable HCE event reset function 2020-11-18 9:04 ` liulongfang @ 2020-11-18 9:14 ` Greg KH 2020-11-18 16:21 ` Mathias Nyman 0 siblings, 1 reply; 8+ messages in thread From: Greg KH @ 2020-11-18 9:14 UTC (permalink / raw) To: liulongfang; +Cc: linux-usb, yisen.zhuang, tanxiaofei, mathias.nyman On Wed, Nov 18, 2020 at 05:04:36PM +0800, liulongfang wrote: > On 2020/11/18 15:02, Greg KH Wrote: > > On Wed, Nov 18, 2020 at 11:36:13AM +0800, Longfang Liu wrote: > >> The HCE(Host Controller Error) event has been defined in > >> the XHCI driver but has not been used. If we want to use > >> the HCE event to reset the controller, can we implement > >> it in the interrupt function as follows: > >> > >> xhci_irq() > >> |----xhci_halt() > >> |----xhci_shutdown() > >> |----xhci_start() > >> |----xhci_run() > >> > >> Signed-off-by: Longfang Liu <liulongfang@huawei.com> > >> --- > >> drivers/usb/host/xhci-ring.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > > > > $ ./scripts/get_maintainer.pl --file drivers/usb/host/xhci-ring.c > > Mathias Nyman <mathias.nyman@intel.com> (supporter:USB XHCI DRIVER) > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) > > linux-usb@vger.kernel.org (open list:USB XHCI DRIVER) > > linux-kernel@vger.kernel.org (open list) > > > > Any reason to not include the maintainer of the xhci driver here? > OK, I will include the maintainer in the next patch. > > > >> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > >> index 2c255d0..87b3a40 100644 > >> --- a/drivers/usb/host/xhci-ring.c > >> +++ b/drivers/usb/host/xhci-ring.c > >> @@ -2857,6 +2857,16 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) > >> goto out; > >> } > >> > >> + if (status & STS_HCE) { > >> + xhci_warn(xhci, "WARNING: Host Controller Error\n"); > >> + xhci_halt(xhci); > >> + xhci_shutdown(hcd); > >> + xhci_start(xhci); > >> + xhci_run(hcd); > >> + ret = IRQ_HANDLED; > >> + goto out; > >> + } > >> + > > > > Does this fix a real problem for you? Are you sure we will not suddenly > > start resetting devices that were working properly and sending this > > error incorrectly? How did you test this? > > > > thanks, > > > > greg k-h > > . > > > Yes, we want to add a RAS feature to the USB, What is "RAS"? > Use the HCE event to trigger the reset operation of the USB controller. Is that allowed by the XHCI specification? > By searching for the current xhci driver, the driver did not handle HCE event. > In fact, I am not sure if other operations will cause HCE, The HCE event is > used to reset the USB controller according to the definition of the event. What generates that event? Do existing controllers do that today? What causes it? > I test this reset operation through the Sysfs file, but have not tested all usage scenarios. What sysfs file? thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] USB: xhci: Enable HCE event reset function 2020-11-18 9:14 ` Greg KH @ 2020-11-18 16:21 ` Mathias Nyman 2020-11-19 1:50 ` liulongfang 0 siblings, 1 reply; 8+ messages in thread From: Mathias Nyman @ 2020-11-18 16:21 UTC (permalink / raw) To: Greg KH, liulongfang; +Cc: linux-usb, yisen.zhuang, tanxiaofei, mathias.nyman On 18.11.2020 11.14, Greg KH wrote: > On Wed, Nov 18, 2020 at 05:04:36PM +0800, liulongfang wrote: >> On 2020/11/18 15:02, Greg KH Wrote: >>> On Wed, Nov 18, 2020 at 11:36:13AM +0800, Longfang Liu wrote: >>>> The HCE(Host Controller Error) event has been defined in >>>> the XHCI driver but has not been used. If we want to use >>>> the HCE event to reset the controller, can we implement >>>> it in the interrupt function as follows: >>>> >>>> xhci_irq() >>>> |----xhci_halt() >>>> |----xhci_shutdown() >>>> |----xhci_start() >>>> |----xhci_run() >>>> >>>> Signed-off-by: Longfang Liu <liulongfang@huawei.com> >>>> --- >>>> drivers/usb/host/xhci-ring.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>> >>> $ ./scripts/get_maintainer.pl --file drivers/usb/host/xhci-ring.c >>> Mathias Nyman <mathias.nyman@intel.com> (supporter:USB XHCI DRIVER) >>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) >>> linux-usb@vger.kernel.org (open list:USB XHCI DRIVER) >>> linux-kernel@vger.kernel.org (open list) >>> >>> Any reason to not include the maintainer of the xhci driver here? >> OK, I will include the maintainer in the next patch. >>> >>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >>>> index 2c255d0..87b3a40 100644 >>>> --- a/drivers/usb/host/xhci-ring.c >>>> +++ b/drivers/usb/host/xhci-ring.c >>>> @@ -2857,6 +2857,16 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) >>>> goto out; >>>> } >>>> >>>> + if (status & STS_HCE) { >>>> + xhci_warn(xhci, "WARNING: Host Controller Error\n"); >>>> + xhci_halt(xhci); >>>> + xhci_shutdown(hcd); >>>> + xhci_start(xhci); >>>> + xhci_run(hcd); >>>> + ret = IRQ_HANDLED; >>>> + goto out; >>>> + } This won't work at all. It doesn't reset the xHC which is the one thing needed to recover from a HCE. This would deadlock immediately. Many of the above functions shouldn't be called from interrupt context, and if HCE is set we probably even won't get any interrupts. A reset will set all registers to their initial values, and need to be re-initialized. Nothing is freed or re-allocated, registrs are not re-initialized here. Please revisit the xhci spec about HCE, and HCRST, and a more detail look at the xhci driver, and test the code before submitting. >>>> + >>> >>> Does this fix a real problem for you? Are you sure we will not suddenly >>> start resetting devices that were working properly and sending this >>> error incorrectly? How did you test this? >>> >>> thanks, >>> >>> greg k-h >>> . >>> >> Yes, we want to add a RAS feature to the USB, > > What is "RAS"? > >> Use the HCE event to trigger the reset operation of the USB controller. > > Is that allowed by the XHCI specification? > >> By searching for the current xhci driver, the driver did not handle HCE event. >> In fact, I am not sure if other operations will cause HCE, The HCE event is >> used to reset the USB controller according to the definition of the event. > > What generates that event? Do existing controllers do that today? What > causes it? > It's not an event, it's a controller internal error state. It's probably not very useful to check for it in the interrupt handler as the xHC hw ceases all activity when it sets the HCE bit, including sending interrupts. From the spec: 4.24.1 Internal Errors The Host Controller Error (HCE) flag is asserted when an internal xHC error is detected that exclusively affects the xHC. When the HCE flag is set to ‘1’ the xHC shall cease all activity. Software response to the assertion of HCE is to reset the xHC (HCRST = ‘1’) and reinitialize it >> I test this reset operation through the Sysfs file, but have not tested all usage scenarios. > > What sysfs file? I'm also interested in this, and to know more about RAS? Thanks -Mathias ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] USB: xhci: Enable HCE event reset function 2020-11-18 16:21 ` Mathias Nyman @ 2020-11-19 1:50 ` liulongfang 2020-11-19 6:09 ` Greg KH 0 siblings, 1 reply; 8+ messages in thread From: liulongfang @ 2020-11-19 1:50 UTC (permalink / raw) To: Mathias Nyman, Greg KH; +Cc: linux-usb, yisen.zhuang, tanxiaofei, mathias.nyman On 2020/11/19 0:21, Mathias Nyman Wrote: > On 18.11.2020 11.14, Greg KH wrote: >> On Wed, Nov 18, 2020 at 05:04:36PM +0800, liulongfang wrote: >>> On 2020/11/18 15:02, Greg KH Wrote: >>>> On Wed, Nov 18, 2020 at 11:36:13AM +0800, Longfang Liu wrote: >>>>> The HCE(Host Controller Error) event has been defined in >>>>> the XHCI driver but has not been used. If we want to use >>>>> the HCE event to reset the controller, can we implement >>>>> it in the interrupt function as follows: >>>>> >>>>> xhci_irq() >>>>> |----xhci_halt() >>>>> |----xhci_shutdown() >>>>> |----xhci_start() >>>>> |----xhci_run() >>>>> >>>>> Signed-off-by: Longfang Liu <liulongfang@huawei.com> >>>>> --- >>>>> drivers/usb/host/xhci-ring.c | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>> >>>> $ ./scripts/get_maintainer.pl --file drivers/usb/host/xhci-ring.c >>>> Mathias Nyman <mathias.nyman@intel.com> (supporter:USB XHCI DRIVER) >>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) >>>> linux-usb@vger.kernel.org (open list:USB XHCI DRIVER) >>>> linux-kernel@vger.kernel.org (open list) >>>> >>>> Any reason to not include the maintainer of the xhci driver here? >>> OK, I will include the maintainer in the next patch. >>>> >>>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >>>>> index 2c255d0..87b3a40 100644 >>>>> --- a/drivers/usb/host/xhci-ring.c >>>>> +++ b/drivers/usb/host/xhci-ring.c >>>>> @@ -2857,6 +2857,16 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) >>>>> goto out; >>>>> } >>>>> >>>>> + if (status & STS_HCE) { >>>>> + xhci_warn(xhci, "WARNING: Host Controller Error\n"); >>>>> + xhci_halt(xhci); >>>>> + xhci_shutdown(hcd); >>>>> + xhci_start(xhci); >>>>> + xhci_run(hcd); >>>>> + ret = IRQ_HANDLED; >>>>> + goto out; >>>>> + } > This won't work at all. It doesn't reset the xHC which is the one thing needed to > recover from a HCE. This would deadlock immediately. > Many of the above functions shouldn't be called from interrupt context, and if > HCE is set we probably even won't get any interrupts. > > A reset will set all registers to their initial values, and need to be > re-initialized. Nothing is freed or re-allocated, registrs are not re-initialized > here. > > Please revisit the xhci spec about HCE, and HCRST, and a more detail look > at the xhci driver, and test the code before submitting. > >>>>> + >>>> >>>> Does this fix a real problem for you? Are you sure we will not suddenly >>>> start resetting devices that were working properly and sending this >>>> error incorrectly? How did you test this? >>>> >>>> thanks, >>>> >>>> greg k-h >>>> . >>>> >>> Yes, we want to add a RAS feature to the USB, >> >> What is "RAS"? >> >>> Use the HCE event to trigger the reset operation of the USB controller. >> >> Is that allowed by the XHCI specification? >> >>> By searching for the current xhci driver, the driver did not handle HCE event. >>> In fact, I am not sure if other operations will cause HCE, The HCE event is >>> used to reset the USB controller according to the definition of the event. >> >> What generates that event? Do existing controllers do that today? What >> causes it? >> > > It's not an event, it's a controller internal error state. > It's probably not very useful to check for it in the interrupt handler as > the xHC hw ceases all activity when it sets the HCE bit, including sending > interrupts. > >>From the spec: > > 4.24.1 Internal Errors > The Host Controller Error (HCE) flag is asserted when an internal xHC error is > detected that exclusively affects the xHC. When the HCE flag is set to ‘1’ the xHC > shall cease all activity. Software response to the assertion of HCE is to reset the > xHC (HCRST = ‘1’) and reinitialize it > >>> I test this reset operation through the Sysfs file, but have not tested all usage scenarios. >> >> What sysfs file? > > I'm also interested in this, and to know more about RAS? > > Thanks > -Mathias > . > RAS: Reliablity, Availability and Serviceability. sysfs: a virtual file system based on memory, it's similar as proc. Use DEVICE_ATTR or DRIVER_ATTR to create files to the /sys directory Thanks Longfang. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] USB: xhci: Enable HCE event reset function 2020-11-19 1:50 ` liulongfang @ 2020-11-19 6:09 ` Greg KH 2020-11-19 13:13 ` liulongfang 0 siblings, 1 reply; 8+ messages in thread From: Greg KH @ 2020-11-19 6:09 UTC (permalink / raw) To: liulongfang Cc: Mathias Nyman, linux-usb, yisen.zhuang, tanxiaofei, mathias.nyman On Thu, Nov 19, 2020 at 09:50:20AM +0800, liulongfang wrote: > On 2020/11/19 0:21, Mathias Nyman Wrote: > > On 18.11.2020 11.14, Greg KH wrote: > >> On Wed, Nov 18, 2020 at 05:04:36PM +0800, liulongfang wrote: > >>> On 2020/11/18 15:02, Greg KH Wrote: > >>>> On Wed, Nov 18, 2020 at 11:36:13AM +0800, Longfang Liu wrote: > >>>>> The HCE(Host Controller Error) event has been defined in > >>>>> the XHCI driver but has not been used. If we want to use > >>>>> the HCE event to reset the controller, can we implement > >>>>> it in the interrupt function as follows: > >>>>> > >>>>> xhci_irq() > >>>>> |----xhci_halt() > >>>>> |----xhci_shutdown() > >>>>> |----xhci_start() > >>>>> |----xhci_run() > >>>>> > >>>>> Signed-off-by: Longfang Liu <liulongfang@huawei.com> > >>>>> --- > >>>>> drivers/usb/host/xhci-ring.c | 10 ++++++++++ > >>>>> 1 file changed, 10 insertions(+) > >>>> > >>>> $ ./scripts/get_maintainer.pl --file drivers/usb/host/xhci-ring.c > >>>> Mathias Nyman <mathias.nyman@intel.com> (supporter:USB XHCI DRIVER) > >>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) > >>>> linux-usb@vger.kernel.org (open list:USB XHCI DRIVER) > >>>> linux-kernel@vger.kernel.org (open list) > >>>> > >>>> Any reason to not include the maintainer of the xhci driver here? > >>> OK, I will include the maintainer in the next patch. > >>>> > >>>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > >>>>> index 2c255d0..87b3a40 100644 > >>>>> --- a/drivers/usb/host/xhci-ring.c > >>>>> +++ b/drivers/usb/host/xhci-ring.c > >>>>> @@ -2857,6 +2857,16 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) > >>>>> goto out; > >>>>> } > >>>>> > >>>>> + if (status & STS_HCE) { > >>>>> + xhci_warn(xhci, "WARNING: Host Controller Error\n"); > >>>>> + xhci_halt(xhci); > >>>>> + xhci_shutdown(hcd); > >>>>> + xhci_start(xhci); > >>>>> + xhci_run(hcd); > >>>>> + ret = IRQ_HANDLED; > >>>>> + goto out; > >>>>> + } > > This won't work at all. It doesn't reset the xHC which is the one thing needed to > > recover from a HCE. This would deadlock immediately. > > Many of the above functions shouldn't be called from interrupt context, and if > > HCE is set we probably even won't get any interrupts. > > > > A reset will set all registers to their initial values, and need to be > > re-initialized. Nothing is freed or re-allocated, registrs are not re-initialized > > here. > > > > Please revisit the xhci spec about HCE, and HCRST, and a more detail look > > at the xhci driver, and test the code before submitting. > > > >>>>> + > >>>> > >>>> Does this fix a real problem for you? Are you sure we will not suddenly > >>>> start resetting devices that were working properly and sending this > >>>> error incorrectly? How did you test this? > >>>> > >>>> thanks, > >>>> > >>>> greg k-h > >>>> . > >>>> > >>> Yes, we want to add a RAS feature to the USB, > >> > >> What is "RAS"? > >> > >>> Use the HCE event to trigger the reset operation of the USB controller. > >> > >> Is that allowed by the XHCI specification? > >> > >>> By searching for the current xhci driver, the driver did not handle HCE event. > >>> In fact, I am not sure if other operations will cause HCE, The HCE event is > >>> used to reset the USB controller according to the definition of the event. > >> > >> What generates that event? Do existing controllers do that today? What > >> causes it? > >> > > > > It's not an event, it's a controller internal error state. > > It's probably not very useful to check for it in the interrupt handler as > > the xHC hw ceases all activity when it sets the HCE bit, including sending > > interrupts. > > > >>>From the spec: > > > > 4.24.1 Internal Errors > > The Host Controller Error (HCE) flag is asserted when an internal xHC error is > > detected that exclusively affects the xHC. When the HCE flag is set to ‘1’ the xHC > > shall cease all activity. Software response to the assertion of HCE is to reset the > > xHC (HCRST = ‘1’) and reinitialize it > > > >>> I test this reset operation through the Sysfs file, but have not tested all usage scenarios. > >> > >> What sysfs file? > > > > I'm also interested in this, and to know more about RAS? > > > > Thanks > > -Mathias > > . > > > RAS: Reliablity, Availability and Serviceability. That's a marketing phrase, what specifically are you talking about when it comes to the xhci driver? > sysfs: a virtual file system based on memory, it's similar as proc. I am very aware of what sysfs is, look at the MAINTAINERS entry for it :) > Use DEVICE_ATTR or DRIVER_ATTR to create files to the /sys directory Yes, but what file did you use for this? I don't see one in the kernel that can cause this, do you? thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] USB: xhci: Enable HCE event reset function 2020-11-19 6:09 ` Greg KH @ 2020-11-19 13:13 ` liulongfang 0 siblings, 0 replies; 8+ messages in thread From: liulongfang @ 2020-11-19 13:13 UTC (permalink / raw) To: Greg KH; +Cc: Mathias Nyman, linux-usb, yisen.zhuang, tanxiaofei, mathias.nyman On 2020/11/19 14:09, Greg KH Wrote: > On Thu, Nov 19, 2020 at 09:50:20AM +0800, liulongfang wrote: >> On 2020/11/19 0:21, Mathias Nyman Wrote: >>> On 18.11.2020 11.14, Greg KH wrote: >>>> On Wed, Nov 18, 2020 at 05:04:36PM +0800, liulongfang wrote: >>>>> On 2020/11/18 15:02, Greg KH Wrote: >>>>>> On Wed, Nov 18, 2020 at 11:36:13AM +0800, Longfang Liu wrote: >>>>>>> The HCE(Host Controller Error) event has been defined in >>>>>>> the XHCI driver but has not been used. If we want to use >>>>>>> the HCE event to reset the controller, can we implement >>>>>>> it in the interrupt function as follows: >>>>>>> >>>>>>> xhci_irq() >>>>>>> |----xhci_halt() >>>>>>> |----xhci_shutdown() >>>>>>> |----xhci_start() >>>>>>> |----xhci_run() >>>>>>> >>>>>>> Signed-off-by: Longfang Liu <liulongfang@huawei.com> >>>>>>> --- >>>>>>> drivers/usb/host/xhci-ring.c | 10 ++++++++++ >>>>>>> 1 file changed, 10 insertions(+) >>>>>> >>>>>> $ ./scripts/get_maintainer.pl --file drivers/usb/host/xhci-ring.c >>>>>> Mathias Nyman <mathias.nyman@intel.com> (supporter:USB XHCI DRIVER) >>>>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) >>>>>> linux-usb@vger.kernel.org (open list:USB XHCI DRIVER) >>>>>> linux-kernel@vger.kernel.org (open list) >>>>>> >>>>>> Any reason to not include the maintainer of the xhci driver here? >>>>> OK, I will include the maintainer in the next patch. >>>>>> >>>>>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >>>>>>> index 2c255d0..87b3a40 100644 >>>>>>> --- a/drivers/usb/host/xhci-ring.c >>>>>>> +++ b/drivers/usb/host/xhci-ring.c >>>>>>> @@ -2857,6 +2857,16 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) >>>>>>> goto out; >>>>>>> } >>>>>>> >>>>>>> + if (status & STS_HCE) { >>>>>>> + xhci_warn(xhci, "WARNING: Host Controller Error\n"); >>>>>>> + xhci_halt(xhci); >>>>>>> + xhci_shutdown(hcd); >>>>>>> + xhci_start(xhci); >>>>>>> + xhci_run(hcd); >>>>>>> + ret = IRQ_HANDLED; >>>>>>> + goto out; >>>>>>> + } >>> This won't work at all. It doesn't reset the xHC which is the one thing needed to >>> recover from a HCE. This would deadlock immediately. >>> Many of the above functions shouldn't be called from interrupt context, and if >>> HCE is set we probably even won't get any interrupts. >>> >>> A reset will set all registers to their initial values, and need to be >>> re-initialized. Nothing is freed or re-allocated, registrs are not re-initialized >>> here. >>> >>> Please revisit the xhci spec about HCE, and HCRST, and a more detail look >>> at the xhci driver, and test the code before submitting. >>> >>>>>>> + >>>>>> >>>>>> Does this fix a real problem for you? Are you sure we will not suddenly >>>>>> start resetting devices that were working properly and sending this >>>>>> error incorrectly? How did you test this? >>>>>> >>>>>> thanks, >>>>>> >>>>>> greg k-h >>>>>> . >>>>>> >>>>> Yes, we want to add a RAS feature to the USB, >>>> >>>> What is "RAS"? >>>> >>>>> Use the HCE event to trigger the reset operation of the USB controller. >>>> >>>> Is that allowed by the XHCI specification? >>>> >>>>> By searching for the current xhci driver, the driver did not handle HCE event. >>>>> In fact, I am not sure if other operations will cause HCE, The HCE event is >>>>> used to reset the USB controller according to the definition of the event. >>>> >>>> What generates that event? Do existing controllers do that today? What >>>> causes it? >>>> >>> >>> It's not an event, it's a controller internal error state. >>> It's probably not very useful to check for it in the interrupt handler as >>> the xHC hw ceases all activity when it sets the HCE bit, including sending >>> interrupts. >>> >>>> >From the spec: >>> >>> 4.24.1 Internal Errors >>> The Host Controller Error (HCE) flag is asserted when an internal xHC error is >>> detected that exclusively affects the xHC. When the HCE flag is set to ‘1’ the xHC >>> shall cease all activity. Software response to the assertion of HCE is to reset the >>> xHC (HCRST = ‘1’) and reinitialize it >>> >>>>> I test this reset operation through the Sysfs file, but have not tested all usage scenarios. >>>> >>>> What sysfs file? >>> >>> I'm also interested in this, and to know more about RAS? >>> >>> Thanks >>> -Mathias >>> . >>> >> RAS: Reliablity, Availability and Serviceability. > > That's a marketing phrase, what specifically are you talking about when > it comes to the xhci driver? > It means that when the hardware detects HCE, the USB controller could recover. >> sysfs: a virtual file system based on memory, it's similar as proc. > > I am very aware of what sysfs is, look at the MAINTAINERS entry for it :) > >> Use DEVICE_ATTR or DRIVER_ATTR to create files to the /sys directory > > Yes, but what file did you use for this? I don't see one in the kernel > that can cause this, do you? > > thanks, > > greg k-h > . > I found that there is debugfs in the USB xhci driver, and the reset test can be realized by adding the debugfs file thanks, Longfang. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-19 13:13 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-18 3:36 [RFC PATCH] USB: xhci: Enable HCE event reset function Longfang Liu 2020-11-18 7:02 ` Greg KH 2020-11-18 9:04 ` liulongfang 2020-11-18 9:14 ` Greg KH 2020-11-18 16:21 ` Mathias Nyman 2020-11-19 1:50 ` liulongfang 2020-11-19 6:09 ` Greg KH 2020-11-19 13:13 ` liulongfang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).