2020-07-13 12:15:19

by Suraj Upadhyay

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

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

Attachments:

2020-07-13 12:17:23

by Suraj Upadhyay

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

Simplify while loops into more readable and simple for loops.

Signed-off-by: Suraj Upadhyay <[email protected]>
---
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

Attachments:

2020-07-13 12:18:53

by Suraj Upadhyay

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

Issue found with checkpatch.pl

Signed-off-by: Suraj Upadhyay <[email protected]>
---
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

Attachments:

2020-07-13 12:18:56

by Suraj Upadhyay

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

Issue found by checkpatch.pl

Signed-off-by: Suraj Upadhyay <[email protected]>
---
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

Attachments:

2020-07-13 12:21:05

by Suraj Upadhyay

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

Simplify while loops into more readable and simple for loops.

Signed-off-by: Suraj Upadhyay <[email protected]>
---
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

Attachments:

2020-07-13 12:22:20

by Suraj Upadhyay

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

Simplify while loops into more readable and simple for loops.

Signed-off-by: Suraj Upadhyay <[email protected]>
---
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

Attachments:

2020-07-13 12:25:13

by Suraj Upadhyay

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

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

Signed-off-by: Suraj Upadhyay <[email protected]>
---
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

Attachments:

2020-07-13 13:39:22

by Greg Kroah-Hartman

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

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 <[email protected]>
> ---
> 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

2020-07-13 14:13:34

by Dan Carpenter

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

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

2020-07-13 14:18:54

by Dan Carpenter

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

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 <[email protected]>
> ---
> 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

2020-07-14 05:43:34

by Benjamin Poirier

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

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 <[email protected]>
> ---
[...]
> @@ -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.

Attachments:

2020-07-14 06:41:07

by Suraj Upadhyay

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

On Mon, Jul 13, 2020 at 05:12:35PM +0300, Dan Carpenter wrote:
> 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

Hi Dan,
Thanks for your response.
Should I send a v2 of this patch or not ??
Also do you have any problems with the other two patches doing the same
thing in different files ??
I am all ears.

Thanks,

Suraj Upadhyay.

Attachments:

2020-07-14 06:44:16

by Suraj Upadhyay

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

On Tue, Jul 14, 2020 at 02:41:37PM +0900, Benjamin Poirier wrote:
> 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 <[email protected]>
> > ---
> [...]
> > @@ -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.

Thanks for pointing that out. It nearly slipped.

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

I mostly tried to convert the do-while loops, which I think are't that
obvious than while and for loops.

Thanks,

Suraj Upadhyay.

Attachments:

2020-07-14 08:31:05

by Dan Carpenter

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

On Tue, Jul 14, 2020 at 12:10:22PM +0530, Suraj Upadhyay wrote:
> On Mon, Jul 13, 2020 at 05:12:35PM +0300, Dan Carpenter wrote:
> > 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
>
> Hi Dan,
> Thanks for your response.
> Should I send a v2 of this patch or not ??
> Also do you have any problems with the other two patches doing the same
> thing in different files ??
> I am all ears.

I would just resend patch 6/6. If this is your driver and you're going
to be working on it extensively then you do what makes you feel
comfortable. But to me the original code seems fine with while count
down loops.

regards,
dan carpenter

2020-07-14 19:05:52

by Joe Perches

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

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 <[email protected]>
> > ---
> > 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);

2020-07-14 19:06:51

by Suraj Upadhyay

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

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 <[email protected]>
> > > ---
> > > 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.

Attachments:

2020-07-14 19:24:52

by Joe Perches

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

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 <[email protected]>
> > > > ---
> > > > 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

2020-07-14 19:55:56

by Suraj Upadhyay

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

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 <[email protected]>
> > > > > ---
> > > > > 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.

Attachments: