| Message ID | 20250406081930.2909-1-vulab@iscas.ac.cn |
|---|---|
| State | New |
| Headers | show |
| Series | brcm80211: fmac: Add error check for brcmf_usb_dlneeded() | expand |
… > 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
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
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 --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);
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(-)