2024-03-19 10:48:49

by Breno Leitao

[permalink] [raw]
Subject: [PATCH] ath10k: allocate dummy net_device dynamically

Embedding net_device into structures prohibits the usage of flexible
arrays in the net_device structure. For more details, see the discussion
at [1].

Un-embed the net_device from struct ath10k by converting it
into a pointer. Then use the leverage alloc_netdev() to allocate the
net_device object at ath10k_core_create(). The free of the device occurs
at ath10k_core_destroy().

[1] https://lore.kernel.org/all/[email protected]/

Signed-off-by: Breno Leitao <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 10 ++++++++--
drivers/net/wireless/ath/ath10k/core.h | 2 +-
drivers/net/wireless/ath/ath10k/pci.c | 2 +-
drivers/net/wireless/ath/ath10k/sdio.c | 2 +-
drivers/net/wireless/ath/ath10k/snoc.c | 4 ++--
drivers/net/wireless/ath/ath10k/usb.c | 2 +-
6 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 9ce6f49ab261..3736517002f6 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -3673,11 +3673,14 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev,
INIT_WORK(&ar->set_coverage_class_work,
ath10k_core_set_coverage_class_work);

- init_dummy_netdev(&ar->napi_dev);
+ ar->napi_dev = alloc_netdev(0, "dummy", NET_NAME_UNKNOWN,
+ init_dummy_netdev);
+ if (!ar->napi_dev)
+ goto err_free_tx_complete;

ret = ath10k_coredump_create(ar);
if (ret)
- goto err_free_tx_complete;
+ goto err_free_netdev;

ret = ath10k_debug_create(ar);
if (ret)
@@ -3687,6 +3690,8 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev,

err_free_coredump:
ath10k_coredump_destroy(ar);
+err_free_netdev:
+ free_netdev(ar->napi_dev);
err_free_tx_complete:
destroy_workqueue(ar->workqueue_tx_complete);
err_free_aux_wq:
@@ -3708,6 +3713,7 @@ void ath10k_core_destroy(struct ath10k *ar)

destroy_workqueue(ar->workqueue_tx_complete);

