[patch 1/2 -next] cxgb4: clean up a type issue

[patch 1/2 -next] cxgb4: clean up a type issue

[Dan Carpenter]

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 <dan.carpenter@oracle.com>

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index bb7851e..599cdfd 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -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);
 		}

[patch 1/2 -next] cxgb4: clean up a type issue

[Dan Carpenter]

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 <dan.carpenter@oracle.com>

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index bb7851e..599cdfd 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -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);
 		}

RE: [patch 1/2 -next] cxgb4: clean up a type issue

[David Laight]

From: Dan Carpenter
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
> index bb7851e..599cdfd 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
> @@ -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);

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

>  		} 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [patch 1/2 -next] cxgb4: clean up a type issue

[David Laight]

From: Dan Carpenter
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
> index bb7851e..599cdfd 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
> @@ -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);

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

>  		} 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 1/2 -next] cxgb4: clean up a type issue

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Thu, 2 Oct 2014 14:22:19 +0300

> 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 <dan.carpenter@oracle.com>

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

Thanks!

Re: [patch 1/2 -next] cxgb4: clean up a type issue

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Thu, 2 Oct 2014 14:22:19 +0300

> 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 <dan.carpenter@oracle.com>

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

Thanks!

Re: [patch 1/2 -next] cxgb4: clean up a type issue

[Dan Carpenter]

On Fri, Oct 03, 2014 at 03:46:29PM -0700, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Thu, 2 Oct 2014 14:22:19 +0300
> 
> > 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 <dan.carpenter@oracle.com>
> 
> 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

Re: [patch 1/2 -next] cxgb4: clean up a type issue

[Dan Carpenter]

On Fri, Oct 03, 2014 at 03:46:29PM -0700, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Thu, 2 Oct 2014 14:22:19 +0300
> 
> > 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 <dan.carpenter@oracle.com>
> 
> 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

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

[Dan Carpenter]

"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 <dan.carpenter@oracle.com>
---
No changes since v1.

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index bb7851e..e8e90ce 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -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;

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

[Dan Carpenter]

"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 <dan.carpenter@oracle.com>
---
No changes since v1.

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index bb7851e..e8e90ce 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -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;

[patch 2/2 v2 -next] cxgb4: clean up a type issue

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
v2: More cleanups as suggested by David Laight.
    Update cxgb_pio_copy() comments.

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index e8e90ce..fab4c84 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -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);
 		}

[patch 2/2 v2 -next] cxgb4: clean up a type issue

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
v2: More cleanups as suggested by David Laight.
    Update cxgb_pio_copy() comments.

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index e8e90ce..fab4c84 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -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);
 		}

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

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 8 Oct 2014 16:43:17 +0300

> "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 <dan.carpenter@oracle.com>

Applied.

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

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 8 Oct 2014 16:43:17 +0300

> "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 <dan.carpenter@oracle.com>

Applied.

Re: [patch 2/2 v2 -next] cxgb4: clean up a type issue

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 8 Oct 2014 16:44:34 +0300

> 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 <dan.carpenter@oracle.com>

Applied.

Re: [patch 2/2 v2 -next] cxgb4: clean up a type issue

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 8 Oct 2014 16:44:34 +0300

> 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 <dan.carpenter@oracle.com>

Applied.