Convert IDPF to use auxiliary bus #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
madhuchittim
force-pushed
the
aux_bus
branch
6 times, most recently
from
41df721 to
a156d88
Compare
etantilov
reviewed
Apr 19, 2024
burra006
reviewed
Apr 23, 2024
burra006
reviewed
Apr 23, 2024
burra006
reviewed
Apr 23, 2024
burra006
reviewed
Apr 23, 2024
|
|
||
| extern struct idpf_eth_idc_auxiliary_driver *idpf_eth_get_driver(void); | ||
| /** | ||
| * idpf_eth_idc_get_driver - returns eth driver ops |
There was a problem hiding this comment.
please check all the comment headers. Many places there is a mismatch...
Collaborator
There was a problem hiding this comment.
Let me know if I missed something. Needs review.
madhuchittim
commented
Apr 23, 2024
| } | ||
|
|
||
| do_memcpy: | ||
| memcpy(rss_data->rss_key, recv_rk->key_flex, rss_data->rss_key_size); |
There was a problem hiding this comment.
recv_rk memory is leaked here unless I am missing something.
Collaborator
There was a problem hiding this comment.
Not sure what leak is referred here. The driver loads fine and I didn't see any issue with the driver.
There was a problem hiding this comment.
I referred specifically to recv_rk. If you examine the function, I don't see it being freed. Loading of driver has nothng to do with a few memory blocks leaking.
Collaborator
There was a problem hiding this comment.
I will keep the original code as it as that is not the purpose of this PR . The other issues (non aux related changes) can be addressed with a separate PR.
| @@ -1,18 +1,29 @@ | |||
| # SPDX-License-Identifier: GPL-2.0-only | |||
| # Copyright (C) 2023 Intel Corporation | |||
| # Copyright (C) 2024 Intel Corporation | |||
There was a problem hiding this comment.
Copyright year is year of coding, not year of modification.
Owner
Author
There was a problem hiding this comment.
Pavan had a comment on this to be changed. @plinga1 Can you please confirm on this.
There was a problem hiding this comment.
There was a comment when we upstreamed IDPF to update the copyright year to the current year the driver makes into the upstream. Couldnt find the comment but you may consult Tony for additional info.
There was a problem hiding this comment.
See here is my past experience with Copyright. When the file is first coded, the copyright line shows that year e.g.
Copyright (C) 2021 Rafał Miłecki rafal@milecki.pl
Now suppose above file is modified again in 2022, the copyright should be
Copyright (C) 2021-2022 Rafał Miłecki rafal@milecki.pl
Then the file gets modified in 2024 (means not modified in 2023), the copyright should read:
Copyright (C) 2021-2022, 2024 Rafał Miłecki rafal@milecki.pl
Essentially, every year that you modify the file should be listed via range (using dash) or explicitly
Collaborator
There was a problem hiding this comment.
I initially talked to Tony about it, and I had left it to the original code date of this driver based on the feedback from him. I will ask him again about it.
Owner
Author
There was a problem hiding this comment.
I have caused a problem here.. some of the files I moved the year based on Pavan's comments so some of them would end up showing as 2024
Collaborator
There was a problem hiding this comment.
No worry, I will fix them again.
| dev_info = adapter->dev_info; | ||
| eth_shared = idpf_eth_adapter_shared(adapter); | ||
| pdev = eth_shared->pdev; | ||
| adapter->req_tx_splitq = true; |
There was a problem hiding this comment.
Should we remove these flags if they are always true ? Or is there some mechanism to make it false
Collaborator
There was a problem hiding this comment.
It is part of original code. I don't want to divert from the definition of this PR for making further changes to this PR.
Collaborator
There was a problem hiding this comment.
How the OOT is doing? I am not sure why it is there in first place? Will it ever need in future?
burra006
reviewed
May 2, 2024
burra006
reviewed
May 3, 2024
tksingh-amr
force-pushed
the
aux_bus
branch
6 times, most recently
from
614edb3 to
4c367cb
Compare
burra006
reviewed
May 6, 2024
tksingh-amr
force-pushed
the
aux_bus
branch
7 times, most recently
from
a97a5dc to
4d486cf
Compare
Repository owner
deleted a comment from
burra006
May 10, 2024
tksingh-amr
force-pushed
the
aux_bus
branch
2 times, most recently
from
37f6713 to
621bcfc
Compare
| * idpf_eth_device_post_init_task - Delayed ethernet post initialization task | ||
| * @work: work_struct handle to ethernet adapter data | ||
| */ | ||
| static void idpf_eth_device_post_init_task(struct work_struct *work) |
There was a problem hiding this comment.
I don't see why this is a task. There is nothing blocking here. So why spawn a task ?
Collaborator
There was a problem hiding this comment.
Please see this flag "adapter->start_post_init" in this function. It is a blocking factor to re spawn.
| return -ENOMEM; | ||
|
|
||
| /* Initialize dev_info */ | ||
| adapter->dev_info = ð_dev->eth_info; |
There was a problem hiding this comment.
assignment to dev_info can be deleted . IT is being done below.
| u32 tx_timeout_count; | ||
| bool req_tx_splitq; | ||
| bool req_rx_splitq; | ||
| u32 *vport_ids; |
There was a problem hiding this comment.
Why is this plural and pointer ? I think there is only one vport_id for one ethernet adapter
Added new files 'idpf_eth_idc.c', idpf_eth.c, idpf_eth.h, and 'idpf_eth_common.h' Added new 'struct idpf_eth_adapter' to make ethernet device's logical separation from main driver. Moved all ethernet related members into this new structure, and corresponding functions into new files 'idpf_eth.c' and 'idpf_eth.h'. Added inter device communication (idc) functions in new file 'idpf_eth_idc.c'. The idc file contains all the necessary hooks to communicate with separated logical device (eth). Many ethernet device code function argumets are now changed from idpf_adapter to idpf_eth_adapter in corresponding ethernet code files. Added 'struct idpf_eth_idc_auxiliary_dev' for tracking eth device creation and deletion. The eth device information is stored in idpf_eth_shared structure for easy access in both lower and upper layers. A new 'struct idpf_eth_idc_dev_info' is added to pass device init parameters like queue, capabilities etc. to ethernet device from main driver. The capabilities (caps) macros are moved into the file 'idpf_eth_idc.h', so that both main and ethernet drivers can access these macros. After the primary pci device's reset and initialization flow, a new task 'idc_eth_init_task' is scheduled. This new task creates ethernet device structure information, allocates ethernet adapter structure, and initializes eth portion of the driver which now takes over the processing of vports of ethernet device (eth). This patch impliments ethernet device as a single ethernet device adapter having default single or multiple vport(s). Ethernet device initialization is a two part process through functions idpf_eth_device_pre_init(), and idpf_eth_device_post_init(). Pre init runs task idpf_eth_device_pre_init_task() which checks to see if all vports devices are pre-initialized and ready for moving on to post init step. Post init step enables the vport's availability into the system. Added events for propagating the information from main driver to ethernet portion of the driver.Added 'enum idpf_eth_idc_event_type' to indicate whether an event is for all vports/ethernet adapters or single vport/ethernet adapter. Added a local flags declaration into 'struct idpf_eth_adapter', and corresponding 'enum idpf_eth_flags' for keeping lower level events information. Removed reference or dependency of 'struct idpf_adapter' i.e. the header file 'idpf.h' from all the files related to ethernet operations. These files are: idpf_eth.c/h, idpf_mac.c/h, idpf_netdev.c/h, idpf_vport.c/h, idpf_vport_config.c, idpf_vport_q.c, idpf_txrx.c, and idpf_singleq_txrx.c. These files now include idpf_eth.h instead of idpf.h. Renamed variable 'vport_ctrl_lock' to 'reset_lock' and associated function to reflect its usage nature. Removed main driver's lock dependency into the ethernet drivers. Ethernet driver code now refers to local version og 'vport_ctrl_lock' and flags in eth_adapter to take decisions. The local flags information is updated via events from main driver. In function idpf_decfg_netdev(), added a netdev register check before unregistering. The content of 'idpf_virtchnl.h' are moved into 'idpf_eth_common.h' as they are common for both, and file 'idpf_virtchnl.h' has been removed. Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com> Signed-off-by: Shailendra Bhatnagar <shailendra.bhatnagar@intel.com>
Added capability to allocate multiple adapter instance based on number of vports. Most of the double pointers into 'struct idpf_eth_adapter'are converted into single pointer to hold data structure for vport, netdev, vport_config, vport_params_reqd, and vport_params_recvd. Removed unused following function(s) while making these changes: idpf_get_free_slot(), idpf_vport_params_buf_alloc(), idpf_vid_to_vport(), and few others. In 'struct idpf_eth_adapter' removed pre_init_task, pre_init_wq, members vport_ids, next_vport, and num_alloc_vports. They are not required into the multi instance adapter implementation. In 'struct idpf_eth_idc_dev_info' a new member 'bool start_probe_post_init' is added so that the probe post init can be triggered concurrently. Added new event to trigger the post initialization part of ethernet probe. Added new function idpf_eth_idc_vid_to_dev_info() to retrieve the device instance based on vport id associated with it. Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com> Signed-off-by: Shailendra Bhatnagar <shailendra.bhatnagar@intel.com>
Created new directory 'eth' to separate the ethernet driver files. Following files are now part of 'eth' and represents ethernet portion of the driver. Files:idpf_txrx.c, idpf_txrx.h, idpf_singleq_txrx.c, idpf_ethtool.c, idpf_mac.c, idpf_mac.h, idpf_netdev.c, idpf_netdev.h, idpf_vport.c idpf_vport.h, idpf_vport_config.c, idpf_vport_q.c, idpf_eth.c, and idpf_eth.h Makefile modification are done to include directory name 'eth' for above moved 'c' files, and a flag. Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com>
It now uses the auxiliary bus to manage ethernet part of driver and its devices. In 'struct idpf_eth_idc_auxiliary_driver' added new member to save signature, and another member 'struct auxiliary_driver adrv' to keep auxiliary device information. Renamed function idpf_eth_device_add() to idpf_eth_probe() to reflect auxiliary device probe. Added new function 'idpf_eth_idc_driver_register()' to register auxiliary driver. And added corespondig call to unregister the auxiliary driver in function 'idpf_eth_idc_driver_unregister()'. Added module_init(idpf_driver_register), and module_exit(idpf_driver_unregister) to register/unregister the auxiliary device information. Added management auxiliary call 'auxiliary_device_init()' and 'auxiliary_device_add()' to initialize the device and to request the system to start the probe for device(s). Added new function 'idpf_eth_idc_device_deinit()' to manage the delete and uninit the auxuliary devices. Removed unused function after conversion idpf_eth_unregister(), and removed variables idpf_context, and eth_context as they are not required. In this new model those are retrieved from the system devices references now on need basis. Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com> Signed-off-by: Shailendra Bhatnagar <shailendra.bhatnagar@intel.com>
| */ | ||
| struct idpf_adapter { | ||
| /* Do not move eth_shared location */ | ||
| struct idpf_eth_shared eth_shared; |
Contributor
There was a problem hiding this comment.
static_assert(offsetof(...) == 0) can be used here
madhuchittim
force-pushed
the
dev-queue
branch
from
ed2dfdd to
8bf4fd7
Compare
azaki1
pushed a commit
that referenced
this pull request
Feb 15, 2025
We have recently seen reports of lockdep circular lock dependency warnings
when loading the iAVF driver:
[ 1504.790308] ======================================================
[ 1504.790309] WARNING: possible circular locking dependency detected
[ 1504.790310] 6.13.0 #net_next_rt.c2933b2befe2.el9 Not tainted
[ 1504.790311] ------------------------------------------------------
[ 1504.790312] kworker/u128:0/13566 is trying to acquire lock:
[ 1504.790313] ffff97d0e4738f18 (&dev->lock){+.+.}-{4:4}, at: register_netdevice+0x52c/0x710
[ 1504.790320]
[ 1504.790320] but task is already holding lock:
[ 1504.790321] ffff97d0e47392e8 (&adapter->crit_lock){+.+.}-{4:4}, at: iavf_finish_config+0x37/0x240 [iavf]
[ 1504.790330]
[ 1504.790330] which lock already depends on the new lock.
[ 1504.790330]
[ 1504.790330]
[ 1504.790330] the existing dependency chain (in reverse order) is:
[ 1504.790331]
[ 1504.790331] -> #1 (&adapter->crit_lock){+.+.}-{4:4}:
[ 1504.790333] __lock_acquire+0x52d/0xbb0
[ 1504.790337] lock_acquire+0xd9/0x330
[ 1504.790338] mutex_lock_nested+0x4b/0xb0
[ 1504.790341] iavf_finish_config+0x37/0x240 [iavf]
[ 1504.790347] process_one_work+0x248/0x6d0
[ 1504.790350] worker_thread+0x18d/0x330
[ 1504.790352] kthread+0x10e/0x250
[ 1504.790354] ret_from_fork+0x30/0x50
[ 1504.790357] ret_from_fork_asm+0x1a/0x30
[ 1504.790361]
[ 1504.790361] -> #0 (&dev->lock){+.+.}-{4:4}:
[ 1504.790364] check_prev_add+0xf1/0xce0
[ 1504.790366] validate_chain+0x46a/0x570
[ 1504.790368] __lock_acquire+0x52d/0xbb0
[ 1504.790370] lock_acquire+0xd9/0x330
[ 1504.790371] mutex_lock_nested+0x4b/0xb0
[ 1504.790372] register_netdevice+0x52c/0x710
[ 1504.790374] iavf_finish_config+0xfa/0x240 [iavf]
[ 1504.790379] process_one_work+0x248/0x6d0
[ 1504.790381] worker_thread+0x18d/0x330
[ 1504.790383] kthread+0x10e/0x250
[ 1504.790385] ret_from_fork+0x30/0x50
[ 1504.790387] ret_from_fork_asm+0x1a/0x30
[ 1504.790389]
[ 1504.790389] other info that might help us debug this:
[ 1504.790389]
[ 1504.790389] Possible unsafe locking scenario:
[ 1504.790389]
[ 1504.790390] CPU0 CPU1
[ 1504.790391] ---- ----
[ 1504.790391] lock(&adapter->crit_lock);
[ 1504.790393] lock(&dev->lock);
[ 1504.790394] lock(&adapter->crit_lock);
[ 1504.790395] lock(&dev->lock);
[ 1504.790397]
[ 1504.790397] *** DEADLOCK ***
This appears to be caused by the change in commit 5fda3f3 ("net: make
netdev_lock() protect netdev->reg_state"), which added a netdev_lock() in
register_netdevice.
The iAVF driver calls register_netdevice() from iavf_finish_config(), as a
final stage of its state machine post-probe. It currently takes the RTNL
lock, then the netdev lock, and then the device critical lock. This pattern
is used throughout the driver. Thus there is a strong dependency that the
crit_lock should not be acquired before the net device lock. The change to
register_netdevice creates an ABBA lock order violation because the iAVF
driver is holding the crit_lock while calling register_netdevice, which
then takes the netdev_lock.
It seems likely that future refactors could result in netdev APIs which
hold the netdev_lock while calling into the driver. This means that we
should not re-order the locks so that netdev_lock is acquired after the
device private crit_lock.
Instead, notice that we already release the netdev_lock prior to calling
the register_netdevice. This flow only happens during the early driver
initialization as we transition through the __IAVF_STARTUP,
__IAVF_INIT_VERSION_CHECK, __IAVF_INIT_GET_RESOURCES, etc.
Analyzing the places where we take crit_lock in the driver there are two
sources:
a) several of the work queue tasks including adminq_task, watchdog_task,
reset_task, and the finish_config task.
b) various callbacks which ultimately stem back to .ndo operations or
ethtool operations.
The latter cannot be triggered until after the netdevice registration is
completed successfully.
The iAVF driver uses alloc_ordered_workqueue, which is an unbound workqueue
that has a max limit of 1, and thus guarantees that only a single work item
on the queue is executing at any given time, so none of the other work
threads could be executing due to the ordered workqueue guarantees.
The iavf_finish_config() function also does not do anything else after
register_netdevice, unless it fails. It seems unlikely that the driver
private crit_lock is protecting anything that register_netdevice() itself
touches.
Thus, to fix this ABBA lock violation, lets simply release the
adapter->crit_lock as well as netdev_lock prior to calling
register_netdevice(). We do still keep holding the RTNL lock as required by
the function. If we do fail to register the netdevice, then we re-acquire
the adapter critical lock to finish the transition back to
__IAVF_INIT_CONFIG_ADAPTER.
This ensures every call where both netdev_lock and the adapter->crit_lock
are acquired under the same ordering.
Fixes: afc6649 ("eth: iavf: extend the netdev_lock usage")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series converts the idpf driver to use the auxiliary bus.
The main motivation for this patch series is to address the following topics:
separation into PCIe and Ethernet driver component.
First 5 patches are in preparation for enabling the code changes for auxiliary
bus plug in. They are only refactoring the existing code to different files. The
similar functionality is accomodated into different files. This enables separating
the ethernet functionality for future patches.
Patch 6 introduces the IDC shared header file. This file is for inter device
communication.
Patch 7 creates logical separation of ethernet portion of the code, but it is
still a global single adapter for all the vports.
Patch 8 makes it multi-instance driver like each vport will have its own adapter instance.
Patch 9 moves the ethernet functionality into eth directory.
And finally Patch 10 connect the ethernet portion of the driver to the auxiliary bus.