| Message ID | 20240319104754.2535294-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath10k: allocate dummy net_device dynamically | expand |
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/20240229225910.79e224cf@kernel.org/ > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > 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
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.
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
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/20240229225910.79e224cf@kernel.org/ > > Signed-off-by: Breno Leitao <leitao@debian.org> NAK this based upon the ath11k patch results. As suggested there we should just use kmalloc/kfree to match the existing logic. /jeff
Jeff Johnson <quic_jjohnson@quicinc.com> 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/20240229225910.79e224cf@kernel.org/ >> >> Signed-off-by: Breno Leitao <leitao@debian.org> > > 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.
On Wed, Mar 20, 2024 at 05:25:52PM +0200, Kalle Valo wrote: > Jeff Johnson <quic_jjohnson@quicinc.com> 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/20240229225910.79e224cf@kernel.org/ > >> > >> Signed-off-by: Breno Leitao <leitao@debian.org> > > > > 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.
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().
On 3/21/2024 7:28 AM, Jakub Kicinski wrote: > 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(). 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. 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. /jeff
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.
Hello Jakub, On Thu, Mar 21, 2024 at 03:17:44PM -0700, Jakub Kicinski wrote: > 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 :( 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(). Also, Kalle's crash is during rmmod, and not during initialization. getting NULL after free_netdev() is called. > Maybe we should add a new helper to "alloc dummy netdev" which can > call alloc_netdev() with right arguments and do necessary init? What are the right arguments in this case? Thanks!
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. > Also, Kalle's crash is during rmmod, and not during initialization. > getting NULL after free_netdev() is called. > > > Maybe we should add a new helper to "alloc dummy netdev" which can > > call alloc_netdev() with right arguments and do necessary init? > > What are the right arguments in this case? I'm not sure we have a noop setup() callback today. If you define a wrapper to allocate a dummy netdev you can define a new empty function next to it and pass that as init? Hope I got the question right.
Hello Jakub, On Fri, Mar 22, 2024 at 08:23:36AM -0700, Jakub Kicinski wrote: > > > Maybe we should add a new helper to "alloc dummy netdev" which can > > > call alloc_netdev() with right arguments and do necessary init? > > > > What are the right arguments in this case? > > I'm not sure we have a noop setup() callback today. If you define a > wrapper to allocate a dummy netdev you can define a new empty function > next to it and pass that as init? Hope I got the question right. Thanks for the explanation, it is clear now. I've been working on it, and this is what I came up with. This is compile-tested by now, and, if this is what you had in mind, I will do more extensive testing. commit db794d99950f68731884a67d911094d94179c522 Author: Breno Leitao <leitao@debian.org> Date: Wed Mar 27 07:20:03 2024 -0700 net: Create net_device allocator for dummy Create a helper to allocate and initialize dummy netdevices. This function basically simplify the allocation of dummy devices, by allocating and initializing it. Suggested-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Breno Leitao <leitao@debian.org> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 643d2b469c49..9d1a5383c23f 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -4546,6 +4546,9 @@ static inline void netif_addr_unlock_bh(struct net_device *dev) void ether_setup(struct net_device *dev); +/* Allocate dummy net_device */ +struct net_device *alloc_netdev_dummy(int sizeof_priv, const char *name); + /* Support for loadable net-drivers */ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, unsigned char name_assign_type, diff --git a/net/core/dev.c b/net/core/dev.c index a08d698fe45c..628f35c3cfa2 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10998,6 +10998,13 @@ void free_netdev(struct net_device *dev) } EXPORT_SYMBOL(free_netdev); +struct net_device *alloc_netdev_dummy(int sizeof_priv, const char *name) +{ + return alloc_netdev(sizeof_priv, name, NET_NAME_UNKNOWN, + init_dummy_netdev_core); +} +EXPORT_SYMBOL_GPL(alloc_netdev_dummy); + /** * synchronize_net - Synchronize with packet receive processing * commit 35500cd6a5db0bbdedbc1067758948769c7ce57e Author: Breno Leitao <leitao@debian.org> Date: Wed Mar 27 07:07:40 2024 -0700 net: Split init_dummy_netdev It is impossible to use init_dummy_netdev together with alloc_netdev() as the 'setup' argument. This is because alloc_netdev() initializes some fields in the net_device structure, and later init_dummy_netdev() memzero them all. This casues some problems as reported here: https://lore.kernel.org/all/20240322082336.49f110cc@kernel.org/ Split the function in two. Create a new function called init_dummy_netdev_core() that does not memset the net_device structure. Then have init_dummy_netdev() memseting and calling init_dummy_netdev_core(). init_dummy_netdev_core() will be the function that could be called as an argument for alloc_netdev(). Signed-off-by: Breno Leitao <leitao@debian.org> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index c6f6ac779b34..643d2b469c49 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3119,6 +3119,7 @@ int netdev_refcnt_read(const struct net_device *dev); void free_netdev(struct net_device *dev); void netdev_freemem(struct net_device *dev); void init_dummy_netdev(struct net_device *dev); +void init_dummy_netdev_core(struct net_device *dev); struct net_device *netdev_get_xmit_slave(struct net_device *dev, struct sk_buff *skb, diff --git a/net/core/dev.c b/net/core/dev.c index 0766a245816b..a08d698fe45c 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10340,25 +10340,11 @@ int register_netdevice(struct net_device *dev) } EXPORT_SYMBOL(register_netdevice); -/** - * init_dummy_netdev - init a dummy network device for NAPI - * @dev: device to init - * - * This takes a network device structure and initialize the minimum - * amount of fields so it can be used to schedule NAPI polls without - * registering a full blown interface. This is to be used by drivers - * that need to tie several hardware interfaces to a single NAPI - * poll scheduler due to HW limitations. +/* Same as init_dummy_netdev, but, basically do not call memset. + * This is useful if you are calling this function after alloc_netdev() */ -void init_dummy_netdev(struct net_device *dev) +void init_dummy_netdev_core(struct net_device *dev) { - /* 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)); - /* make sure we BUG if trying to hit standard * register/unregister code path */ @@ -10379,8 +10365,29 @@ void init_dummy_netdev(struct net_device *dev) * its refcount. */ } -EXPORT_SYMBOL_GPL(init_dummy_netdev); +EXPORT_SYMBOL_GPL(init_dummy_netdev_core); +/** + * init_dummy_netdev - init a dummy network device for NAPI + * @dev: device to init + * + * This takes a network device structure and initialize the minimum + * amount of fields so it can be used to schedule NAPI polls without + * registering a full blown interface. This is to be used by drivers + * that need to tie several hardware interfaces to a single NAPI + * poll scheduler due to HW limitations. + */ +void init_dummy_netdev(struct net_device *dev) +{ + /* 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)); + init_dummy_netdev_core(dev); +} +EXPORT_SYMBOL_GPL(init_dummy_netdev); /** * register_netdev - register a network device
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.
Jakub Kicinski <kuba@kernel.org> 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
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?
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 :(
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);
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/20240229225910.79e224cf@kernel.org/ Signed-off-by: Breno Leitao <leitao@debian.org> --- 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(-)