[PATCH 0/6] staging: qlge: General cleanup and refactor.

[PATCH 0/6] staging: qlge: General cleanup and refactor.

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

Hii,
	This patchest aims to remove several of the checkpatch.pl
warnings and refactor some ugly while loops into for loops for better
readability.
Some of the issues are found with checkpatch and others were listed in
qlge/TODO.

Thanks,

Suraj Upadhyay (6):
  staging: qlge: qlge.h: Function definition arguments should have
    names.
  staging: qlge: qlge.h: Insert line after declaration.
  staging: qlge: qlge_dbg: Simplify while statements
  staging: qlge: qlge_main: Simplify while statements.
  staging: qlge: qlge_mpi: Simplify while statements.
  staging: qlge: qlge_ethtool: Remove one byte memset.

 drivers/staging/qlge/qlge.h         |  7 +++--
 drivers/staging/qlge/qlge_dbg.c     |  5 ++-
 drivers/staging/qlge/qlge_ethtool.c |  4 +--
 drivers/staging/qlge/qlge_main.c    | 49 +++++++++++++----------------
 drivers/staging/qlge/qlge_mpi.c     | 32 +++++++++----------
 5 files changed, 45 insertions(+), 52 deletions(-)

-- 
2.17.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/6] staging: qlge: qlge.h: Function definition arguments should have names.

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

Issue found with checkpatch.pl

Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
---
 drivers/staging/qlge/qlge.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 05e4f47442a3..48bc494028ce 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2057,8 +2057,8 @@ enum {
 };
 
 struct nic_operations {
-	int (*get_flash)(struct ql_adapter *);
-	int (*port_initialize)(struct ql_adapter *);
+	int (*get_flash)(struct ql_adapter *qdev);
+	int (*port_initialize)(struct ql_adapter *qdev);
 };
 
 /*
@@ -2275,7 +2275,7 @@ int ql_mb_set_port_cfg(struct ql_adapter *qdev);
 int ql_wait_fifo_empty(struct ql_adapter *qdev);
 void ql_get_dump(struct ql_adapter *qdev, void *buff);
 netdev_tx_t ql_lb_send(struct sk_buff *skb, struct net_device *ndev);
-void ql_check_lb_frame(struct ql_adapter *, struct sk_buff *);
+void ql_check_lb_frame(struct ql_adapter *qdev, struct sk_buff *skb);
 int ql_own_firmware(struct ql_adapter *qdev);
 int ql_clean_lb_rx_ring(struct rx_ring *rx_ring, int budget);
 
-- 
2.17.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/6] staging: qlge: qlge.h: Insert line after declaration.

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

Issue found by checkpatch.pl

Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
---
 drivers/staging/qlge/qlge.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 48bc494028ce..483ce04789ed 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2224,6 +2224,7 @@ static inline void ql_write_db_reg_relaxed(u32 val, void __iomem *addr)
 static inline u32 ql_read_sh_reg(__le32  *addr)
 {
 	u32 reg;
+
 	reg =  le32_to_cpu(*addr);
 	rmb();
 	return reg;
-- 
2.17.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/6] staging: qlge: qlge_dbg: Simplify while statements

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

Simplify while loops into more readable and simple for loops.

Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
---
 drivers/staging/qlge/qlge_dbg.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index 32fbd30a6a2e..985a6c341294 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -42,9 +42,9 @@ static int ql_wait_other_func_reg_rdy(struct ql_adapter *qdev, u32 reg,
 				      u32 bit, u32 err_bit)
 {
 	u32 temp;
-	int count = 10;
+	int count;
 
-	while (count) {
+	for (count = 10; count; count--) {
 		temp = ql_read_other_func_reg(qdev, reg);
 
 		/* check for errors */
@@ -53,7 +53,6 @@ static int ql_wait_other_func_reg_rdy(struct ql_adapter *qdev, u32 reg,
 		else if (temp & bit)
 			return 0;
 		mdelay(10);
-		count--;
 	}
 	return -1;
 }
