Skip to contentSkip to conversation

xhci: mem: Short cleanup series

PATCH Superseded Andy Shevchenko 2 participants 10 messages Feb 6, 2023 – Feb 7, 2023

Subsystems (2)
USB USB xHCI driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Andy Shevchenko

author
#0 lore raw

cover xhci: mem: Short cleanup series

Clean up xhci-mem.c a bit using latest and greatest Linux kernel features.

Andy Shevchenko (7):
xhci: mem: Carefully calculate size for memory allocations
xhci: mem: Use __GFP_ZERO instead of explicit memset() call
xhci: mem: Get rid of redundant 'else'
xhci: mem: Drop useless return:s
xhci: mem: Use while (i--) pattern to clean up
xhci: mem: Replace explicit castings with appropriate specifiers
xhci: mem: Join string literals back

 drivers/usb/host/xhci-mem.c | 85 +++++++++++++++----------------------
 1 file changed, 35 insertions(+), 50 deletions(-)
Signature · 1 line
2.39.1
author
#1 lore raw

PATCH 1/7 xhci: mem: Carefully calculate size for memory allocations

Carefully calculate size for memory allocations, i.e. with help of size_mul() macro from overflow.h.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/usb/host/xhci-mem.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/usb/host/xhci-mem.c +5 -4
index d0a9467aa5fc..c385513ad00b 100644
@@ -9,6 +9,7 @@
  */
 
 #include <linux/usb.h>
+#include <linux/overflow.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/dmapool.h>

