| Message ID | 20190207000349.7816-1-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xhci: Use ffs() to find page size in xhci_mem_init() | expand |
On 07.02.2019 02:03, Andrey Smirnov wrote: > Get page size order using ffs() instead of open coding it with a loop. > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > Cc: Mathias Nyman <mathias.nyman@intel.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-usb@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/usb/host/xhci-mem.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 36a3eb8849f1..44b43c3d819f 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c > @@ -2362,11 +2362,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > page_size = readl(&xhci->op_regs->page_size); > xhci_dbg_trace(xhci, trace_xhci_dbg_init, > "Supported page size register = 0x%x", page_size); > - for (i = 0; i < 16; i++) { > - if ((0x1 & page_size) != 0) > - break; > - page_size = page_size >> 1; > - } > + i = ffs(page_size); > if (i < 16) > xhci_dbg_trace(xhci, trace_xhci_dbg_init, > "Supported page size of %iK", (1 << (i+12)) / 1024); Hi using ffs() is a welcome change, but it will give different a result than the loop. *old loop valid page_size value if i < 16 *ffs() valid page_size value if i >= 1 and i < 17 -Mathias
Hi, Mathias Nyman <mathias.nyman@linux.intel.com> writes: > On 07.02.2019 02:03, Andrey Smirnov wrote: >> Get page size order using ffs() instead of open coding it with a loop. >> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >> Cc: Mathias Nyman <mathias.nyman@intel.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: linux-usb@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> --- >> drivers/usb/host/xhci-mem.c | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >> index 36a3eb8849f1..44b43c3d819f 100644 >> --- a/drivers/usb/host/xhci-mem.c >> +++ b/drivers/usb/host/xhci-mem.c >> @@ -2362,11 +2362,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) >> page_size = readl(&xhci->op_regs->page_size); >> xhci_dbg_trace(xhci, trace_xhci_dbg_init, >> "Supported page size register = 0x%x", page_size); >> - for (i = 0; i < 16; i++) { >> - if ((0x1 & page_size) != 0) >> - break; >> - page_size = page_size >> 1; >> - } >> + i = ffs(page_size); >> if (i < 16) >> xhci_dbg_trace(xhci, trace_xhci_dbg_init, >> "Supported page size of %iK", (1 << (i+12)) / 1024); > > Hi > > using ffs() is a welcome change, but it will give different a result than the loop. > > *old loop > valid page_size value if i < 16 > *ffs() > valid page_size value if i >= 1 and i < 17 off-by-one, just use i = ffs() - 1. Or use __ffs().
On 07.02.2019 11:06, Felipe Balbi wrote: > > Hi, > > Mathias Nyman <mathias.nyman@linux.intel.com> writes: >> On 07.02.2019 02:03, Andrey Smirnov wrote: >>> Get page size order using ffs() instead of open coding it with a loop. >>> >>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >>> Cc: Mathias Nyman <mathias.nyman@intel.com> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Cc: linux-usb@vger.kernel.org >>> Cc: linux-kernel@vger.kernel.org >>> --- >>> drivers/usb/host/xhci-mem.c | 6 +----- >>> 1 file changed, 1 insertion(+), 5 deletions(-) >>> >>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >>> index 36a3eb8849f1..44b43c3d819f 100644 >>> --- a/drivers/usb/host/xhci-mem.c >>> +++ b/drivers/usb/host/xhci-mem.c >>> @@ -2362,11 +2362,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) >>> page_size = readl(&xhci->op_regs->page_size); >>> xhci_dbg_trace(xhci, trace_xhci_dbg_init, >>> "Supported page size register = 0x%x", page_size); >>> - for (i = 0; i < 16; i++) { >>> - if ((0x1 & page_size) != 0) >>> - break; >>> - page_size = page_size >> 1; >>> - } >>> + i = ffs(page_size); >>> if (i < 16) >>> xhci_dbg_trace(xhci, trace_xhci_dbg_init, >>> "Supported page size of %iK", (1 << (i+12)) / 1024); >> >> Hi >> >> using ffs() is a welcome change, but it will give different a result than the loop. >> >> *old loop >> valid page_size value if i < 16 >> *ffs() >> valid page_size value if i >= 1 and i < 17 > > off-by-one, just use i = ffs() - 1. Or use __ffs(). > and handle the page_size == 0 case. -Mathias
Hi, Mathias Nyman <mathias.nyman@intel.com> writes: >>>> Get page size order using ffs() instead of open coding it with a loop. >>>> >>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >>>> Cc: Mathias Nyman <mathias.nyman@intel.com> >>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>> Cc: linux-usb@vger.kernel.org >>>> Cc: linux-kernel@vger.kernel.org >>>> --- >>>> drivers/usb/host/xhci-mem.c | 6 +----- >>>> 1 file changed, 1 insertion(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >>>> index 36a3eb8849f1..44b43c3d819f 100644 >>>> --- a/drivers/usb/host/xhci-mem.c >>>> +++ b/drivers/usb/host/xhci-mem.c >>>> @@ -2362,11 +2362,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) >>>> page_size = readl(&xhci->op_regs->page_size); >>>> xhci_dbg_trace(xhci, trace_xhci_dbg_init, >>>> "Supported page size register = 0x%x", page_size); >>>> - for (i = 0; i < 16; i++) { >>>> - if ((0x1 & page_size) != 0) >>>> - break; >>>> - page_size = page_size >> 1; >>>> - } >>>> + i = ffs(page_size); >>>> if (i < 16) >>>> xhci_dbg_trace(xhci, trace_xhci_dbg_init, >>>> "Supported page size of %iK", (1 << (i+12)) / 1024); >>> >>> Hi >>> >>> using ffs() is a welcome change, but it will give different a result than the loop. >>> >>> *old loop >>> valid page_size value if i < 16 >>> *ffs() >>> valid page_size value if i >= 1 and i < 17 >> >> off-by-one, just use i = ffs() - 1. Or use __ffs(). > > and handle the page_size == 0 case. Can it be zero in real life, or are you protecting against academic possibility that's never going to happen in HW?
On 07.02.2019 12:58, Felipe Balbi wrote: > > Hi, > > Mathias Nyman <mathias.nyman@intel.com> writes: >>>>> Get page size order using ffs() instead of open coding it with a loop. >>>>> >>>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >>>>> Cc: Mathias Nyman <mathias.nyman@intel.com> >>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>>> Cc: linux-usb@vger.kernel.org >>>>> Cc: linux-kernel@vger.kernel.org >>>>> --- >>>>> drivers/usb/host/xhci-mem.c | 6 +----- >>>>> 1 file changed, 1 insertion(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >>>>> index 36a3eb8849f1..44b43c3d819f 100644 >>>>> --- a/drivers/usb/host/xhci-mem.c >>>>> +++ b/drivers/usb/host/xhci-mem.c >>>>> @@ -2362,11 +2362,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) >>>>> page_size = readl(&xhci->op_regs->page_size); >>>>> xhci_dbg_trace(xhci, trace_xhci_dbg_init, >>>>> "Supported page size register = 0x%x", page_size); >>>>> - for (i = 0; i < 16; i++) { >>>>> - if ((0x1 & page_size) != 0) >>>>> - break; >>>>> - page_size = page_size >> 1; >>>>> - } >>>>> + i = ffs(page_size); >>>>> if (i < 16) >>>>> xhci_dbg_trace(xhci, trace_xhci_dbg_init, >>>>> "Supported page size of %iK", (1 << (i+12)) / 1024); >>>> >>>> Hi >>>> >>>> using ffs() is a welcome change, but it will give different a result than the loop. >>>> >>>> *old loop >>>> valid page_size value if i < 16 >>>> *ffs() >>>> valid page_size value if i >= 1 and i < 17 >>> >>> off-by-one, just use i = ffs() - 1. Or use __ffs(). >> >> and handle the page_size == 0 case. > > Can it be zero in real life, or are you protecting against academic > possibility that's never going to happen in HW? > whole page_size check is not really doing much, just printing out different debug messages. -Mathias
On Thu, Feb 7, 2019 at 1:00 AM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 07.02.2019 02:03, Andrey Smirnov wrote: > > Get page size order using ffs() instead of open coding it with a loop. > > > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > Cc: Mathias Nyman <mathias.nyman@intel.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: linux-usb@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > --- > > drivers/usb/host/xhci-mem.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > > index 36a3eb8849f1..44b43c3d819f 100644 > > --- a/drivers/usb/host/xhci-mem.c > > +++ b/drivers/usb/host/xhci-mem.c > > @@ -2362,11 +2362,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > > page_size = readl(&xhci->op_regs->page_size); > > xhci_dbg_trace(xhci, trace_xhci_dbg_init, > > "Supported page size register = 0x%x", page_size); > > - for (i = 0; i < 16; i++) { > > - if ((0x1 & page_size) != 0) > > - break; > > - page_size = page_size >> 1; > > - } > > + i = ffs(page_size); > > if (i < 16) > > xhci_dbg_trace(xhci, trace_xhci_dbg_init, > > "Supported page size of %iK", (1 << (i+12)) / 1024); > > Hi > > using ffs() is a welcome change, but it will give different a result than the loop. > > *old loop > valid page_size value if i < 16 > *ffs() > valid page_size value if i >= 1 and i < 17 > My bad, sorry about that, was using __fls() in the version I tested, but then switched it to ffs() at the last minute and forgot to double check. Will update in v2. Thanks, Andrey Smirnov
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 36a3eb8849f1..44b43c3d819f 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -2362,11 +2362,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) page_size = readl(&xhci->op_regs->page_size); xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Supported page size register = 0x%x", page_size); - for (i = 0; i < 16; i++) { - if ((0x1 & page_size) != 0) - break; - page_size = page_size >> 1; - } + i = ffs(page_size); if (i < 16) xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Supported page size of %iK", (1 << (i+12)) / 1024);
Get page size order using ffs() instead of open coding it with a loop. Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: Mathias Nyman <mathias.nyman@intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-usb@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- drivers/usb/host/xhci-mem.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)