-- 
2.17.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/6] staging: qlge: qlge_main: Simplify while statements.

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 5664 bytes --]

Simplify while loops into more readable and simple for loops.

Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
---
 drivers/staging/qlge/qlge_main.c | 49 ++++++++++++++------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index f7e26defb844..98710d3d4429 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -138,13 +138,11 @@ static int ql_sem_trylock(struct ql_adapter *qdev, u32 sem_mask)
 
 int ql_sem_spinlock(struct ql_adapter *qdev, u32 sem_mask)
 {
-	unsigned int wait_count = 30;
+	unsigned int wait_count;

-	do {
+	for (wait_count = 30; wait_count; wait_count--) {
 		if (!ql_sem_trylock(qdev, sem_mask))
 			return 0;
 		udelay(100);
-	} while (--wait_count);
+	}
 	return -ETIMEDOUT;
 }
 
@@ -1101,7 +1099,7 @@ static int qlge_refill_bq(struct qlge_bq *bq, gfp_t gfp)
 	i = bq->next_to_use;
 	bq_desc = &bq->queue[i];
 	i -= QLGE_BQ_LEN;
-	do {
+	for (; refill_count; refill_count--) {
 		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
 			     "ring %u %s: try cleaning idx %d\n",
 			     rx_ring->cq_id, bq_type_name[bq->type], i);
@@ -1123,8 +1121,7 @@ static int qlge_refill_bq(struct qlge_bq *bq, gfp_t gfp)
 			bq_desc = &bq->queue[0];
 			i -= QLGE_BQ_LEN;
 		}
-		refill_count--;
-	} while (refill_count);
+	}
 	i += QLGE_BQ_LEN;
 
 	if (bq->next_to_use != i) {
@@ -1824,7 +1821,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
 			sbq_desc->p.skb = NULL;
 			skb_reserve(skb, NET_IP_ALIGN);
 		}
-		do {
+		for (; length > 0; length -= size, i++) {
 			lbq_desc = ql_get_curr_lchunk(qdev, rx_ring);
 			size = min(length, qdev->lbq_buf_size);
 
@@ -1839,7 +1836,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
 			skb->truesize += size;
 			length -= size;
 			i++;
-		} while (length > 0);
+		}
 		ql_update_mac_hdr_len(qdev, ib_mac_rsp, lbq_desc->p.pg_chunk.va,
 				      &hlen);
 		__pskb_pull_tail(skb, hlen);
@@ -2098,11 +2095,11 @@ static int ql_clean_outbound_rx_ring(struct rx_ring *rx_ring)
 	struct ql_adapter *qdev = rx_ring->qdev;
 	u32 prod = ql_read_sh_reg(rx_ring->prod_idx_sh_reg);
 	struct ob_mac_iocb_rsp *net_rsp = NULL;
-	int count = 0;
+	int count;
 
 	struct tx_ring *tx_ring;
 	/* While there are entries in the completion queue. */
-	while (prod != rx_ring->cnsmr_idx) {
+	for (count = 0; prod != rx_ring->cnsmr_idx; count++) {
 
 		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
 			     "cq_id = %d, prod = %d, cnsmr = %d\n",
@@ -2121,7 +2118,6 @@ static int ql_clean_outbound_rx_ring(struct rx_ring *rx_ring)
 				     "Hit default case, not handled! dropping the packet, opcode = %x.\n",
 				     net_rsp->opcode);
 		}
-		count++;
 		ql_update_cq(rx_ring);
 		prod = ql_read_sh_reg(rx_ring->prod_idx_sh_reg);
 	}
@@ -2146,10 +2142,10 @@ static int ql_clean_inbound_rx_ring(struct rx_ring *rx_ring, int budget)
 	struct ql_adapter *qdev = rx_ring->qdev;
 	u32 prod = ql_read_sh_reg(rx_ring->prod_idx_sh_reg);
 	struct ql_net_rsp_iocb *net_rsp;
