Re: [PATCH] xhci: Use ffs() to find page size in xhci_mem_init()
On Thu, Feb 7, 2019 at 1:00 AM Mathias Nyman 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 > > Cc: Mathias Nyman > > Cc: Greg Kroah-Hartman > > Cc: linux-...@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(>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
Re: [PATCH] xhci: Use ffs() to find page size in xhci_mem_init()
On 07.02.2019 12:58, Felipe Balbi wrote: Hi, Mathias Nyman writes: Get page size order using ffs() instead of open coding it with a loop. Signed-off-by: Andrey Smirnov Cc: Mathias Nyman Cc: Greg Kroah-Hartman Cc: linux-...@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(>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
Re: [PATCH] xhci: Use ffs() to find page size in xhci_mem_init()
Hi, Mathias Nyman writes: Get page size order using ffs() instead of open coding it with a loop. Signed-off-by: Andrey Smirnov Cc: Mathias Nyman Cc: Greg Kroah-Hartman Cc: linux-...@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(>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? -- balbi signature.asc Description: PGP signature
Re: [PATCH] xhci: Use ffs() to find page size in xhci_mem_init()
On 07.02.2019 11:06, Felipe Balbi wrote: Hi, Mathias Nyman 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 Cc: Mathias Nyman Cc: Greg Kroah-Hartman Cc: linux-...@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(>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
Re: [PATCH] xhci: Use ffs() to find page size in xhci_mem_init()
Hi, Mathias Nyman 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 >> Cc: Mathias Nyman >> Cc: Greg Kroah-Hartman >> Cc: linux-...@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(>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(). -- balbi signature.asc Description: PGP signature
Re: [PATCH] xhci: Use ffs() to find page size in xhci_mem_init()
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 Cc: Mathias Nyman Cc: Greg Kroah-Hartman Cc: linux-...@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(>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
[PATCH] xhci: Use ffs() to find page size in xhci_mem_init()
Get page size order using ffs() instead of open coding it with a loop. Signed-off-by: Andrey Smirnov Cc: Mathias Nyman Cc: Greg Kroah-Hartman Cc: linux-...@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(>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); -- 2.20.1