cxgb4: clean up a type issue

PATCH In review Dan Carpenter 3 participants 8 messages Oct 2, 2014 – Oct 8, 2014

Subsystems (4)

Networking Network drivers Kernel janitors CXGB4 Ethernet driver (CXGB4)

lore

Share & tools

lore

Dan Carpenter

author

2014-10-02 11:22 UTC #0 lore raw

PATCH 1/2 cxgb4: clean up a type issue

The tx_desc struct hold 8 __be64 values. The original code took a tx_desc pointer then casted it to an int pointer and then casted it to a u64 pointer. It was confusing and triggered some static checker warnings.

I have changed the cxgb_pio_copy() to only take tx_desc pointers. This isn't really a loss of flexibility because anything else was buggy to begin with.

Signed-off-by Dan Carpenter <[email protected]>

drivers/net/ethernet/chelsio/cxgb4/sge.c +5 -5

index bb7851e..599cdfd 100644
@@ -854,9 +854,10 @@ static void write_sgl(const struct sk_buff *skb, struct sge_txq *q,
  * memory mapped BAR2 space(user space writes).
  * For coalesced WR SGE, fetches data from the FIFO instead of from Host.
  */
-static void cxgb_pio_copy(u64 __iomem *dst, u64 *src)
+static void cxgb_pio_copy(u64 __iomem *dst, struct tx_desc *desc)
 {
-	int count = 8;
+	int count = sizeof(*desc) / sizeof(u64);
+	u64 *src = (u64 *)desc;
 
 	while (count) {
 		writeq(*src, dst);

@@ -914,12 +915,11 @@ static inline void ring_tx_db(struct adapter *adap, struct sge_txq *q, int n)
 			int index = (q->pidx
 				     ? (q->pidx - 1)
 				     : (q->size - 1));
-			unsigned int *wr = (unsigned int *)&q->desc[index];
+			struct tx_desc *desc = &q->desc[index];
 
 			cxgb_pio_copy((u64 __iomem *)
 				      (adap->bar2 + q->udb +
-				       SGE_UDB_WCDOORBELL),
-				      (u64 *)wr);
+				       SGE_UDB_WCDOORBELL), desc);
 		} else {
 			writel(val,  adap->bar2 + q->udb + SGE_UDB_KDOORBELL);
 		}

David Laight

2014-10-02 11:31 UTC #1 lore raw

From: Dan Carpenter

Quoted 30 lines

The tx_desc struct hold 8 __be64 values. The original code took a tx_desc pointer then casted it to an int pointer and then casted it to a u64 pointer. It was confusing and triggered some static checker warnings.
I have changed the cxgb_pio_copy() to only take tx_desc pointers. This isn't really a loss of flexibility because anything else was buggy to begin with.
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/ethernet/chelsio/cxgb4/sge.c +3 -2
index bb7851e..599cdfd 100644
@@ -854,9 +854,10 @@ static void write_sgl(const struct sk_buff *skb, struct sge_txq *q,
  * memory mapped BAR2 space(user space writes).
  * For coalesced WR SGE, fetches data from the FIFO instead of from Host.
  */
-static void cxgb_pio_copy(u64 __iomem *dst, u64 *src)
+static void cxgb_pio_copy(u64 __iomem *dst, struct tx_desc *desc)
 {
-	int count = 8;
+	int count = sizeof(*desc) / sizeof(u64);
+	u64 *src = (u64 *)desc;
 
 	while (count) {
 		writeq(*src, dst);
@@ -914,12 +915,11 @@ static inline void ring_tx_db(struct adapter *adap, struct sge_txq *q, int n)
 			int index = (q->pidx

drivers/net/ethernet/chelsio/cxgb4/sge.c static inline void ring_tx_db(struct adapter *adap, struct sge_txq *q, int n) · line 922

Quoted 10 lines

drivers/net/ethernet/chelsio/cxgb4/sge.c +2 -3

 				     ? (q->pidx - 1)
 				     : (q->size - 1));
-			unsigned int *wr = (unsigned int *)&q->desc[index];
+			struct tx_desc *desc = &q->desc[index];
 
 			cxgb_pio_copy((u64 __iomem *)
 				      (adap->bar2 + q->udb +
-				       SGE_UDB_WCDOORBELL),
-				      (u64 *)wr);
+				       SGE_UDB_WCDOORBELL), desc);

Why not put &q->desc[index] in the call itself.
And I can't help think there are better places to break the long line. Getting it to the code below would be nice:

			cxgp_pio_copy(adap->bar2 + q->udb + SGE_UDB_WCDOORBELL,
					  q->desc + index);

	David

Quoted 7 lines

drivers/net/ethernet/chelsio/cxgb4/sge.c +0 -1
 		} else {
 			writel(val,  adap->bar2 + q->udb + SGE_UDB_KDOORBELL);
 		}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

maintainer

2014-10-03 22:44 UTC #2 lore raw

In reply to Dan Carpenter

From: Dan Carpenter <[email protected]>
Date: Thu, 2 Oct 2014 14:22:19 +0300

Quoted 10 lines

The tx_desc struct hold 8 __be64 values. The original code took a tx_desc pointer then casted it to an int pointer and then casted it to a u64 pointer. It was confusing and triggered some static checker warnings.
I have changed the cxgb_pio_copy() to only take tx_desc pointers. This isn't really a loss of flexibility because anything else was buggy to begin with.
Signed-off-by Dan Carpenter <[email protected]>

Please address the feedback you've received, resubmit this series, and actually number this second change "2/2" instead of "1/2" :-)

Thanks!

Dan Carpenter

author

2014-10-08 10:18 UTC #3 lore raw