-	int count = 0;
+	int count;
 
 	/* While there are entries in the completion queue. */
-	while (prod != rx_ring->cnsmr_idx) {
+	for (count = 0; prod != rx_ring->cnsmr_idx; count++) {
 
 		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
 			     "cq_id = %d, prod = %d, cnsmr = %d\n",
@@ -2174,7 +2170,6 @@ static int ql_clean_inbound_rx_ring(struct rx_ring *rx_ring, int budget)
 				     net_rsp->opcode);
 			break;
 		}
-		count++;
 		ql_update_cq(rx_ring);
 		prod = ql_read_sh_reg(rx_ring->prod_idx_sh_reg);
 		if (count == budget)
@@ -3026,13 +3021,12 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
 		cqicb->flags |= FLAGS_LL;	/* Load lbq values */
 		tmp = (u64)rx_ring->lbq.base_dma;
 		base_indirect_ptr = rx_ring->lbq.base_indirect;
-		page_entries = 0;
-		do {
+
+		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
+		     page_entries++, base_indirect_ptr++) {
 			*base_indirect_ptr = cpu_to_le64(tmp);
 			tmp += DB_PAGE_SIZE;
-			base_indirect_ptr++;
-			page_entries++;
-		} while (page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
+		}
 		cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
 		cqicb->lbq_buf_size =
 			cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
@@ -3043,13 +3037,12 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
 		cqicb->flags |= FLAGS_LS;	/* Load sbq values */
 		tmp = (u64)rx_ring->sbq.base_dma;
 		base_indirect_ptr = rx_ring->sbq.base_indirect;
-		page_entries = 0;
-		do {
+
+		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
+		     page_entries++, base_indirect_ptr++) {
 			*base_indirect_ptr = cpu_to_le64(tmp);
 			tmp += DB_PAGE_SIZE;
-			base_indirect_ptr++;
-			page_entries++;
-		} while (page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
+		}
 		cqicb->sbq_addr =
 		    cpu_to_le64(rx_ring->sbq.base_indirect_dma);
 		cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUFFER_SIZE);
