| Message ID | 20240820121501.3593245-3-quic_zhonhan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: Call cpu_relax() when polling registers | expand |
On 20.8.2024 15.15, Zhongqiu Han wrote: > It is considered good practice to call cpu_relax() in busy loops, see > Documentation/process/volatile-considered-harmful.rst. This can lower > CPU power consumption or yield to a hyperthreaded twin processor and > also serve as a compiler barrier. In addition, if something goes wrong > in the busy loop at least it can prevent things from getting worse. > > Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com> > --- > drivers/usb/host/xhci-ext-caps.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci-ext-caps.h > index 96eb36a58738..25d148d60ab0 100644 > --- a/drivers/usb/host/xhci-ext-caps.h > +++ b/drivers/usb/host/xhci-ext-caps.h > @@ -144,6 +144,8 @@ static inline int xhci_find_next_ext_cap(void __iomem *base, u32 start, int id) > if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == id)) > return offset; > > + cpu_relax(); > + > next = XHCI_EXT_CAPS_NEXT(val); > offset += next << 2; > } while (next); Similar case as with PATCH 1/2 This isn't a busy loop polling for some value. We traverse xhci extended capabilities until the one we are looking for is found. Thanks Mathias
On 8/21/2024 5:40 PM, Mathias Nyman wrote: > On 20.8.2024 15.15, Zhongqiu Han wrote: >> It is considered good practice to call cpu_relax() in busy loops, see >> Documentation/process/volatile-considered-harmful.rst. This can lower >> CPU power consumption or yield to a hyperthreaded twin processor and >> also serve as a compiler barrier. In addition, if something goes wrong >> in the busy loop at least it can prevent things from getting worse. >> >> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com> >> --- >> drivers/usb/host/xhci-ext-caps.h | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci- >> ext-caps.h >> index 96eb36a58738..25d148d60ab0 100644 >> --- a/drivers/usb/host/xhci-ext-caps.h >> +++ b/drivers/usb/host/xhci-ext-caps.h >> @@ -144,6 +144,8 @@ static inline int xhci_find_next_ext_cap(void >> __iomem *base, u32 start, int id) >> if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == >> id)) >> return offset; >> + cpu_relax(); >> + >> next = XHCI_EXT_CAPS_NEXT(val); >> offset += next << 2; >> } while (next); > > Similar case as with PATCH 1/2 > > This isn't a busy loop polling for some value. > We traverse xhci extended capabilities until the one we are looking for > is found. > > Thanks > Mathias > Hi Mathias, Thanks a lot for the review, yes, it is similar case as with PATCH 1/2 there is not a busy loop polling, sorry for this and i will careful for similar case next time, and thanks for the discussion as well.
diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci-ext-caps.h index 96eb36a58738..25d148d60ab0 100644 --- a/drivers/usb/host/xhci-ext-caps.h +++ b/drivers/usb/host/xhci-ext-caps.h @@ -144,6 +144,8 @@ static inline int xhci_find_next_ext_cap(void __iomem *base, u32 start, int id) if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == id)) return offset; + cpu_relax(); + next = XHCI_EXT_CAPS_NEXT(val); offset += next << 2; } while (next);
It is considered good practice to call cpu_relax() in busy loops, see Documentation/process/volatile-considered-harmful.rst. This can lower CPU power consumption or yield to a hyperthreaded twin processor and also serve as a compiler barrier. In addition, if something goes wrong in the busy loop at least it can prevent things from getting worse. Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com> --- drivers/usb/host/xhci-ext-caps.h | 2 ++ 1 file changed, 2 insertions(+)