[PATCH 1/2] xhci: Use ilog2() rather than __ffs() for calculating SEGMENT_SHIFT

[PATCH 1/2] xhci: Use ilog2() rather than __ffs() for calculating SEGMENT_SHIFT

[David Howells]

Use ilog2() rather than __ffs() for calculating SEGMENT_SHIFT as ilog2() can
be worked out at compile time, whereas __ffs() must be calculated at runtime.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Sarah Sharp <sarah.a.sharp@linux.intel.com>
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
cc: linux-usb@vger.kernel.org
---

 drivers/usb/host/xhci.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2c510e4..558ba50 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1235,7 +1235,7 @@ union xhci_trb {
 /* Allow two commands + a link TRB, along with any reserved command TRBs */
 #define MAX_RSVD_CMD_TRBS	(TRBS_PER_SEGMENT - 3)
 #define SEGMENT_SIZE		(TRBS_PER_SEGMENT*16)
-#define SEGMENT_SHIFT		(__ffs(SEGMENT_SIZE))
+#define SEGMENT_SHIFT		(ilog2(SEGMENT_SIZE))
 /* TRB buffer pointers can't cross 64KB boundaries */
 #define TRB_MAX_BUFF_SHIFT		16
 #define TRB_MAX_BUFF_SIZE	(1 << TRB_MAX_BUFF_SHIFT)

[PATCH 2/2] xhci: Rename SEGMENT_SIZE and SEGMENT_SHIFT as the former is used in a.out.h

[David Howells]

Rename SEGMENT_SIZE and SEGMENT_SHIFT as the former is used in a.out.h.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Sarah Sharp <sarah.a.sharp@linux.intel.com>
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
cc: linux-usb@vger.kernel.org
---

 drivers/usb/host/xhci-mem.c |   16 ++++++++--------
 drivers/usb/host/xhci.h     |    4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 35616ff..e11e2af 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -51,7 +51,7 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
 		return NULL;
 	}
 
-	memset(seg->trbs, 0, SEGMENT_SIZE);
+	memset(seg->trbs, 0, TRB_SEGMENT_SIZE);
 	/* If the cycle state is 0, set the cycle bit to 1 for all the TRBs */
 	if (cycle_state == 0) {
 		for (i = 0; i < TRBS_PER_SEGMENT; i++)
@@ -467,7 +467,7 @@ struct xhci_ring *xhci_dma_to_transfer_ring(
 {
 	if (ep->ep_state & EP_HAS_STREAMS)
 		return radix_tree_lookup(&ep->stream_info->trb_address_map,
-				address >> SEGMENT_SHIFT);
+				address >> TRB_SEGMENT_SHIFT);
 	return ep->ring;
 }
 
@@ -478,7 +478,7 @@ static struct xhci_ring *dma_to_stream_ring(
 		u64 address)
 {
 	return radix_tree_lookup(&stream_info->trb_address_map,
-			address >> SEGMENT_SHIFT);
+			address >> TRB_SEGMENT_SHIFT);
 }
 #endif	/* CONFIG_USB_XHCI_HCD_DEBUGGING */
 