@@ -4036,9 +4029,11 @@ static int ql_change_rx_buffers(struct ql_adapter *qdev)
 
 	/* Wait for an outstanding reset to complete. */
 	if (!test_bit(QL_ADAPTER_UP, &qdev->flags)) {
-		int i = 4;
+		int i;
 
-		while (--i && !test_bit(QL_ADAPTER_UP, &qdev->flags)) {
+		for (i = 3; i; i--) {
+			if test_bit(QL_ADAPTER_UP, &qdev->flags)
+				break;
 			netif_err(qdev, ifup, qdev->ndev,
 				  "Waiting for adapter UP...\n");
 			ssleep(1);
-- 
2.17.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/6] staging: qlge: qlge_main: Simplify while statements.

[Greg KH]

On Mon, Jul 13, 2020 at 05:50:14PM +0530, Suraj Upadhyay wrote:
> Simplify while loops into more readable and simple for loops.
> 
> Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
> ---
>  drivers/staging/qlge/qlge_main.c | 49 ++++++++++++++------------------
>  1 file changed, 22 insertions(+), 27 deletions(-)

This patch did not apply for some odd reason :(

Please rebase and resend

thanks,

greg k-h

Re: [PATCH 4/6] staging: qlge: qlge_main: Simplify while statements.

[Dan Carpenter]

On Mon, Jul 13, 2020 at 05:50:14PM +0530, Suraj Upadhyay wrote:
> Simplify while loops into more readable and simple for loops.
> 

I don't think either is more clear that the other.  Walter Harms hates
count down loops and he's not entirely wrong...

regards,
dan carpenter

Re: [PATCH 4/6] staging: qlge: qlge_main: Simplify while statements.

[Benjamin Poirier]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

On 2020-07-13 17:50 +0530, Suraj Upadhyay wrote:
> Simplify while loops into more readable and simple for loops.
> 
> Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
> ---
[...]
> @@ -1824,7 +1821,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
>  			sbq_desc->p.skb = NULL;
>  			skb_reserve(skb, NET_IP_ALIGN);
>  		}
> -		do {
> +		for (; length > 0; length -= size, i++) {
>  			lbq_desc = ql_get_curr_lchunk(qdev, rx_ring);
>  			size = min(length, qdev->lbq_buf_size);
>  
> @@ -1839,7 +1836,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
>  			skb->truesize += size;
>  			length -= size;
>  			i++;
> -		} while (length > 0);
> +		}

Looks like length and i modification should be removed from here. But in
this instance, maybe the original was better anyways.

Agreed with Dan. At least some of those loops can be converted to "count
up" loops for a more familiar appearance.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/6] staging: qlge: qlge_mpi: Simplify while statements.

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 2994 bytes --]

Simplify while loops into more readable and simple for loops.

Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
---
 drivers/staging/qlge/qlge_mpi.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
index fa178fc642a6..3b71e5fc2cd0 100644
--- a/drivers/staging/qlge/qlge_mpi.c
+++ b/drivers/staging/qlge/qlge_mpi.c
@@ -17,36 +17,34 @@ int ql_unpause_mpi_risc(struct ql_adapter *qdev)
 int ql_pause_mpi_risc(struct ql_adapter *qdev)
 {
 	u32 tmp;
-	int count = UDELAY_COUNT;
+	int count;
 
 	/* Pause the RISC */
 	ql_write32(qdev, CSR, CSR_CMD_SET_PAUSE);
-	do {
+	for (count = UDELAY_COUNT; count; count--) {
 		tmp = ql_read32(qdev, CSR);
 		if (tmp & CSR_RP)
 			break;
 		mdelay(UDELAY_DELAY);
-		count--;
-	} while (count);
+	}
 	return (count == 0) ? -ETIMEDOUT : 0;
 }
 
 int ql_hard_reset_mpi_risc(struct ql_adapter *qdev)
 {
 	u32 tmp;
-	int count = UDELAY_COUNT;
+	int count;
 
 	/* Reset the RISC */
 	ql_write32(qdev, CSR, CSR_CMD_SET_RST);
-	do {
+	for (count = UDELAY_COUNT; count; count--) {
 		tmp = ql_read32(qdev, CSR);
 		if (tmp & CSR_RR) {
 			ql_write32(qdev, CSR, CSR_CMD_CLR_RST);
 			break;
 		}
 		mdelay(UDELAY_DELAY);
-		count--;
-	} while (count);
+	}
 	return (count == 0) ? -ETIMEDOUT : 0;
 }
 
@@ -147,15 +145,15 @@ static int ql_get_mb_sts(struct ql_adapter *qdev, struct mbox_params *mbcp)
  */
 static int ql_wait_mbx_cmd_cmplt(struct ql_adapter *qdev)
 {
-	int count = 100;
+	int count;
 	u32 value;
 
-	do {
+	for (count = 100; count; count--) {
 		value = ql_read32(qdev, STS);
 		if (value & STS_PI)
 			return 0;
 		mdelay(UDELAY_DELAY); /* 100ms */
-	} while (--count);
+	}
 	return -ETIMEDOUT;
 }
 
@@ -913,10 +911,10 @@ int ql_mb_wol_set_magic(struct ql_adapter *qdev, u32 enable_wol)
 static int ql_idc_wait(struct ql_adapter *qdev)
 {
 	int status = -ETIMEDOUT;
-	long wait_time = 1 * HZ;
 	struct mbox_params *mbcp = &qdev->idc_mbc;
+	long wait_time;
 
-	do {
+	for (wait_time = 1 * HZ; wait_time;) {
 		/* Wait here for the command to complete
 		 * via the IDC process.
 		 */
@@ -946,7 +944,7 @@ static int ql_idc_wait(struct ql_adapter *qdev)
 			status = -EIO;
 			break;
 		}
-	} while (wait_time);
+	}
 
 	return status;
 }
