diff mbox series

brcm80211: fmac: Add error check for brcmf_usb_dlneeded()

Message ID 20250406081930.2909-1-vulab@iscas.ac.cn (mailing list archive)
State Rejected
Delegated to: Johannes Berg
Headers show
Series brcm80211: fmac: Add error check for brcmf_usb_dlneeded() | expand

Checks

Context Check Description
wifibot/fixes_present success Fixes tag not required for -next series
wifibot/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
wifibot/tree_selection success Guessed tree name to be wireless-next
wifibot/ynl success Generated files up to date; no warnings/errors; no diff in generated;
wifibot/build_32bit success Errors and warnings before: 0 this patch: 0
wifibot/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
wifibot/build_clang success Errors and warnings before: 0 this patch: 0
wifibot/build_clang_rust success No Rust files in patch. Skipping build
wifibot/build_tools success No tools touched, skip
wifibot/check_selftest success No net selftest shell script
wifibot/checkpatch success total: 0 errors, 0 warnings, 0 checks, 19 lines checked
wifibot/deprecated_api success None detected
wifibot/header_inline success No static functions without inline keyword in header files
wifibot/kdoc success Errors and warnings before: 0 this patch: 0
wifibot/source_inline success Was 0 now: 0
wifibot/verify_fixes success Fixes tag looks correct
wifibot/verify_signedoff success Signed-off-by tag matches author and committer

Commit Message

Wentao Liang April 6, 2025, 8:19 a.m. UTC
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(-)

Comments

Markus Elfring April 9, 2025, 8:16 a.m. UTC | #1
> 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
Arend Van Spriel April 9, 2025, 12:11 p.m. UTC | #2
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
Arend Van Spriel April 9, 2025, 3:03 p.m. UTC | #3
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
>
diff mbox series

Patch

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);