@@ -514,7 +514,7 @@ static int xhci_test_radix_tree(struct xhci_hcd *xhci,
 
 		cur_ring = stream_info->stream_rings[cur_stream];
 		for (addr = cur_ring->first_seg->dma;
-				addr < cur_ring->first_seg->dma + SEGMENT_SIZE;
+				addr < cur_ring->first_seg->dma + TRB_SEGMENT_SIZE;
 				addr += trb_size) {
 			mapped_ring = dma_to_stream_ring(stream_info, addr);
 			if (cur_ring != mapped_ring) {
@@ -662,7 +662,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
 				cur_stream, (unsigned long long) addr);
 
 		key = (unsigned long)
-			(cur_ring->first_seg->dma >> SEGMENT_SHIFT);
+			(cur_ring->first_seg->dma >> TRB_SEGMENT_SHIFT);
 		ret = radix_tree_insert(&stream_info->trb_address_map,
 				key, cur_ring);
 		if (ret) {
@@ -693,7 +693,7 @@ cleanup_rings:
 		if (cur_ring) {
 			addr = cur_ring->first_seg->dma;
 			radix_tree_delete(&stream_info->trb_address_map,
-					addr >> SEGMENT_SHIFT);
+					addr >> TRB_SEGMENT_SHIFT);
 			xhci_ring_free(xhci, cur_ring);
 			stream_info->stream_rings[cur_stream] = NULL;
 		}
@@ -764,7 +764,7 @@ void xhci_free_stream_info(struct xhci_hcd *xhci,
 		if (cur_ring) {
 			addr = cur_ring->first_seg->dma;
 			radix_tree_delete(&stream_info->trb_address_map,
-					addr >> SEGMENT_SHIFT);
+					addr >> TRB_SEGMENT_SHIFT);
 			xhci_ring_free(xhci, cur_ring);
 			stream_info->stream_rings[cur_stream] = NULL;
 		}
@@ -2325,7 +2325,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 	 * so we pick the greater alignment need.
 	 */
 	xhci->segment_pool = dma_pool_create("xHCI ring segments", dev,
-			SEGMENT_SIZE, 64, xhci->page_size);
+			TRB_SEGMENT_SIZE, 64, xhci->page_size);
 
 	/* See Table 46 and Note on Figure 55 */
 	xhci->device_pool = dma_pool_create("xHCI input/output contexts", dev,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 558ba50..6bf2257 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1234,8 +1234,8 @@ union xhci_trb {
 #define TRBS_PER_SEGMENT	64
 /* Allow two commands + a link TRB, along with any reserved command TRBs */
 #define MAX_RSVD_CMD_TRBS	(TRBS_PER_SEGMENT - 3)
-#define SEGMENT_SIZE		(TRBS_PER_SEGMENT*16)
-#define SEGMENT_SHIFT		(ilog2(SEGMENT_SIZE))
+#define TRB_SEGMENT_SIZE	(TRBS_PER_SEGMENT*16)
+#define TRB_SEGMENT_SHIFT	(ilog2(TRB_SEGMENT_SIZE))
 /* TRB buffer pointers can't cross 64KB boundaries */
 #define TRB_MAX_BUFF_SHIFT		16
 #define TRB_MAX_BUFF_SIZE	(1 << TRB_MAX_BUFF_SHIFT)

Re: [PATCH 2/2] xhci: Rename SEGMENT_SIZE and SEGMENT_SHIFT as the former is used in a.out.h

[Sarah Sharp]

Hi Dave,

I'm a little bit confused about your description for the second one.
Did you need to change the #defines names because they could conflict
with other drivers when the xHCI driver is built in?  Or is there some
other point I'm missing?

Are these feature patches for 3.10, or bug fixes for 3.9?  If they're
for 3.9, should they go into stable?  My inclination is the first patch
shouldn't but I'm not sure about this one.

On Thu, Mar 28, 2013 at 06:48:35PM +0000, David Howells wrote:
> Rename SEGMENT_SIZE and SEGMENT_SHIFT as the former is used in a.out.h.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> cc: linux-usb@vger.kernel.org
...
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1234,8 +1234,8 @@ union xhci_trb {
>  #define TRBS_PER_SEGMENT	64
>  /* Allow two commands + a link TRB, along with any reserved command TRBs */
>  #define MAX_RSVD_CMD_TRBS	(TRBS_PER_SEGMENT - 3)
> -#define SEGMENT_SIZE		(TRBS_PER_SEGMENT*16)
> -#define SEGMENT_SHIFT		(ilog2(SEGMENT_SIZE))
> +#define TRB_SEGMENT_SIZE	(TRBS_PER_SEGMENT*16)
> +#define TRB_SEGMENT_SHIFT	(ilog2(TRB_SEGMENT_SIZE))

I would prefer an XHCI_ prefix instead of a TRB_ prefix.  Segments are
comprised of TRBs, so my brain doesn't parse this well. :)

Thanks,
Sarah Sharp

Re: [PATCH 2/2] xhci: Rename SEGMENT_SIZE and SEGMENT_SHIFT as the former is used in a.out.h

[David Howells]

Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:

> I'm a little bit confused about your description for the second one.
> Did you need to change the #defines names because they could conflict
> with other drivers when the xHCI driver is built in?  Or is there some
> other point I'm missing?

Sorry, I should say.  I'm trying to clean up the UAPI headers and I noticed
that the xHCI SEGMENT_SIZE macro is named the same as one defined by a.out.h
that cannot be changed as it is seen by userspace.  Although it's unlikely
that within the kernel they are unlikely to collide, one cannot be entirely
sure that will stay true as new arches get added (hopefully no one will add
new arches that use a.out format).  It seems best that the xHCI one be renamed
if possible.

> Are these feature patches for 3.10, or bug fixes for 3.9?  If they're
> for 3.9, should they go into stable?  My inclination is the first patch
> shouldn't but I'm not sure about this one.

Both are aimed at 3.10.  Neither are fixes for extant bugs that I know of.

David

Re: [PATCH 2/2] xhci: Rename SEGMENT_SIZE and SEGMENT_SHIFT as the former is used in a.out.h

[Sarah Sharp]

On Thu, Mar 28, 2013 at 10:32:53PM +0000, David Howells wrote:
> Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:
> 
> > I'm a little bit confused about your description for the second one.
> > Did you need to change the #defines names because they could conflict
> > with other drivers when the xHCI driver is built in?  Or is there some
> > other point I'm missing?
> 
> Sorry, I should say.  I'm trying to clean up the UAPI headers and I noticed
> that the xHCI SEGMENT_SIZE macro is named the same as one defined by a.out.h
> that cannot be changed as it is seen by userspace.  Although it's unlikely
> that within the kernel they are unlikely to collide, one cannot be entirely
> sure that will stay true as new arches get added (hopefully no one will add
> new arches that use a.out format).  It seems best that the xHCI one be renamed
> if possible.

I guess my question is a deeper one: do we need to rename all the xHCI
macros to have the XHCI_ prefix, in order to avoid future collision?
For example, one of the macros is MAX_HC_PORTS, which could possibly be
used by other host drivers in the future.

Note that I'm not asking you to do this, just if it needs to be done.

Sarah Sharp

Re: [PATCH 2/2] xhci: Rename SEGMENT_SIZE and SEGMENT_SHIFT as the former is used in a.out.h

[David Howells]

Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:

> I guess my question is a deeper one: do we need to rename all the xHCI
> macros to have the XHCI_ prefix, in order to avoid future collision?
> For example, one of the macros is MAX_HC_PORTS, which could possibly be
> used by other host drivers in the future.

Hmmm...

I suspect the question is whether your symbols are likely to collide with
core symbols rather than symbols of unrelated drivers - after all, you're
unlikely to be #including the headers of those drivers.

I personally prefer to prefix the names of symbols in drivers with something
consistent for that driver to reduce namespace collisions - but I know not
everyone cares about that.  Linux doesn't have much of a policy in this area
though.  I also like it because it makes tags easier to use (fewer definitions
of the same symbol).

Whether you should go back and rename existing xHCI functions, I don't know.
I'd be tempted to leave it for now unless there's some collision.  However,
things like MAX_HC_PORTS does seem a little generic.  Further #define
collisions go unnoticed under some circumstances.  Two obvious cases are (a)
redefinition of a symbol because it happens to be the same value and (b) where
the second one is accidentally suppressed because it is wrapped in a
conditional.

Perhaps we should move to C++ and use namespaces;-)

David

Re: [PATCH 2/2] xhci: Rename SEGMENT_SIZE and SEGMENT_SHIFT as the former is used in a.out.h

[Sarah Sharp]

On Tue, Apr 02, 2013 at 07:07:36PM +0100, David Howells wrote:
> Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:
> 
> > I guess my question is a deeper one: do we need to rename all the xHCI
> > macros to have the XHCI_ prefix, in order to avoid future collision?
> > For example, one of the macros is MAX_HC_PORTS, which could possibly be
> > used by other host drivers in the future.
> 
> Hmmm...
> 
> I suspect the question is whether your symbols are likely to collide with
> core symbols rather than symbols of unrelated drivers - after all, you're
> unlikely to be #including the headers of those drivers.
> 
> I personally prefer to prefix the names of symbols in drivers with something
> consistent for that driver to reduce namespace collisions - but I know not
> everyone cares about that.  Linux doesn't have much of a policy in this area
> though.  I also like it because it makes tags easier to use (fewer definitions
> of the same symbol).
> 
> Whether you should go back and rename existing xHCI functions, I don't know.
> I'd be tempted to leave it for now unless there's some collision.  However,
> things like MAX_HC_PORTS does seem a little generic.  Further #define
> collisions go unnoticed under some circumstances.  Two obvious cases are (a)
> redefinition of a symbol because it happens to be the same value and (b) where
> the second one is accidentally suppressed because it is wrapped in a
> conditional.

Hmm, yeah, I think it doesn't make sense to change all the macros now.
If I were starting a new driver, I think I would use a common prefix.
I'll add the macro renaming to my todo list and see if I can get some
newbie to take it on. 8)

> Perhaps we should move to C++ and use namespaces;-)

Hah!

Sarah Sharp