@@ -1079,18 +1077,18 @@ static int ql_mb_get_mgmnt_traffic_ctl(struct ql_adapter *qdev, u32 *control)
 
 int ql_wait_fifo_empty(struct ql_adapter *qdev)
 {
-	int count = 5;
+	int count;
 	u32 mgmnt_fifo_empty;
 	u32 nic_fifo_empty;
 
-	do {
+	for (count = 6; count; count--) {
 		nic_fifo_empty = ql_read32(qdev, STS) & STS_NFE;
 		ql_mb_get_mgmnt_traffic_ctl(qdev, &mgmnt_fifo_empty);
 		mgmnt_fifo_empty &= MB_GET_MPI_TFK_FIFO_EMPTY;
 		if (nic_fifo_empty && mgmnt_fifo_empty)
 			return 0;
 		msleep(100);
-	} while (count-- > 0);
+	}
 	return -ETIMEDOUT;
 }
 
-- 
2.17.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 6/6] staging: qlge: qlge_ethtool: Remove one byte memset.

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

Use direct assignment instead of using memset with just one byte as an
argument.
Issue found by checkpatch.pl.

Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
---
Hii Maintainers,
	Please correct me if I am wrong here.
---

 drivers/staging/qlge/qlge_ethtool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
index 16fcdefa9687..d44b2dae9213 100644
--- a/drivers/staging/qlge/qlge_ethtool.c
+++ b/drivers/staging/qlge/qlge_ethtool.c
@@ -516,8 +516,8 @@ static void ql_create_lb_frame(struct sk_buff *skb,
 	memset(skb->data, 0xFF, frame_size);
 	frame_size &= ~1;
 	memset(&skb->data[frame_size / 2], 0xAA, frame_size / 2 - 1);
-	memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
-	memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
+	skb->data[frame_size / 2 + 10] = (unsigned char)0xBE;
+	skb->data[frame_size / 2 + 12] = (unsigned char)0xAF;
 }
 
 void ql_check_lb_frame(struct ql_adapter *qdev,
-- 
2.17.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/6] staging: qlge: qlge_ethtool: Remove one byte memset.

[Dan Carpenter]