Quoted 8 lines · David Miller

From: Dan Carpenter <[email protected]>
Date: Thu, 2 Oct 2014 14:22:19 +0300
Quoted 5 lines
The tx_desc struct hold 8 __be64 values. The original code took a tx_desc pointer then casted it to an int pointer and then casted it to a u64 pointer. It was confusing and triggered some static checker warnings.

Quoted 9 lines

Quoted 5 lines
I have changed the cxgb_pio_copy() to only take tx_desc pointers. This isn't really a loss of flexibility because anything else was buggy to begin with.
Signed-off-by Dan Carpenter <[email protected]>
Please address the feedback you've received, resubmit this series, and actually number this second change "2/2" instead of "1/2" :-)

Yes. Sorry for the delay. I'll send that this afternoon.

regards,
dan carpenter

Dan Carpenter

author

2014-10-08 13:43 UTC #4 lore raw

PATCH v2 1/2 cxgb4: potential shift wrapping bug

In reply to David Miller

"cntxt_id" is an unsigned int but "udb" is a u64 so there is a potential shift wrapping bug here.

Signed-off-by Dan Carpenter <[email protected]>

---
No changes since v1.

drivers/net/ethernet/chelsio/cxgb4/sge.c +1 -1

index bb7851e..e8e90ce 100644
@@ -2258,7 +2258,7 @@ static u64 udb_address(struct adapter *adap, unsigned int cntxt_id,
 		(QUEUESPERPAGEPF1 - QUEUESPERPAGEPF0) * adap->fn);
 	udb_density = 1 << ((qpp >> s_qpp) & QUEUESPERPAGEPF0_MASK);
 	qpshift = PAGE_SHIFT - ilog2(udb_density);
-	udb = cntxt_id << qpshift;
+	udb = (u64)cntxt_id << qpshift;
 	udb &= PAGE_MASK;
 	page = udb / PAGE_SIZE;
 	udb += (cntxt_id - (page * udb_density)) * SGE_UDB_SIZE;

David Miller

maintainer Applied

2014-10-08 20:08 UTC #5 lore raw

Re: [patch 1/2 v2 -next] cxgb4: potential shift wrapping bug

From: Dan Carpenter <[email protected]>
Date: Wed, 8 Oct 2014 16:43:17 +0300

Quoted 4 lines

"cntxt_id" is an unsigned int but "udb" is a u64 so there is a potential shift wrapping bug here.
Signed-off-by Dan Carpenter <[email protected]>

Applied.

Dan Carpenter

author

2014-10-08 13:44 UTC #6 lore raw

PATCH v2 2/2 cxgb4: clean up a type issue

In reply to David Miller

The tx_desc struct holds 8 __be64 values. The original code in ring_tx_db() took a tx_desc pointer then casted it to an int pointer and then casted it to a u64 pointer. It was confusing and triggered some static checker warnings.

I have changed the cxgb_pio_copy() function to only take tx_desc pointers. This isn't really a loss of flexibility because anything else was buggy to begin with.

I also removed the casting on the destination pointer since that was unnecessary and a bit messy.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: More cleanups as suggested by David Laight.
Update cxgb_pio_copy() comments.

drivers/net/ethernet/chelsio/cxgb4/sge.c +8 ~~-10~~

index e8e90ce..fab4c84 100644
@@ -850,13 +850,14 @@ static void write_sgl(const struct sk_buff *skb, struct sge_txq *q,
 		*end = 0;
 }
 
-/* This function copies 64 byte coalesced work request to
- * memory mapped BAR2 space(user space writes).
- * For coalesced WR SGE, fetches data from the FIFO instead of from Host.
+/* This function copies a tx_desc struct to memory mapped BAR2 space(user space
+ * writes). For coalesced WR SGE, fetches data from the FIFO instead of from
+ * Host.
  */
-static void cxgb_pio_copy(u64 __iomem *dst, u64 *src)
+static void cxgb_pio_copy(u64 __iomem *dst, struct tx_desc *desc)
 {
-	int count = 8;
+	int count = sizeof(*desc) / sizeof(u64);
+	u64 *src = (u64 *)desc;
 
 	while (count) {
 		writeq(*src, dst);

@@ -914,12 +915,9 @@ static inline void ring_tx_db(struct adapter *adap, struct sge_txq *q, int n)
 			int index = (q->pidx
 				     ? (q->pidx - 1)
 				     : (q->size - 1));
-			unsigned int *wr = (unsigned int *)&q->desc[index];
 
-			cxgb_pio_copy((u64 __iomem *)
-				      (adap->bar2 + q->udb +
-				       SGE_UDB_WCDOORBELL),
-				      (u64 *)wr);
+			cxgb_pio_copy(adap->bar2 + q->udb + SGE_UDB_WCDOORBELL,
+				      q->desc + index);
 		} else {
 			writel(val,  adap->bar2 + q->udb + SGE_UDB_KDOORBELL);
 		}

David Miller

maintainer Applied

2014-10-08 20:08 UTC #7 lore raw

From: Dan Carpenter <[email protected]>
Date: Wed, 8 Oct 2014 16:44:34 +0300

Quoted 5 lines

The tx_desc struct holds 8 __be64 values. The original code in ring_tx_db() took a tx_desc pointer then casted it to an int pointer and then casted it to a u64 pointer. It was confusing and triggered some static checker warnings.

Quoted 8 lines

I have changed the cxgb_pio_copy() function to only take tx_desc pointers. This isn't really a loss of flexibility because anything else was buggy to begin with.
I also removed the casting on the destination pointer since that was unnecessary and a bit messy.
Signed-off-by Dan Carpenter <[email protected]>

Applied.