| Message ID | 1531953919-20804-6-git-send-email-ajay.kathat@microchip.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On Thu, Jul 19, 2018 at 04:15:01AM +0530, Ajay Singh wrote: > diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c > index 85af365..8e71c28 100644 > --- a/drivers/staging/wilc1000/wilc_wlan.c > +++ b/drivers/staging/wilc1000/wilc_wlan.c > @@ -850,13 +850,13 @@ static void wilc_wlan_handle_isr_ext(struct wilc *wilc, u32 int_status) > if (wilc->rx_buffer) > buffer = &wilc->rx_buffer[offset]; > else > - goto _end_; > + goto end; This isn't related to your patch but this goto doesn't appear to make any sort of sense. I have no idea what was intended. > > wilc->hif_func->hif_clear_int_ext(wilc, > DATA_INT_CLR | ENABLE_RX_VMM); > ret = wilc->hif_func->hif_block_rx_ext(wilc, 0, buffer, size); > > -_end_: > +end: > if (ret) { > offset += size; > wilc->rx_buffer_offset = offset; regards, dan carpenter
Hi Dan, On Thu, 19 Jul 2018 12:27:44 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Thu, Jul 19, 2018 at 04:15:01AM +0530, Ajay Singh wrote: > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c > > b/drivers/staging/wilc1000/wilc_wlan.c index 85af365..8e71c28 100644 > > --- a/drivers/staging/wilc1000/wilc_wlan.c > > +++ b/drivers/staging/wilc1000/wilc_wlan.c > > @@ -850,13 +850,13 @@ static void wilc_wlan_handle_isr_ext(struct > > wilc *wilc, u32 int_status) if (wilc->rx_buffer) > > buffer = &wilc->rx_buffer[offset]; > > else > > - goto _end_; > > + goto end; > > This isn't related to your patch but this goto doesn't appear to make > any sort of sense. I have no idea what was intended. > Thanks for pointing it out. I will include these changes in separate patchset. Yes, the position of goto label can be moved just before wilc_wlan_handle_rxq(wilc), as 'ret' will always be '0' when goto statement is executed. Actually earlier there were few more goto statement in this function and single label 'end' was used to handle for different cases. But in previous cleanup patches those cases were removed. Now this function can be further refactor by either moving goto label before wilc_wlan_handle_rxq(wilc) or avoid goto use by adding the rx_buffer validation along with size check. i.e end: wilc_wlan_handle_rxq(wilc) OR if (size > 0 && wilc->rx_buffer) { .... } wilc_wlan_handle_rxq(wilc) > > > > wilc->hif_func->hif_clear_int_ext(wilc, > > DATA_INT_CLR | > > ENABLE_RX_VMM); ret = wilc->hif_func->hif_block_rx_ext(wilc, 0, > > buffer, size); > > -_end_: > > +end: > > if (ret) { > > offset += size; > > wilc->rx_buffer_offset = offset; > > regards, > dan carpenter > > _______________________________________________ > devel mailing list > devel@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Fri, Jul 20, 2018 at 04:35:24AM +0530, Ajay Singh wrote: > Hi Dan, > > On Thu, 19 Jul 2018 12:27:44 +0300 > Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > On Thu, Jul 19, 2018 at 04:15:01AM +0530, Ajay Singh wrote: > > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c > > > b/drivers/staging/wilc1000/wilc_wlan.c index 85af365..8e71c28 100644 > > > --- a/drivers/staging/wilc1000/wilc_wlan.c > > > +++ b/drivers/staging/wilc1000/wilc_wlan.c > > > @@ -850,13 +850,13 @@ static void wilc_wlan_handle_isr_ext(struct > > > wilc *wilc, u32 int_status) if (wilc->rx_buffer) > > > buffer = &wilc->rx_buffer[offset]; > > > else > > > - goto _end_; > > > + goto end; > > > > This isn't related to your patch but this goto doesn't appear to make > > any sort of sense. I have no idea what was intended. > > > > Thanks for pointing it out. I will include these changes in separate > patchset. > > Yes, the position of goto label can be moved just before > wilc_wlan_handle_rxq(wilc), as 'ret' will always be '0' when goto > statement is executed. > > Actually earlier there were few more goto statement in this function > and single label 'end' was used to handle for different cases. But in > previous cleanup patches those cases were removed. > Now this function can be further refactor by either moving > goto label before wilc_wlan_handle_rxq(wilc) or avoid goto use by > adding the rx_buffer validation along with size check. > > i.e > > end: > wilc_wlan_handle_rxq(wilc) > > > OR > > if (size > 0 && wilc->rx_buffer) { > > .... > } > wilc_wlan_handle_rxq(wilc) > Actually looking at it now, you could probably just remove the if statement. Hopefully wilc->rx_buffer is non-NULL at this point? Is there really any need to call wilc_wlan_handle_rxq() when we haven't called wilc_wlan_rxq_add()? regards, dan carpenter
Hi Dan, On Mon, 30 Jul 2018 11:41:13 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Fri, Jul 20, 2018 at 04:35:24AM +0530, Ajay Singh wrote: > > Hi Dan, > > > > On Thu, 19 Jul 2018 12:27:44 +0300 > > Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > > On Thu, Jul 19, 2018 at 04:15:01AM +0530, Ajay Singh wrote: > > > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c > > > > b/drivers/staging/wilc1000/wilc_wlan.c index 85af365..8e71c28 > > > > 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c > > > > +++ b/drivers/staging/wilc1000/wilc_wlan.c > > > > @@ -850,13 +850,13 @@ static void > > > > wilc_wlan_handle_isr_ext(struct wilc *wilc, u32 int_status) if > > > > (wilc->rx_buffer) buffer = &wilc->rx_buffer[offset]; > > > > else > > > > - goto _end_; > > > > + goto end; > > > > > > This isn't related to your patch but this goto doesn't appear to > > > make any sort of sense. I have no idea what was intended. > > > > > > > Thanks for pointing it out. I will include these changes in separate > > patchset. > > > > Yes, the position of goto label can be moved just before > > wilc_wlan_handle_rxq(wilc), as 'ret' will always be '0' when goto > > statement is executed. > > > > Actually earlier there were few more goto statement in this function > > and single label 'end' was used to handle for different cases. But > > in previous cleanup patches those cases were removed. > > Now this function can be further refactor by either moving > > goto label before wilc_wlan_handle_rxq(wilc) or avoid goto use by > > adding the rx_buffer validation along with size check. > > > > i.e > > > > end: > > wilc_wlan_handle_rxq(wilc) > > > > > > OR > > > > if (size > 0 && wilc->rx_buffer) { > > > > .... > > } > > wilc_wlan_handle_rxq(wilc) > > > > Actually looking at it now, you could probably just remove the if > statement. Hopefully wilc->rx_buffer is non-NULL at this point? Is > there really any need to call wilc_wlan_handle_rxq() when we haven't > called wilc_wlan_rxq_add()? > Yes, wilc->rx_buffer would be non NULL value as its only one time allocated buffer. wilc_wlan_handle_rxq() was called without wilc_wlan_rxq_add() just as a fail safe to ensure there are no pending packets in the queue. Regards, Ajay > regards, > dan carpenter > >
On Mon, Jul 30, 2018 at 03:40:24PM +0530, Ajay Singh wrote: > Hi Dan, > > On Mon, 30 Jul 2018 11:41:13 +0300 > Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > On Fri, Jul 20, 2018 at 04:35:24AM +0530, Ajay Singh wrote: > > > Hi Dan, > > > > > > On Thu, 19 Jul 2018 12:27:44 +0300 > > > Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > > > > On Thu, Jul 19, 2018 at 04:15:01AM +0530, Ajay Singh wrote: > > > > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c > > > > > b/drivers/staging/wilc1000/wilc_wlan.c index 85af365..8e71c28 > > > > > 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c > > > > > +++ b/drivers/staging/wilc1000/wilc_wlan.c > > > > > @@ -850,13 +850,13 @@ static void > > > > > wilc_wlan_handle_isr_ext(struct wilc *wilc, u32 int_status) if > > > > > (wilc->rx_buffer) buffer = &wilc->rx_buffer[offset]; > > > > > else > > > > > - goto _end_; > > > > > + goto end; > > > > > > > > This isn't related to your patch but this goto doesn't appear to > > > > make any sort of sense. I have no idea what was intended. > > > > > > > > > > Thanks for pointing it out. I will include these changes in separate > > > patchset. > > > > > > Yes, the position of goto label can be moved just before > > > wilc_wlan_handle_rxq(wilc), as 'ret' will always be '0' when goto > > > statement is executed. > > > > > > Actually earlier there were few more goto statement in this function > > > and single label 'end' was used to handle for different cases. But > > > in previous cleanup patches those cases were removed. > > > Now this function can be further refactor by either moving > > > goto label before wilc_wlan_handle_rxq(wilc) or avoid goto use by > > > adding the rx_buffer validation along with size check. > > > > > > i.e > > > > > > end: > > > wilc_wlan_handle_rxq(wilc) > > > > > > > > > OR > > > > > > if (size > 0 && wilc->rx_buffer) { > > > > > > .... > > > } > > > wilc_wlan_handle_rxq(wilc) > > > > > > > Actually looking at it now, you could probably just remove the if > > statement. Hopefully wilc->rx_buffer is non-NULL at this point? Is > > there really any need to call wilc_wlan_handle_rxq() when we haven't > > called wilc_wlan_rxq_add()? > > > > Yes, wilc->rx_buffer would be non NULL value as its only one time > allocated buffer. wilc_wlan_handle_rxq() was called without > wilc_wlan_rxq_add() just as a fail safe to ensure there are no pending > packets in the queue. The only thing is wilc->quit can be set. Otherwise if ->rx_buffer is NULL it would just result in a NULL dereference. (We are deep into hypotheticals here because we're discussing impossible code). regards, dan carpenter
Hi Dan, On Mon, 30 Jul 2018 14:32:31 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Mon, Jul 30, 2018 at 03:40:24PM +0530, Ajay Singh wrote: > > Hi Dan, > > > > On Mon, 30 Jul 2018 11:41:13 +0300 > > Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > > On Fri, Jul 20, 2018 at 04:35:24AM +0530, Ajay Singh wrote: > > > > Hi Dan, > > > > > > > > On Thu, 19 Jul 2018 12:27:44 +0300 > > > > Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > > > > > > On Thu, Jul 19, 2018 at 04:15:01AM +0530, Ajay Singh > > > > > wrote: > > > > > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c > > > > > > b/drivers/staging/wilc1000/wilc_wlan.c index > > > > > > 85af365..8e71c28 100644 --- > > > > > > a/drivers/staging/wilc1000/wilc_wlan.c +++ > > > > > > b/drivers/staging/wilc1000/wilc_wlan.c @@ -850,13 +850,13 > > > > > > @@ static void wilc_wlan_handle_isr_ext(struct wilc *wilc, > > > > > > u32 int_status) if (wilc->rx_buffer) buffer = > > > > > > &wilc->rx_buffer[offset]; else > > > > > > - goto _end_; > > > > > > + goto end; > > > > > > > > > > This isn't related to your patch but this goto doesn't appear > > > > > to make any sort of sense. I have no idea what was intended. > > > > > > > > > > > > > Thanks for pointing it out. I will include these changes in > > > > separate patchset. > > > > > > > > Yes, the position of goto label can be moved just before > > > > wilc_wlan_handle_rxq(wilc), as 'ret' will always be '0' when > > > > goto statement is executed. > > > > > > > > Actually earlier there were few more goto statement in this > > > > function and single label 'end' was used to handle for > > > > different cases. But in previous cleanup patches those cases > > > > were removed. Now this function can be further refactor by > > > > either moving goto label before wilc_wlan_handle_rxq(wilc) or > > > > avoid goto use by adding the rx_buffer validation along with > > > > size check. > > > > > > > > i.e > > > > > > > > end: > > > > wilc_wlan_handle_rxq(wilc) > > > > > > > > > > > > OR > > > > > > > > if (size > 0 && wilc->rx_buffer) { > > > > > > > > .... > > > > } > > > > wilc_wlan_handle_rxq(wilc) > > > > > > > > > > Actually looking at it now, you could probably just remove the if > > > statement. Hopefully wilc->rx_buffer is non-NULL at this point? > > > Is there really any need to call wilc_wlan_handle_rxq() when we > > > haven't called wilc_wlan_rxq_add()? > > > > > > > Yes, wilc->rx_buffer would be non NULL value as its only one time > > allocated buffer. wilc_wlan_handle_rxq() was called without > > wilc_wlan_rxq_add() just as a fail safe to ensure there are no > > pending packets in the queue. > > The only thing is wilc->quit can be set. Otherwise if ->rx_buffer is > NULL it would just result in a NULL dereference. (We are deep into > hypotheticals here because we're discussing impossible code). > I have prepared a patch based on the feedback. The patch is submitted in https://patchwork.kernel.org/patch/10551703/. Regards, Ajay > > _______________________________________________ > devel mailing list > devel@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Fantastic. Thanks! regards, dan carpenter
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index f90b9b6..73606c3 100644 --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -2196,11 +2196,11 @@ static struct wireless_dev *wilc_wfi_cfg_alloc(void) wdev = kzalloc(sizeof(*wdev), GFP_KERNEL); if (!wdev) - goto _fail_; + goto out; wdev->wiphy = wiphy_new(&wilc_cfg80211_ops, sizeof(struct wilc_priv)); if (!wdev->wiphy) - goto _fail_mem_; + goto free_mem; wilc_band_2ghz.ht_cap.ht_supported = 1; wilc_band_2ghz.ht_cap.cap |= (1 << IEEE80211_HT_CAP_RX_STBC_SHIFT); @@ -2212,9 +2212,9 @@ static struct wireless_dev *wilc_wfi_cfg_alloc(void) return wdev; -_fail_mem_: +free_mem: kfree(wdev); -_fail_: +out: return NULL; } diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c index 85af365..8e71c28 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c +++ b/drivers/staging/wilc1000/wilc_wlan.c @@ -850,13 +850,13 @@ static void wilc_wlan_handle_isr_ext(struct wilc *wilc, u32 int_status) if (wilc->rx_buffer) buffer = &wilc->rx_buffer[offset]; else - goto _end_; + goto end; wilc->hif_func->hif_clear_int_ext(wilc, DATA_INT_CLR | ENABLE_RX_VMM); ret = wilc->hif_func->hif_block_rx_ext(wilc, 0, buffer, size); -_end_: +end: if (ret) { offset += size; wilc->rx_buffer_offset = offset;
Cleanup patch to avoid use of leading '_' in goto label name. Also used proper string for lable names. Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> --- drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 8 ++++---- drivers/staging/wilc1000/wilc_wlan.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-)