On Mon, Jul 13, 2020 at 05:52:22PM +0530, Suraj Upadhyay wrote:
> Use direct assignment instead of using memset with just one byte as an
> argument.
> Issue found by checkpatch.pl.
> 
> Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
> ---
> Hii Maintainers,
> 	Please correct me if I am wrong here.
> ---
> 
>  drivers/staging/qlge/qlge_ethtool.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
> index 16fcdefa9687..d44b2dae9213 100644
> --- a/drivers/staging/qlge/qlge_ethtool.c
> +++ b/drivers/staging/qlge/qlge_ethtool.c
> @@ -516,8 +516,8 @@ static void ql_create_lb_frame(struct sk_buff *skb,
>  	memset(skb->data, 0xFF, frame_size);
>  	frame_size &= ~1;
>  	memset(&skb->data[frame_size / 2], 0xAA, frame_size / 2 - 1);
> -	memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> -	memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> +	skb->data[frame_size / 2 + 10] = (unsigned char)0xBE;
> +	skb->data[frame_size / 2 + 12] = (unsigned char)0xAF;

Remove the casting.

I guess this is better than the original because now it looks like
ql_check_lb_frame().  It's still really weird looking though.

regards,
dan carpenter

Re: [PATCH 6/6] staging: qlge: qlge_ethtool: Remove one byte memset.

[Joe Perches]

On Mon, 2020-07-13 at 17:17 +0300, Dan Carpenter wrote:
> On Mon, Jul 13, 2020 at 05:52:22PM +0530, Suraj Upadhyay wrote:
> > Use direct assignment instead of using memset with just one byte as an
> > argument.
> > Issue found by checkpatch.pl.
> > 
> > Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
> > ---
> > Hii Maintainers,
> > 	Please correct me if I am wrong here.
> > ---
> > 
> >  drivers/staging/qlge/qlge_ethtool.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
> > index 16fcdefa9687..d44b2dae9213 100644
> > --- a/drivers/staging/qlge/qlge_ethtool.c
> > +++ b/drivers/staging/qlge/qlge_ethtool.c
> > @@ -516,8 +516,8 @@ static void ql_create_lb_frame(struct sk_buff *skb,
> >  	memset(skb->data, 0xFF, frame_size);
> >  	frame_size &= ~1;
> >  	memset(&skb->data[frame_size / 2], 0xAA, frame_size / 2 - 1);
> > -	memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > -	memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> > +	skb->data[frame_size / 2 + 10] = (unsigned char)0xBE;
> > +	skb->data[frame_size / 2 + 12] = (unsigned char)0xAF;
> 
> Remove the casting.
> 
> I guess this is better than the original because now it looks like
> ql_check_lb_frame().  It's still really weird looking though.

There are several of these in the intel drivers too:

drivers/net/ethernet/intel/e1000/e1000_ethtool.c:       memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
drivers/net/ethernet/intel/e1000/e1000_ethtool.c:       memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
drivers/net/ethernet/intel/e1000e/ethtool.c:    memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
drivers/net/ethernet/intel/e1000e/ethtool.c:    memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
drivers/net/ethernet/intel/igb/igb_ethtool.c:   memset(&skb->data[frame_size + 10], 0xBE, 1);
drivers/net/ethernet/intel/igb/igb_ethtool.c:   memset(&skb->data[frame_size + 12], 0xAF, 1);
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:       memset(&skb->data[frame_size + 10], 0xBE, 1);
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:       memset(&skb->data[frame_size + 12], 0xAF, 1);
drivers/staging/qlge/qlge_ethtool.c:    memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
drivers/staging/qlge/qlge_ethtool.c:    memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);

Re: [PATCH 6/6] staging: qlge: qlge_ethtool: Remove one byte memset.

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]

On Tue, Jul 14, 2020 at 11:57:23AM -0700, Joe Perches wrote:
> On Mon, 2020-07-13 at 17:17 +0300, Dan Carpenter wrote:
> > On Mon, Jul 13, 2020 at 05:52:22PM +0530, Suraj Upadhyay wrote:
> > > Use direct assignment instead of using memset with just one byte as an
> > > argument.
> > > Issue found by checkpatch.pl.
> > > 
> > > Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
> > > ---
> > > Hii Maintainers,
> > > 	Please correct me if I am wrong here.
> > > ---
> > > 
> > >  drivers/staging/qlge/qlge_ethtool.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
> > > index 16fcdefa9687..d44b2dae9213 100644
> > > --- a/drivers/staging/qlge/qlge_ethtool.c
> > > +++ b/drivers/staging/qlge/qlge_ethtool.c
> > > @@ -516,8 +516,8 @@ static void ql_create_lb_frame(struct sk_buff *skb,
> > >  	memset(skb->data, 0xFF, frame_size);
> > >  	frame_size &= ~1;
> > >  	memset(&skb->data[frame_size / 2], 0xAA, frame_size / 2 - 1);
> > > -	memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > > -	memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> > > +	skb->data[frame_size / 2 + 10] = (unsigned char)0xBE;
> > > +	skb->data[frame_size / 2 + 12] = (unsigned char)0xAF;
> > 
> > Remove the casting.
> > 
> > I guess this is better than the original because now it looks like
> > ql_check_lb_frame().  It's still really weird looking though.
> 
> There are several of these in the intel drivers too:
> 
> drivers/net/ethernet/intel/e1000/e1000_ethtool.c:       memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> drivers/net/ethernet/intel/e1000/e1000_ethtool.c:       memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> drivers/net/ethernet/intel/e1000e/ethtool.c:    memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> drivers/net/ethernet/intel/e1000e/ethtool.c:    memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> drivers/net/ethernet/intel/igb/igb_ethtool.c:   memset(&skb->data[frame_size + 10], 0xBE, 1);
> drivers/net/ethernet/intel/igb/igb_ethtool.c:   memset(&skb->data[frame_size + 12], 0xAF, 1);
> drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:       memset(&skb->data[frame_size + 10], 0xBE, 1);
> drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:       memset(&skb->data[frame_size + 12], 0xAF, 1);
> drivers/staging/qlge/qlge_ethtool.c:    memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> drivers/staging/qlge/qlge_ethtool.c:    memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);

