[PATCH] brcm80211: fmac: Add error check for brcmf_usb_dlneeded()

[PATCH] brcm80211: fmac: Add error check for brcmf_usb_dlneeded()

[Wentao Liang]

The function brcmf_usb_dlneeded() calls the function brcmf_usb_dl_cmd()
but dose not check its return value. The 'id.chiprev' is uninitialized if
the function brcmf_usb_dl_cmd() fails, and may propagate to
'devinfo->bus_pub.chiprev'.

Add error handling for brcmf_usb_dl_cmd() to return the function if the
'id.chiprev' is uninitialized.

Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for bcm43235/6/8 chipsets")
Cc: stable@vger.kernel.org # v3.4+
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 2821c27f317e..50dddac8a2ab 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -790,6 +790,7 @@ brcmf_usb_dlneeded(struct brcmf_usbdev_info *devinfo)
 {
 	struct bootrom_id_le id;
 	u32 chipid, chiprev;
+	int err;
 
 	brcmf_dbg(USB, "Enter\n");
 
@@ -798,7 +799,11 @@ brcmf_usb_dlneeded(struct brcmf_usbdev_info *devinfo)
 
 	/* Check if firmware downloaded already by querying runtime ID */
 	id.chip = cpu_to_le32(0xDEAD);
-	brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
+	err = brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
+	if (err) {
+		brcmf_err("DL_GETID Failed\n");
+		return false;
+	}
 
 	chipid = le32_to_cpu(id.chip);
 	chiprev = le32_to_cpu(id.chiprev);
-- 
2.42.0.windows.2

Re: [PATCH] brcm80211: fmac: Add error check for brcmf_usb_dlneeded()

[Markus Elfring]

…
> Add error handling for brcmf_usb_dl_cmd() to return the function if the
> 'id.chiprev' is uninitialized.

* Please reconsider the offered conclusion once more.

* I propose to replace the word “for” by “in” (before function names)
  in some summary phrases.


…
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
…
> @@ -798,7 +799,11 @@ brcmf_usb_dlneeded(struct brcmf_usbdev_info *devinfo)
>
>  	/* Check if firmware downloaded already by querying runtime ID */
>  	id.chip = cpu_to_le32(0xDEAD);
> -	brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
> +	err = brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
> +	if (err) {
> +		brcmf_err("DL_GETID Failed\n");
> +		return false;
> +	}
…

Would an error hint like “DL_GETVER failed\n” be more appropriate here?

Regards,
Markus

Re: [PATCH] brcm80211: fmac: Add error check for brcmf_usb_dlneeded()

[Arend van Spriel]

On 4/6/2025 10:19 AM, Wentao Liang wrote:
> The function brcmf_usb_dlneeded() calls the function brcmf_usb_dl_cmd()
> but dose not check its return value. The 'id.chiprev' is uninitialized if
> the function brcmf_usb_dl_cmd() fails, and may propagate to
> 'devinfo->bus_pub.chiprev'.
> 
> Add error handling for brcmf_usb_dl_cmd() to return the function if the
> 'id.chiprev' is uninitialized.

Thanks for the patch, but NAK. Let me explain why below...

> Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for bcm43235/6/8 chipsets")
> Cc: stable@vger.kernel.org # v3.4+
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> index 2821c27f317e..50dddac8a2ab 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> @@ -790,6 +790,7 @@ brcmf_usb_dlneeded(struct brcmf_usbdev_info *devinfo)
>   {
>   	struct bootrom_id_le id;
>   	u32 chipid, chiprev;
> +	int err;
>   
>   	brcmf_dbg(USB, "Enter\n");
>   
> @@ -798,7 +799,11 @@ brcmf_usb_dlneeded(struct brcmf_usbdev_info *devinfo)
>   
>   	/* Check if firmware downloaded already by querying runtime ID */
>   	id.chip = cpu_to_le32(0xDEAD);
> -	brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
> +	err = brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
> +	if (err) {
> +		brcmf_err("DL_GETID Failed\n");
> +		return false;

The boolean return value does not indicate pass or fail. It answers the 
question implied by the function name brcmf_usb_dlneeded(), ie. is the 
USB device running firmware (false) or do we need to download firmware 
(true). So returning false here is not going to help us.

The id.chip is initialized to 0xDEAD so upon a failure that value is 
being passed to brcmf_usb_prepare_fw_request() which will consequently 
return NULL, because we do not support a 0xDEAD chip. So there is no 
need to bail out here. Just print the failure message is enough although 
I would suggest to include the err value:

-	brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
+	err = brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
+	if (err)
+		brcmf_err("DL_GETVER failed: err=%d\n", err);

Regards,
Arend

Re: [PATCH] brcm80211: fmac: Add error check for brcmf_usb_dlneeded()

[Arend Van Spriel]

On April 9, 2025 2:11:08 PM Arend van Spriel <arend.vanspriel@broadcom.com> 
wrote:

> On 4/6/2025 10:19 AM, Wentao Liang wrote:
>> The function brcmf_usb_dlneeded() calls the function brcmf_usb_dl_cmd()
>> but dose not check its return value. The 'id.chiprev' is uninitialized if
>> the function brcmf_usb_dl_cmd() fails, and may propagate to
>> 'devinfo->bus_pub.chiprev'.
>>
>> Add error handling for brcmf_usb_dl_cmd() to return the function if the
>> 'id.chiprev' is uninitialized.
>
> Thanks for the patch, but NAK. Let me explain why below...
>
>> Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for bcm43235/6/8 
>> chipsets")
>> Cc: stable@vger.kernel.org # v3.4+
>> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> index 2821c27f317e..50dddac8a2ab 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
>> @@ -790,6 +790,7 @@ brcmf_usb_dlneeded(struct brcmf_usbdev_info *devinfo)
>> {
>> struct bootrom_id_le id;
>> u32 chipid, chiprev;
>> + int err;
>>
>> brcmf_dbg(USB, "Enter\n");
>>
>> @@ -798,7 +799,11 @@ brcmf_usb_dlneeded(struct brcmf_usbdev_info *devinfo)
>>
>> /* Check if firmware downloaded already by querying runtime ID */
>> id.chip = cpu_to_le32(0xDEAD);
>> - brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
>> + err = brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
>> + if (err) {
>> + brcmf_err("DL_GETID Failed\n");
>> + return false;
>
> The boolean return value does not indicate pass or fail. It answers the
> question implied by the function name brcmf_usb_dlneeded(), ie. is the
> USB device running firmware (false) or do we need to download firmware
> (true). So returning false here is not going to help us.
>
> The id.chip is initialized to 0xDEAD so upon a failure that value is
> being passed to brcmf_usb_prepare_fw_request() which will consequently
> return NULL, because we do not support a 0xDEAD chip. So there is no
> need to bail out here. Just print the failure message is enough although
> I would suggest to include the err value:
>
> - brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
> + err = brcmf_usb_dl_cmd(devinfo, DL_GETVER, &id, sizeof(id));
> + if (err)
> + brcmf_err("DL_GETVER failed: err=%d\n", err);

Maybe an error message in brcmf_usb_dl_cmd() would suffice printing the 
command id and error value. That way every invocation of the function logs 
a message upon failure.

Regards,
Arend
>