[PATCH 1/2] staging: qlge: fix trailing */ in block comment

[PATCH 1/2] staging: qlge: fix trailing */ in block comment

[Coiby Xu]

Remove trailing "*/" in block comments.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge_main.c |  3 ++-
 drivers/staging/qlge/qlge_mpi.c  | 10 ++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 1650de13842f..aaecf2b0f9a1 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -3244,7 +3244,8 @@ static void ql_set_irq_mask(struct ql_adapter *qdev, struct intr_context *ctx)
 		 */
 		ctx->irq_mask = (1 << qdev->rx_ring[vect].cq_id);
 		/* Add the TX ring(s) serviced by this vector
-		 * to the mask. */
+		 * to the mask.
+		 */
 		for (j = 0; j < tx_rings_per_vector; j++) {
 			ctx->irq_mask |=
 			(1 << qdev->rx_ring[qdev->rss_ring_count +
diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
index 60c08d9cc034..3bb08d290525 100644
--- a/drivers/staging/qlge/qlge_mpi.c
+++ b/drivers/staging/qlge/qlge_mpi.c
@@ -389,7 +389,8 @@ static void ql_init_fw_done(struct ql_adapter *qdev, struct mbox_params *mbcp)
  *  This can get called iteratively from the mpi_work thread
  *  when events arrive via an interrupt.
  *  It also gets called when a mailbox command is polling for
- *  it's completion. */
+ *  it's completion.
+ */
 static int ql_mpi_handler(struct ql_adapter *qdev, struct mbox_params *mbcp)
 {
 	int status;
@@ -520,7 +521,7 @@ static int ql_mpi_handler(struct ql_adapter *qdev, struct mbox_params *mbcp)
 	 * changed when a mailbox command is waiting
 	 * for a response and an AEN arrives and
 	 * is handled.
-	 * */
+	 */
 	mbcp->out_count = orig_count;
 	return status;
 }
@@ -555,7 +556,8 @@ static int ql_mailbox_command(struct ql_adapter *qdev, struct mbox_params *mbcp)
 	 * here because some AEN might arrive while
 	 * we're waiting for the mailbox command to
 	 * complete. If more than 5 seconds expire we can
-	 * assume something is wrong. */
+	 * assume something is wrong.
+	 */
 	count = jiffies + HZ * MAILBOX_TIMEOUT;
 	do {
 		/* Wait for the interrupt to come in. */
@@ -1178,7 +1180,7 @@ void ql_mpi_idc_work(struct work_struct *work)
 		/* Signal the resulting link up AEN
 		 * that the frame routing and mac addr
 		 * needs to be set.
-		 * */
+		 */
 		set_bit(QL_CAM_RT_SET, &qdev->flags);
 		/* Do ACK if required */
 		if (timeout) {
--
2.27.0

[PATCH 2/2] staging: qlge: fix else after return or break

[Coiby Xu]

Remove unnecessary elses after return or break.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge_dbg.c  | 23 ++++++++++-------------
 drivers/staging/qlge/qlge_main.c |  8 ++++----
 drivers/staging/qlge/qlge_mpi.c  |  4 ++--
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index 058889687907..87433510a224 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -1391,12 +1391,11 @@ static void ql_dump_cam_entries(struct ql_adapter *qdev)
 			pr_err("%s: Failed read of mac index register\n",
 			       __func__);
 			return;
-		} else {
-			if (value[0])
-				pr_err("%s: CAM index %d CAM Lookup Lower = 0x%.08x:%.08x, Output = 0x%.08x\n",
-				       qdev->ndev->name, i, value[1], value[0],
-				       value[2]);
 		}
+		if (value[0])
+			pr_err("%s: CAM index %d CAM Lookup Lower = 0x%.08x:%.08x, Output = 0x%.08x\n",
+			       qdev->ndev->name, i, value[1], value[0],
+			       value[2]);
 	}
 	for (i = 0; i < 32; i++) {
 		if (ql_get_mac_addr_reg
@@ -1404,11 +1403,10 @@ static void ql_dump_cam_entries(struct ql_adapter *qdev)
 			pr_err("%s: Failed read of mac index register\n",
 			       __func__);
 			return;
-		} else {
-			if (value[0])
-				pr_err("%s: MCAST index %d CAM Lookup Lower = 0x%.08x:%.08x\n",
-				       qdev->ndev->name, i, value[1], value[0]);
 		}
+		if (value[0])
+			pr_err("%s: MCAST index %d CAM Lookup Lower = 0x%.08x:%.08x\n",
+			       qdev->ndev->name, i, value[1], value[0]);
 	}
 	ql_sem_unlock(qdev, SEM_MAC_ADDR_MASK);
 }
@@ -1427,11 +1425,10 @@ void ql_dump_routing_entries(struct ql_adapter *qdev)
 			pr_err("%s: Failed read of routing index register\n",
 			       __func__);
 			return;
-		} else {
-			if (value)
-				pr_err("%s: Routing Mask %d = 0x%.08x\n",
-				       qdev->ndev->name, i, value);
 		}
+		if (value)
+			pr_err("%s: Routing Mask %d = 0x%.08x\n",
+			       qdev->ndev->name, i, value);
 	}
 	ql_sem_unlock(qdev, SEM_RT_IDX_MASK);
 }
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index aaecf2b0f9a1..0054c454506b 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -3778,10 +3778,10 @@ static int ql_wol(struct ql_adapter *qdev)
 				  "Failed to set magic packet on %s.\n",
 				  qdev->ndev->name);
 			return status;