Thanks to point this out,
	I will be sending a patchset for that soon.

Thanks,

Suraj Upadhyay.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/6] staging: qlge: qlge_ethtool: Remove one byte memset.

[Joe Perches]

On Wed, 2020-07-15 at 00:36 +0530, Suraj Upadhyay wrote:
> On Tue, Jul 14, 2020 at 11:57:23AM -0700, Joe Perches wrote:
> > On Mon, 2020-07-13 at 17:17 +0300, Dan Carpenter wrote:
> > > On Mon, Jul 13, 2020 at 05:52:22PM +0530, Suraj Upadhyay wrote:
> > > > Use direct assignment instead of using memset with just one byte as an
> > > > argument.
> > > > Issue found by checkpatch.pl.
> > > > 
> > > > Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
> > > > ---
> > > > Hii Maintainers,
> > > > 	Please correct me if I am wrong here.
> > > > ---
> > > > 
> > > >  drivers/staging/qlge/qlge_ethtool.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
> > > > index 16fcdefa9687..d44b2dae9213 100644
> > > > --- a/drivers/staging/qlge/qlge_ethtool.c
> > > > +++ b/drivers/staging/qlge/qlge_ethtool.c
> > > > @@ -516,8 +516,8 @@ static void ql_create_lb_frame(struct sk_buff *skb,
> > > >  	memset(skb->data, 0xFF, frame_size);
> > > >  	frame_size &= ~1;
> > > >  	memset(&skb->data[frame_size / 2], 0xAA, frame_size / 2 - 1);
> > > > -	memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > > > -	memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> > > > +	skb->data[frame_size / 2 + 10] = (unsigned char)0xBE;
> > > > +	skb->data[frame_size / 2 + 12] = (unsigned char)0xAF;
> > > 
> > > Remove the casting.
> > > 
> > > I guess this is better than the original because now it looks like
> > > ql_check_lb_frame().  It's still really weird looking though.
> > 
> > There are several of these in the intel drivers too:
> > 
> > drivers/net/ethernet/intel/e1000/e1000_ethtool.c:       memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > drivers/net/ethernet/intel/e1000/e1000_ethtool.c:       memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> > drivers/net/ethernet/intel/e1000e/ethtool.c:    memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > drivers/net/ethernet/intel/e1000e/ethtool.c:    memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> > drivers/net/ethernet/intel/igb/igb_ethtool.c:   memset(&skb->data[frame_size + 10], 0xBE, 1);
> > drivers/net/ethernet/intel/igb/igb_ethtool.c:   memset(&skb->data[frame_size + 12], 0xAF, 1);
> > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:       memset(&skb->data[frame_size + 10], 0xBE, 1);
> > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:       memset(&skb->data[frame_size + 12], 0xAF, 1);
> > drivers/staging/qlge/qlge_ethtool.c:    memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > drivers/staging/qlge/qlge_ethtool.c:    memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> 
> Thanks to point this out,
> 	I will be sending a patchset for that soon.


It _might_ be useful to create and use a standard
mechanism for the loopback functions:

	<foo>create_lbtest_frame
and
	<foo>check_lbtest_frame

Maybe use something like:

	ether_loopback_frame_create
and
	ether_loopback_frame_check

Re: [PATCH 6/6] staging: qlge: qlge_ethtool: Remove one byte memset.

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 3559 bytes --]

