In this case req will never be NULL, so remove unnecessary check.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for bcm43235/6/8 chipsets")
Signed-off-by: Daniil Dulov <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 9fb68c2dc7e3..38e4e4f32a39 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -455,8 +455,7 @@ brcmf_usbdev_qinit(struct list_head *q, int qsize)
brcmf_err("fail!\n");
while (!list_empty(q)) {
req = list_entry(q->next, struct brcmf_usbreq, list);
- if (req)
- usb_free_urb(req->urb);
+ usb_free_urb(req->urb);
list_del(q->next);
}
kfree(reqs);
--
2.25.1
On 2/11/2024 4:05 PM, Daniil Dulov wrote:
> In this case req will never be NULL, so remove unnecessary check.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
Looks good to me, but when do we call things a "fix" and when is
"improvement" more appropriate.
> Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for bcm43235/6/8 chipsets")
Reviewed-by: Arend van Spriel <[email protected]>
> Signed-off-by: Daniil Dulov <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> index 9fb68c2dc7e3..38e4e4f32a39 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> @@ -455,8 +455,7 @@ brcmf_usbdev_qinit(struct list_head *q, int qsize)
> brcmf_err("fail!\n");
> while (!list_empty(q)) {
> req = list_entry(q->next, struct brcmf_usbreq, list);
> - if (req)
> - usb_free_urb(req->urb);
> + usb_free_urb(req->urb);
> list_del(q->next);
> }
Ay you are already touching this code you could consider using
list_for_each_entry_safe().
> kfree(reqs);
On 2/11/2024 8:25 PM, Arend van Spriel wrote:
> On 2/11/2024 4:05 PM, Daniil Dulov wrote:
>> In this case req will never be NULL, so remove unnecessary check.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Looks good to me, but when do we call things a "fix" and when is
> "improvement" more appropriate.
>
>> Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for
>> bcm43235/6/8 chipsets")
> Reviewed-by: Arend van Spriel <[email protected]>
>> Signed-off-by: Daniil Dulov <[email protected]>
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> index 9fb68c2dc7e3..38e4e4f32a39 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> @@ -455,8 +455,7 @@ brcmf_usbdev_qinit(struct list_head *q, int qsize)
>> brcmf_err("fail!\n");
>> while (!list_empty(q)) {
>> req = list_entry(q->next, struct brcmf_usbreq, list);
>> - if (req)
>> - usb_free_urb(req->urb);
>> + usb_free_urb(req->urb);
>> list_del(q->next);
>> }
>
> Ay you are already touching this code you could consider using
> list_for_each_entry_safe().
That "Ay you are ..." should be "As you are ...".
>> kfree(reqs);
Arend van Spriel <[email protected]> writes:
> On 2/11/2024 4:05 PM, Daniil Dulov wrote:
>> In this case req will never be NULL, so remove unnecessary check.
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Looks good to me, but when do we call things a "fix" and when is
> "improvement" more appropriate.
>
>> Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for bcm43235/6/8 chipsets")
> Reviewed-by: Arend van Spriel <[email protected]>
Yeah, this is not a fix and the Fixes tag should be removed.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Hello!
Thank you for the review! I'll correct the patch and send the 2nd version today.
Daniil Dulov
-----Original Message-----
From: Arend van Spriel [mailto:[email protected]]
Sent: Sunday, February 11, 2024 10:27 PM
To: Daniil Dulov <[email protected]>; Kalle Valo <[email protected]>
Cc: David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Pieter-Paul Giesberts <[email protected]>; Franky (Zhenhui) Lin <[email protected]>; John W. Linville <[email protected]>; Kan Yan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] brcmfmac: Remove unnecessary NULL-check.
On 2/11/2024 8:25 PM, Arend van Spriel wrote:
> On 2/11/2024 4:05 PM, Daniil Dulov wrote:
>> In this case req will never be NULL, so remove unnecessary check.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Looks good to me, but when do we call things a "fix" and when is
> "improvement" more appropriate.
>
>> Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for
>> bcm43235/6/8 chipsets")
> Reviewed-by: Arend van Spriel <[email protected]>
>> Signed-off-by: Daniil Dulov <[email protected]>
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> index 9fb68c2dc7e3..38e4e4f32a39 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> @@ -455,8 +455,7 @@ brcmf_usbdev_qinit(struct list_head *q, int qsize)
>> brcmf_err("fail!\n");
>> while (!list_empty(q)) {
>> req = list_entry(q->next, struct brcmf_usbreq, list);
>> - if (req)
>> - usb_free_urb(req->urb);
>> + usb_free_urb(req->urb);
>> list_del(q->next);
>> }
>
> Ay you are already touching this code you could consider using
> list_for_each_entry_safe().
That "Ay you are ..." should be "As you are ...".
>> kfree(reqs);