@@ -568,7 +569,7 @@ static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct xhci_hcd *xhci,
 		gfp_t mem_flags)
 {
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
-	size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs;
+	size_t size = size_mul(sizeof(struct xhci_stream_ctx), num_stream_ctxs);
 
 	if (size > MEDIUM_STREAM_ARRAY_SIZE)
 		return dma_alloc_coherent(dev, size,

@@ -1660,7 +1661,7 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
 		goto fail_sp;
 
 	xhci->scratchpad->sp_array = dma_alloc_coherent(dev,
-				     num_sp * sizeof(u64),
+				     size_mul(sizeof(u64), num_sp),
 				     &xhci->scratchpad->sp_dma, flags);
 	if (!xhci->scratchpad->sp_array)
 		goto fail_sp2;

@@ -1799,7 +1800,7 @@ int xhci_alloc_erst(struct xhci_hcd *xhci,
 	struct xhci_segment *seg;
 	struct xhci_erst_entry *entry;
 
-	size = sizeof(struct xhci_erst_entry) * evt_ring->num_segs;
+	size = size_mul(sizeof(struct xhci_erst_entry), evt_ring->num_segs);
 	erst->entries = dma_alloc_coherent(xhci_to_hcd(xhci)->self.sysdev,
 					   size, &erst->erst_dma_addr, flags);
 	if (!erst->entries)

@@ -1830,7 +1831,7 @@ xhci_free_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
 	if (!ir)
 		return;
 
-	erst_size = sizeof(struct xhci_erst_entry) * (ir->erst.num_entries);
+	erst_size = sizeof(struct xhci_erst_entry) * ir->erst.num_entries;
 	if (ir->erst.entries)
 		dma_free_coherent(dev, erst_size,
 				  ir->erst.entries,
Signature · 1 line
2.39.1
author
#2 lore raw

PATCH 2/7 xhci: mem: Use __GFP_ZERO instead of explicit memset() call

In reply to Andy Shevchenko

Use __GFP_ZERO instead of explicit memset() call in xhci_alloc_stream_ctx().

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/usb/host/xhci-mem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/host/xhci-mem.c +2 -2
index c385513ad00b..768adcb544a7 100644
@@ -571,6 +571,8 @@ static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct xhci_hcd *xhci,
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
 	size_t size = size_mul(sizeof(struct xhci_stream_ctx), num_stream_ctxs);
 
+	mem_flags |= __GFP_ZERO;
+
 	if (size > MEDIUM_STREAM_ARRAY_SIZE)
 		return dma_alloc_coherent(dev, size,
 				dma, mem_flags);

@@ -643,8 +645,6 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
 			mem_flags);
 	if (!stream_info->stream_ctx_array)
 		goto cleanup_ring_array;
-	memset(stream_info->stream_ctx_array, 0,
-			sizeof(struct xhci_stream_ctx)*num_stream_ctxs);
 
 	/* Allocate everything needed to free the stream rings later */
 	stream_info->free_streams_command =
Signature · 1 line
2.39.1
#3 lore raw

Re: [PATCH v1 2/7] xhci: mem: Use __GFP_ZERO instead of explicit memset() call

Quoted 13 lines · Andy Shevchenko

Use __GFP_ZERO instead of explicit memset() call in xhci_alloc_stream_ctx().

Signed-off-by Andy Shevchenko <[email protected]>
---
  drivers/usb/host/xhci-mem.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/host/xhci-mem.c +0 -0
index c385513ad00b..768adcb544a7 100644

drivers/usb/host/xhci-mem.c static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct xhci_hcd *xhci, · line 575

Quoted 5 lines
drivers/usb/host/xhci-mem.c +2 -0
@@ -571,6 +571,8 @@ static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct xhci_hcd *xhci,
  	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
  	size_t size = size_mul(sizeof(struct xhci_stream_ctx), num_stream_ctxs);
 
+	mem_flags |= __GFP_ZERO;
+

How about calling dma_pool_zalloc() instead of setting __GFP_ZERO flag? Memory returned by dma_alloc_coherent() should already be zeroed if I remember correctly

-Mathias

author
#4 lore raw

Re: [PATCH v1 2/7] xhci: mem: Use __GFP_ZERO instead of explicit memset() call

Quoted 3 lines · Mathias Nyman
Quoted 2 lines · Andy Shevchenko

Use __GFP_ZERO instead of explicit memset() call in xhci_alloc_stream_ctx().

...

Quoted 4 lines

drivers/usb/host/xhci-mem.c static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct xhci_hcd *xhci, · line 574

Quoted 1 line
drivers/usb/host/xhci-mem.c +1 -0
+	mem_flags |= __GFP_ZERO;

How about calling dma_pool_zalloc() instead of setting __GFP_ZERO flag? Memory returned by dma_alloc_coherent() should already be zeroed if I remember correctly

That will work too.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#5 lore raw

PATCH 3/7 xhci: mem: Get rid of redundant 'else'

In reply to Andy Shevchenko

In the snippets like the following

	if (...)
		return / goto / break / continue ...;
	else
		...

the 'else' is redundant. Get rid of it.

While at it, make if chain sorted from testing bigger values to smaller.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/usb/host/xhci-mem.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/usb/host/xhci-mem.c +6 -8
index 768adcb544a7..34f5ba19471e 100644
@@ -574,14 +574,11 @@ static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct xhci_hcd *xhci,
 	mem_flags |= __GFP_ZERO;
 
 	if (size > MEDIUM_STREAM_ARRAY_SIZE)
-		return dma_alloc_coherent(dev, size,
-				dma, mem_flags);
-	else if (size <= SMALL_STREAM_ARRAY_SIZE)
-		return dma_pool_alloc(xhci->small_streams_pool,
-				mem_flags, dma);
+		return dma_alloc_coherent(dev, size, dma, mem_flags);
+	if (size > SMALL_STREAM_ARRAY_SIZE)
+		return dma_pool_alloc(xhci->medium_streams_pool, mem_flags, dma);
 	else
-		return dma_pool_alloc(xhci->medium_streams_pool,
-				mem_flags, dma);
+		return dma_pool_alloc(xhci->small_streams_pool, mem_flags, dma);
 }
 
 struct xhci_ring *xhci_dma_to_transfer_ring(

@@ -1401,8 +1398,9 @@ static u32 xhci_get_max_esit_payload(struct usb_device *udev,
 	if ((udev->speed >= USB_SPEED_SUPER_PLUS) &&
 	    USB_SS_SSP_ISOC_COMP(ep->ss_ep_comp.bmAttributes))
 		return le32_to_cpu(ep->ssp_isoc_ep_comp.dwBytesPerInterval);
+
 	/* SuperSpeed or SuperSpeedPlus Isoc ep with less than 48k per esit */
-	else if (udev->speed >= USB_SPEED_SUPER)
+	if (udev->speed >= USB_SPEED_SUPER)
 		return le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval);
 
 	max_packet = usb_endpoint_maxp(&ep->desc);
Signature · 1 line
2.39.1
author
#6 lore raw

PATCH 4/7 xhci: mem: Drop useless return:s

In reply to Andy Shevchenko

When function returns void and we have if-else-if chain, there is no need to explicitly call return. Drop them and indent lines better.

While at it, make if chain sorted from testing bigger values to smaller.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/usb/host/xhci-mem.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/usb/host/xhci-mem.c +4 -7
index 34f5ba19471e..5c873e62c4d3 100644
@@ -544,14 +544,11 @@ static void xhci_free_stream_ctx(struct xhci_hcd *xhci,
 	size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs;
 
 	if (size > MEDIUM_STREAM_ARRAY_SIZE)
-		dma_free_coherent(dev, size,
-				stream_ctx, dma);
-	else if (size <= SMALL_STREAM_ARRAY_SIZE)
-		return dma_pool_free(xhci->small_streams_pool,
-				stream_ctx, dma);
+		dma_free_coherent(dev, size, stream_ctx, dma);
+	else if (size > SMALL_STREAM_ARRAY_SIZE)
+		dma_pool_free(xhci->medium_streams_pool, stream_ctx, dma);
 	else
-		return dma_pool_free(xhci->medium_streams_pool,
-				stream_ctx, dma);
+		dma_pool_free(xhci->small_streams_pool, stream_ctx, dma);
 }
 
 /*
Signature · 1 line
2.39.1
author
#7 lore raw

PATCH 5/7 xhci: mem: Use while (i--) pattern to clean up

In reply to Andy Shevchenko

Use more natural while (i--) patter to clean up allocated resources.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/usb/host/xhci-mem.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/host/xhci-mem.c +1 -2
index 5c873e62c4d3..a3351b11efa5 100644
@@ -1681,11 +1681,10 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
 	return 0;
 
  fail_sp4:
-	for (i = i - 1; i >= 0; i--) {
+	while (i--)
 		dma_free_coherent(dev, xhci->page_size,
 				    xhci->scratchpad->sp_buffers[i],
 				    xhci->scratchpad->sp_array[i]);
-	}
 
 	kfree(xhci->scratchpad->sp_buffers);
Signature · 1 line
2.39.1
author
#8 lore raw

PATCH 6/7 xhci: mem: Replace explicit castings with appropriate specifiers

In reply to Andy Shevchenko

There is no need to have explicit castings when we have specific pointer extensions Replace the explicit castings with appropriate specifiers.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/usb/host/xhci-mem.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
drivers/usb/host/xhci-mem.c +7 -10
index a3351b11efa5..fef74e7c20fc 100644
@@ -668,8 +668,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
 			cur_ring->cycle_state;
 		stream_info->stream_ctx_array[cur_stream].stream_ring =
 			cpu_to_le64(addr);
-		xhci_dbg(xhci, "Setting stream %d ring ptr to 0x%08llx\n",
-				cur_stream, (unsigned long long) addr);
+		xhci_dbg(xhci, "Setting stream %d ring ptr to 0x%08llx\n", cur_stream, addr);
 
 		ret = xhci_update_stream_mapping(cur_ring, mem_flags);
 		if (ret) {

@@ -979,16 +978,14 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 	if (!dev->out_ctx)
 		goto fail;
 
-	xhci_dbg(xhci, "Slot %d output ctx = 0x%llx (dma)\n", slot_id,
-			(unsigned long long)dev->out_ctx->dma);
+	xhci_dbg(xhci, "Slot %d output ctx = 0x%pad (dma)\n", slot_id, &dev->out_ctx->dma);
 
 	/* Allocate the (input) device context for address device command */
 	dev->in_ctx = xhci_alloc_container_ctx(xhci, XHCI_CTX_TYPE_INPUT, flags);
 	if (!dev->in_ctx)
 		goto fail;
 
-	xhci_dbg(xhci, "Slot %d input ctx = 0x%llx (dma)\n", slot_id,
-			(unsigned long long)dev->in_ctx->dma);
+	xhci_dbg(xhci, "Slot %d input ctx = 0x%pad (dma)\n", slot_id, &dev->in_ctx->dma);
 
 	/* Initialize the cancellation and bandwidth list for each ep */
 	for (i = 0; i < 31; i++) {

@@ -2353,8 +2350,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 		goto fail;
 	xhci->dcbaa->dma = dma;
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-			"// Device context base array address = 0x%llx (DMA), %p (virt)",
-			(unsigned long long)xhci->dcbaa->dma, xhci->dcbaa);
+			"// Device context base array address = 0x%pad (DMA), %p (virt)",
+			&xhci->dcbaa->dma, xhci->dcbaa);
 	xhci_write_64(xhci, dma, &xhci->op_regs->dcbaa_ptr);
 
 	/*

@@ -2395,8 +2392,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 		goto fail;
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
 			"Allocated command ring at %p", xhci->cmd_ring);
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "First segment DMA is 0x%llx",
-			(unsigned long long)xhci->cmd_ring->first_seg->dma);
+	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "First segment DMA is 0x%pad",
+			&xhci->cmd_ring->first_seg->dma);
 
 	/* Set the address in the Command Ring Control register */
 	val_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
Signature · 1 line
2.39.1
author
#9 lore raw

PATCH 7/7 xhci: mem: Join string literals back

In reply to Andy Shevchenko

For easy grepping on debug purposes join string literals back in the messages.

No functional change.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/usb/host/xhci-mem.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)
drivers/usb/host/xhci-mem.c +10 -17
index fef74e7c20fc..9bdf2ed85772 100644
@@ -609,8 +609,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
 	int ret;
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
 
-	xhci_dbg(xhci, "Allocating %u streams and %u "
-			"stream context array entries.\n",
+	xhci_dbg(xhci, "Allocating %u streams and %u stream context array entries.\n",
 			num_streams, num_stream_ctxs);
 	if (xhci->cmd_ring_reserved_trbs == MAX_RSVD_CMD_TRBS) {
 		xhci_dbg(xhci, "Command ring has no reserved TRBs available\n");

@@ -1952,8 +1951,7 @@ static void xhci_set_hc_event_deq(struct xhci_hcd *xhci, struct xhci_interrupter
 	deq = xhci_trb_virt_to_dma(ir->event_ring->deq_seg,
 			ir->event_ring->dequeue);
 	if (!deq)
-		xhci_warn(xhci, "WARN something wrong with SW event ring "
-				"dequeue ptr.\n");
+		xhci_warn(xhci, "WARN something wrong with SW event ring dequeue ptr.\n");
 	/* Update HC event ring dequeue pointer */
 	temp = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
 	temp &= ERST_PTR_MASK;

@@ -1962,8 +1960,7 @@ static void xhci_set_hc_event_deq(struct xhci_hcd *xhci, struct xhci_interrupter
 	 */
 	temp &= ~ERST_EHB;
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-			"// Write event ring dequeue pointer, "
-			"preserving EHB bit");
+		       "// Write event ring dequeue pointer, preserving EHB bit");
 	xhci_write_64(xhci, ((u64) deq & (u64) ~ERST_PTR_MASK) | temp,
 			&ir->ir_set->erst_dequeue);
 }

@@ -1996,8 +1993,7 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports,
 	} else if (major_revision <= 0x02) {
 		rhub = &xhci->usb2_rhub;
 	} else {
-		xhci_warn(xhci, "Ignoring unknown port speed, "
-				"Ext Cap %p, revision = 0x%x\n",
+		xhci_warn(xhci, "Ignoring unknown port speed, Ext Cap %p, revision = 0x%x\n",
 				addr, major_revision);
 		/* Ignoring port protocol we can't understand. FIXME */
 		return;

@@ -2012,9 +2008,8 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports,
 	port_offset = XHCI_EXT_PORT_OFF(temp);
 	port_count = XHCI_EXT_PORT_COUNT(temp);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-			"Ext Cap %p, port offset = %u, "
-			"count = %u, revision = 0x%x",
-			addr, port_offset, port_count, major_revision);
+		       "Ext Cap %p, port offset = %u, count = %u, revision = 0x%x",
+		       addr, port_offset, port_count, major_revision);
 	/* Port count includes the current port offset */
 	if (port_offset == 0 || (port_offset + port_count - 1) > num_ports)
 		/* WTF? "Valid values are ‘1’ to MaxPorts" */

@@ -2071,10 +2066,8 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports,
 		struct xhci_port *hw_port = &xhci->hw_ports[i];
 		/* Duplicate entry.  Ignore the port if the revisions differ. */
 		if (hw_port->rhub) {
-			xhci_warn(xhci, "Duplicate port entry, Ext Cap %p,"
-					" port %u\n", addr, i);
-			xhci_warn(xhci, "Port was marked as USB %u, "
-					"duplicated as USB %u\n",
+			xhci_warn(xhci, "Duplicate port entry, Ext Cap %p, port %u\n", addr, i);
+			xhci_warn(xhci, "Port was marked as USB %u, duplicated as USB %u\n",
 					hw_port->rhub->maj_rev, major_revision);
 			/* Only adjust the roothub port counts if we haven't
 			 * found a similar duplicate.

@@ -2413,8 +2406,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 	val = readl(&xhci->cap_regs->db_off);
 	val &= DBOFF_MASK;
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-			"// Doorbell array is located at offset 0x%x"
-			" from cap regs base addr", val);
+		       "// Doorbell array is located at offset 0x%x from cap regs base addr",
+		       val);
 	xhci->dba = (void __iomem *) xhci->cap_regs + val;
 	/* Set ir_set to interrupt register set 0 */
Signature · 1 line
2.39.1