* [PATCH 1/2] staging: qlge: fix trailing */ in block comment [not found] <20200625215755.70329-1-coiby.xu@gmail.com> @ 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 1 sibling, 0 replies; 9+ 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] 9+ messages in thread
* [PATCH 2/2] staging: qlge: fix else after return or break [not found] <20200625215755.70329-1-coiby.xu@gmail.com> 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 22:13 ` Joe Perches 2020-06-26 8:34 ` Dan Carpenter 1 sibling, 2 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break 2020-06-25 21:57 ` [PATCH 2/2] staging: qlge: fix else after return or break Coiby Xu @ 2020-06-25 22:13 ` Joe Perches 2020-06-26 11:47 ` Coiby Xu 2020-06-26 23:57 ` Coiby Xu 2020-06-26 8:34 ` Dan Carpenter 1 sibling, 2 replies; 9+ 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] 9+ 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 2020-06-26 23:57 ` Coiby Xu 1 sibling, 0 replies; 9+ 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] 9+ 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 @ 2020-06-26 23:57 ` Coiby Xu 2020-06-27 0:06 ` Joe Perches 1 sibling, 1 reply; 9+ 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] 9+ 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 2020-06-27 10:21 ` Coiby Xu 0 siblings, 1 reply; 9+ 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] 9+ 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 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH 2/2] staging: qlge: fix else after return or break 2020-06-25 21:57 ` [PATCH 2/2] staging: qlge: fix else after return or break Coiby Xu 2020-06-25 22:13 ` Joe Perches @ 2020-06-26 8:34 ` Dan Carpenter 2020-06-26 11:41 ` Coiby Xu 1 sibling, 1 reply; 9+ 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] 9+ 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 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
end of thread, other threads:[~2020-06-27 10:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200625215755.70329-1-coiby.xu@gmail.com>
2020-06-25 21:57 ` [PATCH 1/2] staging: qlge: fix trailing */ in block comment Coiby Xu
2020-06-25 21:57 ` [PATCH 2/2] staging: qlge: fix else after return or break Coiby Xu
2020-06-25 22:13 ` Joe Perches
2020-06-26 11:47 ` Coiby Xu
2020-06-26 23:57 ` Coiby Xu
2020-06-27 0:06 ` Joe Perches
2020-06-27 10:21 ` Coiby Xu
2020-06-26 8:34 ` Dan Carpenter
2020-06-26 11:41 ` Coiby Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).