* [PATCH 0/2] staging: qlge: coding style fix for the qlge driver
@ 2020-06-25 21:57 Coiby Xu
2020-06-25 21:57 ` Coiby Xu
2020-06-25 21:57 ` Coiby Xu
0 siblings, 2 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-25 21:57 UTC (permalink / raw)
To: devel
These two patches fix two coding style problems for all files under
drivers/staging/qlge as reported by checkpatch.pl,
- trailing */ in block comment
- unnecessary else after return or break
Coiby Xu (2):
fix trailing */ in block comment
fix else after return or break
drivers/staging/qlge/qlge_dbg.c | 23 ++++++++++-------------
drivers/staging/qlge/qlge_main.c | 11 ++++++-----
drivers/staging/qlge/qlge_mpi.c | 14 ++++++++------
3 files changed, 24 insertions(+), 24 deletions(-)
--
2.27.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] staging: qlge: fix trailing */ in block comment
2020-06-25 21:57 [PATCH 0/2] staging: qlge: coding style fix for the qlge driver Coiby Xu
@ 2020-06-25 21:57 ` Coiby Xu
2020-06-25 21:57 ` Coiby Xu
1 sibling, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-25 21:57 UTC (permalink / raw)
To: devel
Cc: Manish Chopra, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
Greg Kroah-Hartman, open list:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 1/2] staging: qlge: fix trailing */ in block comment
@ 2020-06-25 21:57 ` Coiby Xu
0 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-25 21:57 UTC (permalink / raw)
To: devel
Cc: Greg Kroah-Hartman, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list, Manish Chopra,
open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] staging: qlge: fix else after return or break
2020-06-25 21:57 [PATCH 0/2] staging: qlge: coding style fix for the qlge driver Coiby Xu
@ 2020-06-25 21:57 ` Coiby Xu
2020-06-25 21:57 ` Coiby Xu
1 sibling, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-25 21:57 UTC (permalink / raw)
To: devel
Cc: Manish Chopra, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
Greg Kroah-Hartman, open list:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] staging: qlge: fix else after return or break
@ 2020-06-25 21:57 ` Coiby Xu
0 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-25 21:57 UTC (permalink / raw)
To: devel
Cc: Greg Kroah-Hartman, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list, Manish Chopra,
open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
2020-06-25 21:57 ` Coiby Xu
@ 2020-06-25 22:13 ` Joe Perches
-1 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2020-06-25 22:13 UTC (permalink / raw)
To: Coiby Xu, devel
Cc: Manish Chopra, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
Greg Kroah-Hartman, open list:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list
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...
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
@ 2020-06-25 22:13 ` Joe Perches
0 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2020-06-25 22:13 UTC (permalink / raw)
To: Coiby Xu, devel
Cc: Greg Kroah-Hartman, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list, Manish Chopra,
open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
2020-06-25 21:57 ` Coiby Xu
@ 2020-06-26 8:34 ` Dan Carpenter
-1 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2020-06-26 8:34 UTC (permalink / raw)
To: Coiby Xu
Cc: devel, Greg Kroah-Hartman,
supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER, open list,
Manish Chopra, open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
@ 2020-06-26 8:34 ` Dan Carpenter
0 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2020-06-26 8:34 UTC (permalink / raw)
To: Coiby Xu
Cc: devel, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER, Manish Chopra,
Greg Kroah-Hartman, open list,
open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
2020-06-26 8:34 ` Dan Carpenter
@ 2020-06-26 11:41 ` Coiby Xu
-1 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-26 11:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Greg Kroah-Hartman,
supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER, open list,
Manish Chopra, open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
@ 2020-06-26 11:41 ` Coiby Xu
0 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-26 11:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER, Manish Chopra,
Greg Kroah-Hartman, open list,
open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
2020-06-25 22:13 ` Joe Perches
@ 2020-06-26 11:47 ` Coiby Xu
-1 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-26 11:47 UTC (permalink / raw)
To: Joe Perches
Cc: devel, Manish Chopra, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
Greg Kroah-Hartman, open list:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
@ 2020-06-26 11:47 ` Coiby Xu
0 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-26 11:47 UTC (permalink / raw)
To: Joe Perches
Cc: devel, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER, Manish Chopra,
Greg Kroah-Hartman, open list,
open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
2020-06-25 22:13 ` Joe Perches
@ 2020-06-26 23:57 ` Coiby Xu
-1 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-26 23:57 UTC (permalink / raw)
To: Joe Perches
Cc: devel, Manish Chopra, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
Greg Kroah-Hartman, open list:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
@ 2020-06-26 23:57 ` Coiby Xu
0 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-26 23:57 UTC (permalink / raw)
To: Joe Perches
Cc: devel, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER, Manish Chopra,
Greg Kroah-Hartman, open list,
open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
2020-06-26 23:57 ` Coiby Xu
@ 2020-06-27 0:06 ` Joe Perches
-1 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2020-06-27 0:06 UTC (permalink / raw)
To: Coiby Xu
Cc: devel, Manish Chopra, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
Greg Kroah-Hartman, open list:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list
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>.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
@ 2020-06-27 0:06 ` Joe Perches
0 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2020-06-27 0:06 UTC (permalink / raw)
To: Coiby Xu
Cc: devel, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER, Manish Chopra,
Greg Kroah-Hartman, open list,
open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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>.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
2020-06-27 0:06 ` Joe Perches
@ 2020-06-27 10:21 ` Coiby Xu
-1 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-27 10:21 UTC (permalink / raw)
To: Joe Perches
Cc: devel, Manish Chopra, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER,
Greg Kroah-Hartman, open list:QLOGIC QLGE 10Gb ETHERNET DRIVER,
open list
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break
@ 2020-06-27 10:21 ` Coiby Xu
0 siblings, 0 replies; 19+ messages in thread
From: Coiby Xu @ 2020-06-27 10:21 UTC (permalink / raw)
To: Joe Perches
Cc: devel, supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER, Manish Chopra,
Greg Kroah-Hartman, open list,
open list:QLOGIC QLGE 10Gb ETHERNET DRIVER
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-06-27 10:21 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25 21:57 [PATCH 0/2] staging: qlge: coding style fix for the qlge driver Coiby Xu
2020-06-25 21:57 ` [PATCH 1/2] staging: qlge: fix trailing */ in block comment Coiby Xu
2020-06-25 21:57 ` Coiby Xu
2020-06-25 21:57 ` [PATCH 2/2] staging: qlge: fix else after return or break Coiby Xu
2020-06-25 21:57 ` Coiby Xu
2020-06-25 22:13 ` Joe Perches
2020-06-25 22:13 ` Joe Perches
2020-06-26 11:47 ` Coiby Xu
2020-06-26 11:47 ` Coiby Xu
2020-06-26 23:57 ` Coiby Xu
2020-06-26 23:57 ` Coiby Xu
2020-06-27 0:06 ` Joe Perches
2020-06-27 0:06 ` Joe Perches
2020-06-27 10:21 ` Coiby Xu
2020-06-27 10:21 ` Coiby Xu
2020-06-26 8:34 ` Dan Carpenter
2020-06-26 8:34 ` Dan Carpenter
2020-06-26 11:41 ` Coiby Xu
2020-06-26 11:41 ` Coiby Xu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.