On Tue, Jul 14, 2020 at 12:22:05PM -0700, Joe Perches wrote:
> On Wed, 2020-07-15 at 00:36 +0530, Suraj Upadhyay wrote:
> > On Tue, Jul 14, 2020 at 11:57:23AM -0700, Joe Perches wrote:
> > > On Mon, 2020-07-13 at 17:17 +0300, Dan Carpenter wrote:
> > > > On Mon, Jul 13, 2020 at 05:52:22PM +0530, Suraj Upadhyay wrote:
> > > > > Use direct assignment instead of using memset with just one byte as an
> > > > > argument.
> > > > > Issue found by checkpatch.pl.
> > > > > 
> > > > > Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
> > > > > ---
> > > > > Hii Maintainers,
> > > > > 	Please correct me if I am wrong here.
> > > > > ---
> > > > > 
> > > > >  drivers/staging/qlge/qlge_ethtool.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
> > > > > index 16fcdefa9687..d44b2dae9213 100644
> > > > > --- a/drivers/staging/qlge/qlge_ethtool.c
> > > > > +++ b/drivers/staging/qlge/qlge_ethtool.c
> > > > > @@ -516,8 +516,8 @@ static void ql_create_lb_frame(struct sk_buff *skb,
> > > > >  	memset(skb->data, 0xFF, frame_size);
> > > > >  	frame_size &= ~1;
> > > > >  	memset(&skb->data[frame_size / 2], 0xAA, frame_size / 2 - 1);
> > > > > -	memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > > > > -	memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> > > > > +	skb->data[frame_size / 2 + 10] = (unsigned char)0xBE;
> > > > > +	skb->data[frame_size / 2 + 12] = (unsigned char)0xAF;
> > > > 
> > > > Remove the casting.
> > > > 
> > > > I guess this is better than the original because now it looks like
> > > > ql_check_lb_frame().  It's still really weird looking though.
> > > 
> > > There are several of these in the intel drivers too:
> > > 
> > > drivers/net/ethernet/intel/e1000/e1000_ethtool.c:       memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > > drivers/net/ethernet/intel/e1000/e1000_ethtool.c:       memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> > > drivers/net/ethernet/intel/e1000e/ethtool.c:    memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > > drivers/net/ethernet/intel/e1000e/ethtool.c:    memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> > > drivers/net/ethernet/intel/igb/igb_ethtool.c:   memset(&skb->data[frame_size + 10], 0xBE, 1);
> > > drivers/net/ethernet/intel/igb/igb_ethtool.c:   memset(&skb->data[frame_size + 12], 0xAF, 1);
> > > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:       memset(&skb->data[frame_size + 10], 0xBE, 1);
> > > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:       memset(&skb->data[frame_size + 12], 0xAF, 1);
> > > drivers/staging/qlge/qlge_ethtool.c:    memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> > > drivers/staging/qlge/qlge_ethtool.c:    memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> > 
> > Thanks to point this out,
> > 	I will be sending a patchset for that soon.
> 
> 
> It _might_ be useful to create and use a standard
> mechanism for the loopback functions:
> 
> 	<foo>create_lbtest_frame
> and
> 	<foo>check_lbtest_frame
> 
> Maybe use something like:
> 
> 	ether_loopback_frame_create
> and
> 	ether_loopback_frame_check
> 
I thought about it  but then again the fram_size is sometimes divided by two

e.g. `frame_size /= 2;` or `frame_size >>= 1;`.

and sometimes it is subtracted by one. i.e. `frame_size &= ~1;`.

Anyway, I sent my layman patchset to the lkml and intel maintainers.

Forgive my brevity.

Thanks, 

Suraj Upadhyay. 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]