-		} else
-			netif_info(qdev, drv, qdev->ndev,
-				   "Enabled magic packet successfully on %s.\n",
-				   qdev->ndev->name);
+		}
+		netif_info(qdev, drv, qdev->ndev,
+			   "Enabled magic packet successfully on %s.\n",
+			   qdev->ndev->name);

 		wol |= MB_WOL_MAGIC_PKT;
 	}
diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
index 3bb08d290525..fa178fc642a6 100644
--- a/drivers/staging/qlge/qlge_mpi.c
+++ b/drivers/staging/qlge/qlge_mpi.c
@@ -276,8 +276,8 @@ static void ql_link_up(struct ql_adapter *qdev, struct mbox_params *mbcp)
 			netif_err(qdev, ifup, qdev->ndev,
 				  "Failed to init CAM/Routing tables.\n");
 			return;
-		} else
-			clear_bit(QL_CAM_RT_SET, &qdev->flags);
+		}
+		clear_bit(QL_CAM_RT_SET, &qdev->flags);
 	}

 	/* Queue up a worker to check the frame
--
2.27.0

Re: [PATCH 2/2] staging: qlge: fix else after return or break

[Joe Perches]

On Fri, 2020-06-26 at 05:57 +0800, Coiby Xu wrote:
> Remove unnecessary elses after return or break.

unrelated trivia:

> diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
[]
> @@ -1391,12 +1391,11 @@ static void ql_dump_cam_entries(struct ql_adapter *qdev)
>  			pr_err("%s: Failed read of mac index register\n",
>  			       __func__);
>  			return;
> -		} else {
> -			if (value[0])
> -				pr_err("%s: CAM index %d CAM Lookup Lower = 0x%.08x:%.08x, Output = 0x%.08x\n",
> -				       qdev->ndev->name, i, value[1], value[0],
> -				       value[2]);

looks like all of these could use netdev_err

				netdev_err(qdev, "etc...",
					   i, value[1], value[0], value[2]);

etc...

Re: [PATCH 2/2] staging: qlge: fix else after return or break

[Dan Carpenter]

On Fri, Jun 26, 2020 at 05:57:55AM +0800, Coiby Xu wrote:
> @@ -1404,11 +1403,10 @@ static void ql_dump_cam_entries(struct ql_adapter *qdev)
>  			pr_err("%s: Failed read of mac index register\n",
>  			       __func__);
>  			return;
                        ^^^^^^
> -		} else {
> -			if (value[0])
> -				pr_err("%s: MCAST index %d CAM Lookup Lower = 0x%.08x:%.08x\n",
> -				       qdev->ndev->name, i, value[1], value[0]);
>  		}
> +		if (value[0])
> +			pr_err("%s: MCAST index %d CAM Lookup Lower = 0x%.08x:%.08x\n",
> +			       qdev->ndev->name, i, value[1], value[0]);
>  	}
>  	ql_sem_unlock(qdev, SEM_MAC_ADDR_MASK);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  }
> @@ -1427,11 +1425,10 @@ void ql_dump_routing_entries(struct ql_adapter *qdev)
>  			pr_err("%s: Failed read of routing index register\n",
>  			       __func__);
>  			return;
                        ^^^^^^


> -		} else {
> -			if (value)
> -				pr_err("%s: Routing Mask %d = 0x%.08x\n",
> -				       qdev->ndev->name, i, value);
>  		}
> +		if (value)
> +			pr_err("%s: Routing Mask %d = 0x%.08x\n",
> +			       qdev->ndev->name, i, value);
>  	}
>  	ql_sem_unlock(qdev, SEM_RT_IDX_MASK);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  }

This is not caused by your patch, but in these two functions we return
without dropping the lock.  There may be other places as well, but these
are the two I can see without leaving my email client.

Do you think you could fix that before we forget?  Just change the
return to a break to fix the bug.

regards,
dan carpenter

Re: [PATCH 2/2] staging: qlge: fix else after return or break

[Coiby Xu]

On Fri, Jun 26, 2020 at 11:34:36AM +0300, Dan Carpenter wrote:
>On Fri, Jun 26, 2020 at 05:57:55AM +0800, Coiby Xu wrote:
>> @@ -1404,11 +1403,10 @@ static void ql_dump_cam_entries(struct ql_adapter *qdev)
>>  			pr_err("%s: Failed read of mac index register\n",
>>  			       __func__);
>>  			return;
>                        ^^^^^^
>> -		} else {
>> -			if (value[0])
>> -				pr_err("%s: MCAST index %d CAM Lookup Lower = 0x%.08x:%.08x\n",
>> -				       qdev->ndev->name, i, value[1], value[0]);
>>  		}
>> +		if (value[0])
>> +			pr_err("%s: MCAST index %d CAM Lookup Lower = 0x%.08x:%.08x\n",
>> +			       qdev->ndev->name, i, value[1], value[0]);
>>  	}
>>  	ql_sem_unlock(qdev, SEM_MAC_ADDR_MASK);
>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>  }
>> @@ -1427,11 +1425,10 @@ void ql_dump_routing_entries(struct ql_adapter *qdev)
>>  			pr_err("%s: Failed read of routing index register\n",
>>  			       __func__);
>>  			return;
>                        ^^^^^^
>
>
>> -		} else {
>> -			if (value)
>> -				pr_err("%s: Routing Mask %d = 0x%.08x\n",
>> -				       qdev->ndev->name, i, value);
>>  		}
>> +		if (value)
>> +			pr_err("%s: Routing Mask %d = 0x%.08x\n",
>> +			       qdev->ndev->name, i, value);
>>  	}
>>  	ql_sem_unlock(qdev, SEM_RT_IDX_MASK);
>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>  }
>
>This is not caused by your patch, but in these two functions we return
>without dropping the lock.  There may be other places as well, but these
>are the two I can see without leaving my email client.
>
>Do you think you could fix that before we forget?  Just change the
>return to a break to fix the bug.

Sure, I'll address this issue in the next series
of patches. Thank you for bringing up this issue!

--
Best regards,
Coiby

Re: [PATCH 2/2] staging: qlge: fix else after return or break

[Coiby Xu]

On Thu, Jun 25, 2020 at 03:13:14PM -0700, Joe Perches wrote:
>On Fri, 2020-06-26 at 05:57 +0800, Coiby Xu wrote:
>> Remove unnecessary elses after return or break.
>
>unrelated trivia:
>
>> diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
>[]
>> @@ -1391,12 +1391,11 @@ static void ql_dump_cam_entries(struct ql_adapter *qdev)
>>  			pr_err("%s: Failed read of mac index register\n",
>>  			       __func__);
>>  			return;
>> -		} else {
>> -			if (value[0])
>> -				pr_err("%s: CAM index %d CAM Lookup Lower = 0x%.08x:%.08x, Output = 0x%.08x\n",
>> -				       qdev->ndev->name, i, value[1], value[0],
>> -				       value[2]);
>
>looks like all of these could use netdev_err
>
>				netdev_err(qdev, "etc...",
>					   i, value[1], value[0], value[2]);
>
>etc...
>
Interestingly, scripts/checkpatch.pl couldn't detect this problem.
I once used printk(KERN_ALERT...) and the script would properly warn me
that,

     WARNING: Prefer [subsystem eg: netdev]_alert([subsystem]dev, ... then dev_alert(dev, ... then pr_alert(...  to printk(KERN_ALERT .

I'll fix this issue when sending another version of the patches. Thank
you for the reminding!

--
Best regards,
Coiby

Re: [PATCH 2/2] staging: qlge: fix else after return or break

[Coiby Xu]

On Thu, Jun 25, 2020 at 03:13:14PM -0700, Joe Perches wrote:
>On Fri, 2020-06-26 at 05:57 +0800, Coiby Xu wrote:
>> Remove unnecessary elses after return or break.
>
>unrelated trivia:
>
>> diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
>[]
>> @@ -1391,12 +1391,11 @@ static void ql_dump_cam_entries(struct ql_adapter *qdev)
>>  			pr_err("%s: Failed read of mac index register\n",
>>  			       __func__);
>>  			return;
>> -		} else {
>> -			if (value[0])
>> -				pr_err("%s: CAM index %d CAM Lookup Lower = 0x%.08x:%.08x, Output = 0x%.08x\n",
>> -				       qdev->ndev->name, i, value[1], value[0],
>> -				       value[2]);
>
>looks like all of these could use netdev_err
>
>				netdev_err(qdev, "etc...",
>					   i, value[1], value[0], value[2]);
>
>etc...

Should we also replace all pr_errs with netdev_err in
ql_dump_* functions? I'm not sure how we will use ql_dump_*. For example,
ql_dump_regs is not referred by any kernel source, so I guess it's for
the sole purpose of debugging the driver by the developer. But one
pr_err in ql_dump_routing_entries which is called by dl_dump_regs doesn't
prints out the device name whereas the other does,

> void ql_dump_routing_entries(struct ql_adapter *qdev)
> {
> 	int i;
> 	u32 value;
>
> 	i = ql_sem_spinlock(qdev, SEM_RT_IDX_MASK);
> 	if (i)
> 		return;
> 	for (i = 0; i < 16; i++) {
> 		value = 0;
> 		if (ql_get_routing_reg(qdev, i, &value)) {
> 			pr_err("%s: Failed read of routing index register\n",
> 			       __func__);
> 			break;
> 		}
> 		if (value)
> 			pr_err("%s: Routing Mask %d = 0x%.08x\n",
> 			       qdev->ndev->name, i, value);
> 	}

--
Best regards,
Coiby

Re: [PATCH 2/2] staging: qlge: fix else after return or break

[Joe Perches]

On Sat, 2020-06-27 at 07:57 +0800, Coiby Xu wrote:
> On Thu, Jun 25, 2020 at 03:13:14PM -0700, Joe Perches wrote:
> > On Fri, 2020-06-26 at 05:57 +0800, Coiby Xu wrote:
> > > Remove unnecessary elses after return or break.
> > 
> > unrelated trivia:
[]
> > looks like all of these could use netdev_err
[]
> should we also replace all pr_errs with netdev_err in
> ql_dump_* functions?

Ideally, anywhere a struct netdevice * is available, it should
be used to output netdev_<level> in preference to pr_<level>.

Re: [PATCH 2/2] staging: qlge: fix else after return or break

[Coiby Xu]

On Fri, Jun 26, 2020 at 05:06:33PM -0700, Joe Perches wrote:
>On Sat, 2020-06-27 at 07:57 +0800, Coiby Xu wrote:
>> On Thu, Jun 25, 2020 at 03:13:14PM -0700, Joe Perches wrote:
>> > On Fri, 2020-06-26 at 05:57 +0800, Coiby Xu wrote:
>> > > Remove unnecessary elses after return or break.
>> >
>> > unrelated trivia:
>[]
>> > looks like all of these could use netdev_err
>[]
>> should we also replace all pr_errs with netdev_err in
>> ql_dump_* functions?
>
>Ideally, anywhere a struct netdevice * is available, it should
>be used to output netdev_<level> in preference to pr_<level>.

Thank you for the explaining!

--
Best regards,
Coiby