+ free_netdev(ar->napi_dev);
ath10k_debug_destroy(ar);
ath10k_coredump_destroy(ar);
ath10k_htt_tx_destroy(&ar->htt);
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index c110d15528bd..26003b519574 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -1269,7 +1269,7 @@ struct ath10k {
struct ath10k_per_peer_tx_stats peer_tx_stats;

/* NAPI */
- struct net_device napi_dev;
+ struct net_device *napi_dev;
struct napi_struct napi;

struct work_struct set_coverage_class_work;
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 5c34b156b4ff..558bec96ae40 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -3217,7 +3217,7 @@ static void ath10k_pci_free_irq(struct ath10k *ar)

void ath10k_pci_init_napi(struct ath10k *ar)
{
- netif_napi_add(&ar->napi_dev, &ar->napi, ath10k_pci_napi_poll);
+ netif_napi_add(ar->napi_dev, &ar->napi, ath10k_pci_napi_poll);
}

static int ath10k_pci_init_irq(struct ath10k *ar)
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 0ab5433f6cf6..e28f2fe1101b 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -2532,7 +2532,7 @@ static int ath10k_sdio_probe(struct sdio_func *func,
return -ENOMEM;
}

- netif_napi_add(&ar->napi_dev, &ar->napi, ath10k_sdio_napi_poll);
+ netif_napi_add(ar->napi_dev, &ar->napi, ath10k_sdio_napi_poll);

ath10k_dbg(ar, ATH10K_DBG_BOOT,
"sdio new func %d vendor 0x%x device 0x%x block 0x%x/0x%x\n",
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 2c39bad7ebfb..0449b9ffc32d 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -935,7 +935,7 @@ static int ath10k_snoc_hif_start(struct ath10k *ar)

bitmap_clear(ar_snoc->pending_ce_irqs, 0, CE_COUNT_MAX);

- dev_set_threaded(&ar->napi_dev, true);
+ dev_set_threaded(ar->napi_dev, true);
ath10k_core_napi_enable(ar);
ath10k_snoc_irq_enable(ar);
ath10k_snoc_rx_post(ar);
@@ -1253,7 +1253,7 @@ static int ath10k_snoc_napi_poll(struct napi_struct *ctx, int budget)

static void ath10k_snoc_init_napi(struct ath10k *ar)
{
- netif_napi_add(&ar->napi_dev, &ar->napi, ath10k_snoc_napi_poll);
+ netif_napi_add(ar->napi_dev, &ar->napi, ath10k_snoc_napi_poll);
}

static int ath10k_snoc_request_irq(struct ath10k *ar)
diff --git a/drivers/net/wireless/ath/ath10k/usb.c b/drivers/net/wireless/ath/ath10k/usb.c
index 3c482baacec1..3b51b7f52130 100644
--- a/drivers/net/wireless/ath/ath10k/usb.c
+++ b/drivers/net/wireless/ath/ath10k/usb.c
@@ -1014,7 +1014,7 @@ static int ath10k_usb_probe(struct usb_interface *interface,
return -ENOMEM;
}

- netif_napi_add(&ar->napi_dev, &ar->napi, ath10k_usb_napi_poll);
+ netif_napi_add(ar->napi_dev, &ar->napi, ath10k_usb_napi_poll);

usb_get_dev(dev);
vendor_id = le16_to_cpu(dev->descriptor.idVendor);
--
2.43.0



2024-03-19 16:05:56

by Jeff Johnson

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

On 3/19/2024 3:47 AM, Breno Leitao wrote:
> Embedding net_device into structures prohibits the usage of flexible
> arrays in the net_device structure. For more details, see the discussion
> at [1].
>
> Un-embed the net_device from struct ath10k by converting it
> into a pointer. Then use the leverage alloc_netdev() to allocate the
> net_device object at ath10k_core_create(). The free of the device occurs
> at ath10k_core_destroy().
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Breno Leitao <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/core.c | 10 ++++++++--
> drivers/net/wireless/ath/ath10k/core.h | 2 +-
> drivers/net/wireless/ath/ath10k/pci.c | 2 +-
> drivers/net/wireless/ath/ath10k/sdio.c | 2 +-
> drivers/net/wireless/ath/ath10k/snoc.c | 4 ++--
> drivers/net/wireless/ath/ath10k/usb.c | 2 +-
> 6 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index 9ce6f49ab261..3736517002f6 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -3673,11 +3673,14 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev,
> INIT_WORK(&ar->set_coverage_class_work,
> ath10k_core_set_coverage_class_work);
>
> - init_dummy_netdev(&ar->napi_dev);
> + ar->napi_dev = alloc_netdev(0, "dummy", NET_NAME_UNKNOWN,
> + init_dummy_netdev);
> + if (!ar->napi_dev)
> + goto err_free_tx_complete;
>
> ret = ath10k_coredump_create(ar);
> if (ret)
> - goto err_free_tx_complete;
> + goto err_free_netdev;
>
> ret = ath10k_debug_create(ar);
> if (ret)
> @@ -3687,6 +3690,8 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev,
>
> err_free_coredump:
> ath10k_coredump_destroy(ar);
> +err_free_netdev:
> + free_netdev(ar->napi_dev);
> err_free_tx_complete:
> destroy_workqueue(ar->workqueue_tx_complete);
> err_free_aux_wq:
> @@ -3708,6 +3713,7 @@ void ath10k_core_destroy(struct ath10k *ar)
>
> destroy_workqueue(ar->workqueue_tx_complete);
>
> + free_netdev(ar->napi_dev);
> ath10k_debug_destroy(ar);
> ath10k_coredump_destroy(ar);
> ath10k_htt_tx_destroy(&ar->htt);

looks like there is a pre-existing issue that the order of operations in
_destroy() doesn't match the order of operations in the _create() error path.

but the placement of your changes looks ok to me


> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index c110d15528bd..26003b519574 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -1269,7 +1269,7 @@ struct ath10k {
> struct ath10k_per_peer_tx_stats peer_tx_stats;
>
> /* NAPI */
> - struct net_device napi_dev;
> + struct net_device *napi_dev;
> struct napi_struct napi;
>
> struct work_struct set_coverage_class_work;
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index 5c34b156b4ff..558bec96ae40 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -3217,7 +3217,7 @@ static void ath10k_pci_free_irq(struct ath10k *ar)
>
> void ath10k_pci_init_napi(struct ath10k *ar)
> {
> - netif_napi_add(&ar->napi_dev, &ar->napi, ath10k_pci_napi_poll);
> + netif_napi_add(ar->napi_dev, &ar->napi, ath10k_pci_napi_poll);
> }
>
> static int ath10k_pci_init_irq(struct ath10k *ar)
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index 0ab5433f6cf6..e28f2fe1101b 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -2532,7 +2532,7 @@ static int ath10k_sdio_probe(struct sdio_func *func,
> return -ENOMEM;
> }
>
> - netif_napi_add(&ar->napi_dev, &ar->napi, ath10k_sdio_napi_poll);
> + netif_napi_add(ar->napi_dev, &ar->napi, ath10k_sdio_napi_poll);
>
> ath10k_dbg(ar, ATH10K_DBG_BOOT,
> "sdio new func %d vendor 0x%x device 0x%x block 0x%x/0x%x\n",
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 2c39bad7ebfb..0449b9ffc32d 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -935,7 +935,7 @@ static int ath10k_snoc_hif_start(struct ath10k *ar)
>
> bitmap_clear(ar_snoc->pending_ce_irqs, 0, CE_COUNT_MAX);
>
> - dev_set_threaded(&ar->napi_dev, true);
> + dev_set_threaded(ar->napi_dev, true);
> ath10k_core_napi_enable(ar);
> ath10k_snoc_irq_enable(ar);
> ath10k_snoc_rx_post(ar);
> @@ -1253,7 +1253,7 @@ static int ath10k_snoc_napi_poll(struct napi_struct *ctx, int budget)
>
> static void ath10k_snoc_init_napi(struct ath10k *ar)
> {
> - netif_napi_add(&ar->napi_dev, &ar->napi, ath10k_snoc_napi_poll);
> + netif_napi_add(ar->napi_dev, &ar->napi, ath10k_snoc_napi_poll);
> }
>
> static int ath10k_snoc_request_irq(struct ath10k *ar)
> diff --git a/drivers/net/wireless/ath/ath10k/usb.c b/drivers/net/wireless/ath/ath10k/usb.c
> index 3c482baacec1..3b51b7f52130 100644
> --- a/drivers/net/wireless/ath/ath10k/usb.c
> +++ b/drivers/net/wireless/ath/ath10k/usb.c
> @@ -1014,7 +1014,7 @@ static int ath10k_usb_probe(struct usb_interface *interface,
> return -ENOMEM;
> }
>
> - netif_napi_add(&ar->napi_dev, &ar->napi, ath10k_usb_napi_poll);
> + netif_napi_add(ar->napi_dev, &ar->napi, ath10k_usb_napi_poll);
>
> usb_get_dev(dev);
> vendor_id = le16_to_cpu(dev->descriptor.idVendor);

I've pinged the development team to see if they can validate this on hardware
-- I don't have any ath10k setups.

Will ack this once I get confirmation.

/jeff

2024-03-19 17:15:41

by Breno Leitao

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

On Tue, Mar 19, 2024 at 09:05:24AM -0700, Jeff Johnson wrote:
> On 3/19/2024 3:47 AM, Breno Leitao wrote:
> > @@ -3687,6 +3690,8 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev,
> >
> > err_free_coredump:
> > ath10k_coredump_destroy(ar);
> > +err_free_netdev:
> > + free_netdev(ar->napi_dev);
> > err_free_tx_complete:
> > destroy_workqueue(ar->workqueue_tx_complete);
> > err_free_aux_wq:
> > @@ -3708,6 +3713,7 @@ void ath10k_core_destroy(struct ath10k *ar)
> >
> > destroy_workqueue(ar->workqueue_tx_complete);
> >
> > + free_netdev(ar->napi_dev);
> > ath10k_debug_destroy(ar);
> > ath10k_coredump_destroy(ar);
> > ath10k_htt_tx_destroy(&ar->htt);
>
> looks like there is a pre-existing issue that the order of operations in
> _destroy() doesn't match the order of operations in the _create() error path.

Right. I found it weird as well. Basically "ath10k_coredump" and
"ath10k_debug" operations are swapped between ath10k_core_create() and
ath10k_core_destroy().

If you wish, I can submit a patch ordering it properly.

> but the placement of your changes looks ok to me

Right. It is done in-between the workqueues and the coredump/debug
creation/destroy.

Thanks for the review.

2024-03-19 18:47:08

by Jeff Johnson

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

On 3/19/2024 10:15 AM, Breno Leitao wrote:
> On Tue, Mar 19, 2024 at 09:05:24AM -0700, Jeff Johnson wrote:
>> On 3/19/2024 3:47 AM, Breno Leitao wrote:
>>> @@ -3687,6 +3690,8 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev,
>>>
>>> err_free_coredump:
>>> ath10k_coredump_destroy(ar);
>>> +err_free_netdev:
>>> + free_netdev(ar->napi_dev);
>>> err_free_tx_complete:
>>> destroy_workqueue(ar->workqueue_tx_complete);
>>> err_free_aux_wq:
>>> @@ -3708,6 +3713,7 @@ void ath10k_core_destroy(struct ath10k *ar)
>>>
>>> destroy_workqueue(ar->workqueue_tx_complete);
>>>
>>> + free_netdev(ar->napi_dev);
>>> ath10k_debug_destroy(ar);
>>> ath10k_coredump_destroy(ar);
>>> ath10k_htt_tx_destroy(&ar->htt);
>>
>> looks like there is a pre-existing issue that the order of operations in
>> _destroy() doesn't match the order of operations in the _create() error path.
>
> Right. I found it weird as well. Basically "ath10k_coredump" and
> "ath10k_debug" operations are swapped between ath10k_core_create() and
> ath10k_core_destroy().
>
> If you wish, I can submit a patch ordering it properly.

Don't bother. I'll queue that up to fix separately myself

/jeff

2024-03-20 15:13:19

by Jeff Johnson

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

On 3/19/2024 3:47 AM, Breno Leitao wrote:
> Embedding net_device into structures prohibits the usage of flexible
> arrays in the net_device structure. For more details, see the discussion
> at [1].
>
> Un-embed the net_device from struct ath10k by converting it
> into a pointer. Then use the leverage alloc_netdev() to allocate the
> net_device object at ath10k_core_create(). The free of the device occurs
> at ath10k_core_destroy().
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Breno Leitao <[email protected]>

NAK this based upon the ath11k patch results.

As suggested there we should just use kmalloc/kfree to match the existing logic.

/jeff

2024-03-20 15:26:00

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

Jeff Johnson <[email protected]> writes:

> On 3/19/2024 3:47 AM, Breno Leitao wrote:
>> Embedding net_device into structures prohibits the usage of flexible
>> arrays in the net_device structure. For more details, see the discussion
>> at [1].
>>
>> Un-embed the net_device from struct ath10k by converting it
>> into a pointer. Then use the leverage alloc_netdev() to allocate the
>> net_device object at ath10k_core_create(). The free of the device occurs
>> at ath10k_core_destroy().
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>>
>> Signed-off-by: Breno Leitao <[email protected]>
>
> NAK this based upon the ath11k patch results.
>
> As suggested there we should just use kmalloc/kfree to match the existing logic.

BTW if the patch is not tested on a real device then it's good to
document that in the commit message with "Compile tested only" or
similar.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2024-03-20 17:02:11

by Breno Leitao

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

On Wed, Mar 20, 2024 at 05:25:52PM +0200, Kalle Valo wrote:
> Jeff Johnson <[email protected]> writes:
>
> > On 3/19/2024 3:47 AM, Breno Leitao wrote:
> >> Embedding net_device into structures prohibits the usage of flexible
> >> arrays in the net_device structure. For more details, see the discussion
> >> at [1].
> >>
> >> Un-embed the net_device from struct ath10k by converting it
> >> into a pointer. Then use the leverage alloc_netdev() to allocate the
> >> net_device object at ath10k_core_create(). The free of the device occurs
> >> at ath10k_core_destroy().
> >>
> >> [1] https://lore.kernel.org/all/[email protected]/
> >>
> >> Signed-off-by: Breno Leitao <[email protected]>
> >
> > NAK this based upon the ath11k patch results.
> >
> > As suggested there we should just use kmalloc/kfree to match the existing logic.
>
> BTW if the patch is not tested on a real device then it's good to
> document that in the commit message with "Compile tested only" or
> similar.

Good to know. Thanks. I will add it to the next patches.

2024-03-21 14:29:20

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

On Wed, 20 Mar 2024 08:12:46 -0700 Jeff Johnson wrote:
> NAK this based upon the ath11k patch results.

The ath11 patch is much more complex, I'd wager this one is fine.

> As suggested there we should just use kmalloc/kfree to match the existing logic.

Please no. There is no magic here. alloc + free must match whether
you're using magic object alloc wrapper (alloc_netdev()) or straight
up kzalloc().

2024-03-21 22:18:16

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

On Thu, 21 Mar 2024 15:02:39 -0700 Jeff Johnson wrote:
> >> As suggested there we should just use kmalloc/kfree to match the existing logic.
> >
> > Please no. There is no magic here. alloc + free must match whether
> > you're using magic object alloc wrapper (alloc_netdev()) or straight
> > up kzalloc().
>
> Based upon the ath11k patch there must be something going on with
> alloc_netdev()/free_netdev() that doesn't occur when these aren't used.

Looks like init_dummy_netdev wipes the netdev structure clean, so I
don't think we can use it directly as the setup function, Breno :(
Maybe we should add a new helper to "alloc dummy netdev" which can
call alloc_netdev() with right arguments and do necessary init?

> So I'm just suggesting that instead we use kmalloc() and kfree(), which are
> matching functions, and which, like the existing code, are not subject to
> whatever is happening in alloc_netdev()/free_netdev().
>
> I don't understand your objection.

Using subsystem APIs to allocate its objects is preferable to ad hoc
kmalloc().

We're working upstream, basic alloc/free of an object should work.
Took me 5 min to realize what the problem is.

2024-03-27 15:38:33

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

Jakub Kicinski <[email protected]> writes:

> On Fri, 22 Mar 2024 07:58:02 -0700 Breno Leitao wrote:
>> > Looks like init_dummy_netdev wipes the netdev structure clean, so I
>> > don't think we can use it directly as the setup function, Breno :(
>>
>> Before my patch, init_dummy_netdev was being also used. The patch was
>> basically replacing the init_dummy_netdev by alloc_netdev() with will
>> call "setup(dev);" later.
>>
>> - init_dummy_netdev(&irq_grp->napi_ndev);
>> + irq_grp->napi_ndev = alloc_netdev(0, "dummy", NET_NAME_UNKNOWN,
>> + init_dummy_netdev);
>>
>> I am wondering if alloc_netdev() is messing with something instead of
>> init_dummy_netdev().
>
> alloc_netdev() allocates some memory and initializes lists which
> free_netdev() wants to free, basically. But init_dummy_netdev() does:
>
> /* Clear everything. Note we don't initialize spinlocks
> * are they aren't supposed to be taken by any of the
> * NAPI code and this dummy netdev is supposed to be
> * only ever used for NAPI polls
> */
> memset(dev, 0, sizeof(struct net_device));
>
> so all those pointers and init alloc_netdev() did before calling setup
> will get wiped.

After some tweaking I was able to get an old x86 laptop with ath10k back
alive and I tested this patch using QCA9984/QCA9994 hw1.0 PCI device. As
expected it did crash during rmmod and it seems to crash because
dev->dev_addr is zero:

[ 130.849286] dev->dev_addr 00000000 dev->dev_addr_shadow 5acebe41

Breno, I can now easily test new ath10k and ath11k patches, just let me
know. Here's the full back trace:

[ 130.849310] BUG: kernel NULL pointer dereference, address: 00000000
[ 130.849413] #PF: supervisor read access in kernel mode
[ 130.849450] #PF: error_code(0x0000) - not-present page
[ 130.849486] *pdpt = 00000000048b9001 *pde = 0000000000000000
[ 130.849529] Oops: 0000 [#1] PREEMPT SMP PTI
[ 130.849564] CPU: 1 PID: 882 Comm: rmmod Tainted: G E 6.9.0-rc1-wt-ath+ #17
[ 130.849617] Hardware name: Hewlett-Packard HP ProBook 6540b/1722, BIOS 68CDD Ver. F.04 01/27/2010
[ 130.849671] EIP: memcmp+0x4c/0x54
[ 130.849706] Code: 39 d9 74 0a 0f b6 03 0f b6 32 29 f0 74 ec 5b 5e 5d c3 8d b4 26 00 00 00 00 90 83 e9 04 83 c3 04 83 c2 04 83 f9 03 76 c2 8b 02 <39> 03 74 ec eb c0 66 90 55 89 e5 e8 a4 ff ff ff 5d c3 66 90 55 89
[ 130.849809] EAX: 00000000 EBX: 00000000 ECX: 00000020 EDX: c32c1d84
[ 130.854170] ESI: c32c1d84 EDI: 00000000 EBP: c4b87e2c ESP: c4b87e24
[ 130.854222] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010216
[ 130.854274] CR0: 80050033 CR2: 00000000 CR3: 03348000 CR4: 000006f0
[ 130.854324] Call Trace:
[ 130.854354] ? show_regs+0x74/0x7c
[ 130.854393] ? __die+0x1d/0x58
[ 130.854425] ? page_fault_oops+0x16c/0x340
[ 130.854469] ? kernelmode_fixup_or_oops.constprop.0+0x84/0xd4
[ 130.854518] ? __bad_area_nosemaphore.constprop.0+0x10e/0x1a4
[ 130.854567] ? lock_mm_and_find_vma+0xeb/0x288
[ 130.854612] ? bad_area_nosemaphore+0xf/0x14
[ 130.854652] ? do_user_addr_fault+0x238/0x454
[ 130.854693] ? exc_page_fault+0x58/0x170
[ 130.854734] ? pvclock_clocksource_read_nowd+0x130/0x130
[ 130.854781] ? handle_exception+0x150/0x150
[ 130.854824] ? posix_cpu_timer_del+0x17/0x140
[ 130.854867] ? pvclock_clocksource_read_nowd+0x130/0x130
[ 130.854914] ? memcmp+0x4c/0x54
[ 130.854947] ? pvclock_clocksource_read_nowd+0x130/0x130
[ 130.857510] ? memcmp+0x4c/0x54
[ 130.860078] dev_addr_check+0x27/0xd4
[ 130.862635] dev_addr_flush+0x18/0x78
[ 130.865178] free_netdev+0x76/0x1b0
[ 130.867708] ath10k_core_destroy+0x54/0x84 [ath10k_core]
[ 130.870283] ath10k_pci_remove+0x88/0xcc [ath10k_pci]
[ 130.872831] pci_device_remove+0x38/0x90
[ 130.875311] device_remove+0x37/0x58
[ 130.877757] device_release_driver_internal+0x185/0x1d8
[ 130.880182] driver_detach+0x3c/0x78
[ 130.882603] bus_remove_driver+0x56/0xc0
[ 130.885019] driver_unregister+0x25/0x40
[ 130.887378] pci_unregister_driver+0x21/0x60
[ 130.889702] ath10k_pci_exit+0xd/0x3d0 [ath10k_pci]
[ 130.892007] __ia32_sys_delete_module+0x163/0x254
[ 130.894291] ? preempt_count_add+0x6d/0xbc
[ 130.896504] ? debug_smp_processor_id+0x12/0x14
[ 130.898711] ? fpregs_assert_state_consistent+0x29/0x50
[ 130.900883] __do_fast_syscall_32+0x57/0xd0
[ 130.903026] do_fast_syscall_32+0x29/0x58
[ 130.905178] do_SYSENTER_32+0x15/0x18
[ 130.907295] entry_SYSENTER_32+0xa2/0x102
[ 130.909421] EIP: 0xb7ef0569
[ 130.911523] Code: 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d 76 00 58 b8 77 00 00 00 cd 80 90 8d 76
[ 130.916271] EAX: ffffffda EBX: 010f618c ECX: 00000800 EDX: 0044b1f9
[ 130.918758] ESI: 010f6150 EDI: 00000001 EBP: bf8957f3 ESP: bf8941b8
[ 130.921221] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000202
[ 130.923696] Modules linked in: ath10k_usb(E) ath10k_sdio(E) ath10k_pci(E-) ath10k_core(E) ath(E) mac80211(E) cfg80211(E) libarc4(E) snd_hda_codec_hdmi snd_ctl_led ledtrig_audio snd_hda_codec_idt snd_hda_codec_generic intel_powerclamp coretemp snd_hda_intel wmi_bmof kvm_intel snd_intel_dspcfg uvcvideo i915 kvm videobuf2_vmalloc snd_hda_codec i2c_algo_bit drm_buddy snd_hwdep videobuf2_memops btusb uvc drm_display_helper intel_cstate snd_hda_core videobuf2_v4l2 btrtl btintel drm_kms_helper snd_pcm joydev videodev btbcm input_leds snd_timer serio_raw intel_ips binfmt_misc ttm videobuf2_common bluetooth snd lpc_ich mei_me video mc ecdh_generic soundcore ecc drm mei tpm_infineon wmi mac_hid sch_fq_codel parport_pc ppdev lp parport autofs4 netconsole firewire_ohci sdhci_pci cqhci firewire_core ahci psmouse sdhci sky2 crc_itu_t libahci [last unloaded: libarc4]
[ 130.937173] CR2: 0000000000000000
[ 130.939863] ---[ end trace 0000000000000000 ]---
[ 130.942404] EIP: memcmp+0x4c/0x54
[ 130.944777] Code: 39 d9 74 0a 0f b6 03 0f b6 32 29 f0 74 ec 5b 5e 5d c3 8d b4 26 00 00 00 00 90 83 e9 04 83 c3 04 83 c2 04 83 f9 03 76 c2 8b 02 <39> 03 74 ec eb c0 66 90 55 89 e5 e8 a4 ff ff ff 5d c3 66 90 55 89
[ 130.950719] EAX: 00000000 EBX: 00000000 ECX: 00000020 EDX: c32c1d84
[ 130.952997] ESI: c32c1d84 EDI: 00000000 EBP: c4b87e2c ESP: c4b87e24
[ 130.955166] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010216
[ 130.957339] CR0: 80050033 CR2: 00000000 CR3: 03348000 CR4: 000006f0


--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2024-03-27 15:52:30

by Jeff Johnson

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

On 3/27/2024 7:45 AM, Jakub Kicinski wrote:
> On Wed, 27 Mar 2024 07:38:05 -0700 Breno Leitao wrote:
>> -void init_dummy_netdev(struct net_device *dev)
>> +void init_dummy_netdev_core(struct net_device *dev)
>
> Can init_dummy_netdev_core() be a static function (and no export)?
> alloc_netdev_dummy() is probably going to be the only user.
>
> I'd also lean towards squashing the two commits into one.

And once all of the conversions are complete, won't init_dummy_netdev() no
longer be used and can be removed?




2024-03-27 16:17:21

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH] ath10k: allocate dummy net_device dynamically

On Wed, 27 Mar 2024 08:42:55 -0700 Jeff Johnson wrote:
> On 3/27/2024 7:45 AM, Jakub Kicinski wrote:
> > On Wed, 27 Mar 2024 07:38:05 -0700 Breno Leitao wrote:
> >> -void init_dummy_netdev(struct net_device *dev)
> >> +void init_dummy_netdev_core(struct net_device *dev)
> >
> > Can init_dummy_netdev_core() be a static function (and no export)?
> > alloc_netdev_dummy() is probably going to be the only user.
> >
> > I'd also lean towards squashing the two commits into one.
>
> And once all of the conversions are complete, won't init_dummy_netdev() no
> longer be used and can be removed?

That's right, it should be removed. But it may take another release
cycle until we can do that, depending on how quickly the patches
propagate :(