The probe() id argument may be NULL in 2 scenarios:
1. brcmf_pcie_pm_leave_D3() calling brcmf_pcie_probe() to reprobe
the device.
2. If a user tries to manually bind the driver from sysfs then the sdio /
pcie / usb probe() function gets called with NULL as id argument.
1. Is being hit by users causing the following oops on resume and causing
wifi to stop working:
BUG: kernel NULL pointer dereference, address: 0000000000000018
<snip>
Hardware name: Dell Inc. XPS 13 9350/0PWNCR, BIDS 1.13.0 02/10/2020
Workgueue: events_unbound async_run_entry_fn
RIP: 0010:brcmf_pcie_probe+Ox16b/0x7a0 [brcmfmac]
<snip>
Call Trace:
<TASK>
brcmf_pcie_pm_leave_D3+0xc5/8x1a0 [brcmfmac be3b4cefca451e190fa35be8f00db1bbec293887]
? pci_pm_resume+0x5b/0xf0
? pci_legacy_resume+0x80/0x80
dpm_run_callback+0x47/0x150
device_resume+0xa2/0x1f0
async_resume+0x1d/0x30
<snip>
Fix this by checking for id being NULL.
In the case of brcmf_pcie_probe() also add a manual lookup of the id
when it is NULL so that fwvid will still get set correctly on reprobe
on resume.
Fixes: da6d9c8ecd00 ("wifi: brcmfmac: add firmware vendor info in driver info")
Reported-by: Felix <[email protected]>
Link: https://lore.kernel.org/regressions/[email protected]/
Cc: Arend van Spriel <[email protected]>
Cc: [email protected]
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +-
drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 +++++++-
drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 2 +-
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 65d4799a5658..833524bc9b50 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1067,7 +1067,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
sdiodev->bus_if = bus_if;
bus_if->bus_priv.sdio = sdiodev;
bus_if->proto_type = BRCMF_PROTO_BCDC;
- bus_if->fwvid = id->driver_data;
+ bus_if->fwvid = id ? id->driver_data : BRCMF_FWVENDOR_INVALID;
dev_set_drvdata(&func->dev, bus_if);
dev_set_drvdata(&sdiodev->func1->dev, bus_if);
sdiodev->dev = &sdiodev->func1->dev;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index a9b9b2dc62d4..b29ab19d695d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -2339,6 +2339,9 @@ static void brcmf_pcie_debugfs_create(struct device *dev)
}
#endif
+/* Forward declaration for pci_match_id() call */
+static const struct pci_device_id brcmf_pcie_devid_table[];
+
static int
brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
@@ -2351,6 +2354,9 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
brcmf_dbg(PCIE, "Enter %x:%x\n", pdev->vendor, pdev->device);
+ if (!id)
+ id = pci_match_id(brcmf_pcie_devid_table, pdev);
+
ret = -ENOMEM;
devinfo = kzalloc(sizeof(*devinfo), GFP_KERNEL);
if (devinfo == NULL)
@@ -2406,7 +2412,7 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
bus->bus_priv.pcie = pcie_bus_dev;
bus->ops = &brcmf_pcie_bus_ops;
bus->proto_type = BRCMF_PROTO_MSGBUF;
- bus->fwvid = id->driver_data;
+ bus->fwvid = id ? id->driver_data : BRCMF_FWVENDOR_INVALID;
bus->chip = devinfo->coreid;
bus->wowl_supported = pci_pme_capable(pdev, PCI_D3hot);
dev_set_drvdata(&pdev->dev, bus);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 246843aeb696..2541a47b1f8f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1425,7 +1425,7 @@ brcmf_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
else
brcmf_dbg(USB, "Broadcom full speed USB WLAN interface detected\n");
- ret = brcmf_usb_probe_cb(devinfo, id->driver_info);
+ ret = brcmf_usb_probe_cb(devinfo, id ? id->driver_info : BRCMF_FWVENDOR_INVALID);
if (ret)
goto fail;
--
2.40.1
On 5/10/2023 12:00 PM, Hans de Goede wrote:
> The probe() id argument may be NULL in 2 scenarios:
>
> 1. brcmf_pcie_pm_leave_D3() calling brcmf_pcie_probe() to reprobe
> the device.
>
> 2. If a user tries to manually bind the driver from sysfs then the sdio /
> pcie / usb probe() function gets called with NULL as id argument.
>
> 1. Is being hit by users causing the following oops on resume and causing
> wifi to stop working:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000018
> <snip>
> Hardware name: Dell Inc. XPS 13 9350/0PWNCR, BIDS 1.13.0 02/10/2020
> Workgueue: events_unbound async_run_entry_fn
> RIP: 0010:brcmf_pcie_probe+Ox16b/0x7a0 [brcmfmac]
> <snip>
> Call Trace:
> <TASK>
> brcmf_pcie_pm_leave_D3+0xc5/8x1a0 [brcmfmac be3b4cefca451e190fa35be8f00db1bbec293887]
> ? pci_pm_resume+0x5b/0xf0
> ? pci_legacy_resume+0x80/0x80
> dpm_run_callback+0x47/0x150
> device_resume+0xa2/0x1f0
> async_resume+0x1d/0x30
> <snip>
>
> Fix this by checking for id being NULL.
>
> In the case of brcmf_pcie_probe() also add a manual lookup of the id
> when it is NULL so that fwvid will still get set correctly on reprobe
> on resume.
Hi Hans,
Thanks for looking into this and coming up with a fix. Does it make
sense to proceed with BRCMF_FWVENDOR_INVALID though. I think it will end
up in probe failure in brcmf_fwvid_attach() because of it so why not
bail out right away.
Regards,
Arend
> Fixes: da6d9c8ecd00 ("wifi: brcmfmac: add firmware vendor info in driver info")
> Reported-by: Felix <[email protected]>
> Link: https://lore.kernel.org/regressions/[email protected]/
> Cc: Arend van Spriel <[email protected]>
> Cc: [email protected]
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +-
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 +++++++-
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 2 +-
> 3 files changed, 9 insertions(+), 3 deletions(-)
Hi Arend,
On 5/10/23 14:12, Arend van Spriel wrote:
> On 5/10/2023 12:00 PM, Hans de Goede wrote:
>> The probe() id argument may be NULL in 2 scenarios:
>>
>> 1. brcmf_pcie_pm_leave_D3() calling brcmf_pcie_probe() to reprobe
>> the device.
>>
>> 2. If a user tries to manually bind the driver from sysfs then the sdio /
>> pcie / usb probe() function gets called with NULL as id argument.
>>
>> 1. Is being hit by users causing the following oops on resume and causing
>> wifi to stop working:
>>
>> BUG: kernel NULL pointer dereference, address: 0000000000000018
>> <snip>
>> Hardware name: Dell Inc. XPS 13 9350/0PWNCR, BIDS 1.13.0 02/10/2020
>> Workgueue: events_unbound async_run_entry_fn
>> RIP: 0010:brcmf_pcie_probe+Ox16b/0x7a0 [brcmfmac]
>> <snip>
>> Call Trace:
>> <TASK>
>> brcmf_pcie_pm_leave_D3+0xc5/8x1a0 [brcmfmac be3b4cefca451e190fa35be8f00db1bbec293887]
>> ? pci_pm_resume+0x5b/0xf0
>> ? pci_legacy_resume+0x80/0x80
>> dpm_run_callback+0x47/0x150
>> device_resume+0xa2/0x1f0
>> async_resume+0x1d/0x30
>> <snip>
>>
>> Fix this by checking for id being NULL.
>>
>> In the case of brcmf_pcie_probe() also add a manual lookup of the id
>> when it is NULL so that fwvid will still get set correctly on reprobe
>> on resume.
>
> Hi Hans,
>
> Thanks for looking into this and coming up with a fix. Does it make sense to proceed with
> BRCMF_FWVENDOR_INVALID though. I think it will end up in probe failure in brcmf_fwvid_attach()
> because of it so why not bail out right away.
You are right since the vendor stuff is in separate modules I thought
the driver would just continue without, but that indeed does not work.
I have just prepared a version 2 which immediately bails from probe()
if id is still null after doing a pci_match_id() / usb_match_id()
to try and lookup the id. Note for the SDIO case v2 will immediately
bail on a NULL id since there is no sdio_match_id() helper.
I'll send out v2 right away.
Regards,
Hans
>> Fixes: da6d9c8ecd00 ("wifi: brcmfmac: add firmware vendor info in driver info")
>> Reported-by: Felix <[email protected]>
>> Link: https://lore.kernel.org/regressions/[email protected]/
>> Cc: Arend van Spriel <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +-
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 +++++++-
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 2 +-
>> 3 files changed, 9 insertions(+), 3 deletions(-)