| Message ID | 20230217225558.19837-1-shannon.nelson@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pds_core driver | expand |
On Fri, Feb 17, 2023 at 02:55:44PM -0800, Shannon Nelson wrote: > Summary: > -------- > This patchset implements new driver for use with the AMD/Pensando > Distributed Services Card (DSC), intended to provide core configuration > services through the auxiliary_bus for VFio and vDPA feature specific > drivers. Hi, I didn't look very deeply to this series, but three things caught my attention and IMHO they need to be changed/redesinged before someone can consider to merge it. 1. Use of bus_register_notifier to communicate between auxiliary devices. This whole concept makes aux logic in this driver looks horrid. The idea of auxiliary bus is separate existing device to sub-devices, while every such sub-device is controlled through relevant subsystem. Current implementation challenges this assumption by inventing own logic. 2. devm_* interfaces. It is bad. Please don't use them in such a complex driver. 3. Listen to PCI BOUND_DRIVER event can't be correct either. Thanks
On 2/19/23 2:11 AM, Leon Romanovsky wrote: > On Fri, Feb 17, 2023 at 02:55:44PM -0800, Shannon Nelson wrote: >> Summary: >> -------- >> This patchset implements new driver for use with the AMD/Pensando >> Distributed Services Card (DSC), intended to provide core configuration >> services through the auxiliary_bus for VFio and vDPA feature specific >> drivers. > > Hi, > > I didn't look very deeply to this series, but three things caught my > attention and IMHO they need to be changed/redesinged before someone > can consider to merge it. > > 1. Use of bus_register_notifier to communicate between auxiliary devices. > This whole concept makes aux logic in this driver looks horrid. The idea > of auxiliary bus is separate existing device to sub-devices, while every > such sub-device is controlled through relevant subsystem. Current > implementation challenges this assumption by inventing own logic. > 2. devm_* interfaces. It is bad. Please don't use them in such a complex > driver. > 3. Listen to PCI BOUND_DRIVER event can't be correct either. > > Thanks Hi Leon, Thanks for your comments. I’ll respond to 1 and 3 together. > 1. Use of bus_register_notifier to communicate between auxiliary devices. > 3. Listen to PCI BOUND_DRIVER event can't be correct either We’re only using the notifier for the core driver to know when to create and destroy auxiliary devices, not for communicate between auxiliary devices or drivers – I agree, that would be ugly. We want to create the auxiliary device after a VF PCI device is bound to a driver (BUS_NOTIFY_BOUND_DRIVER), and remove that auxiliary device just before a VF is unbound from its PCI driver (BUS_NOTIFY_UNBIND_DRIVER); bus_notify_register gives us access to these messages. I believe this is not too far off from other examples that can be seen in vfio/pci/vfio_pci_core, net/ethernet/ibm/emac, and net/mctp. Early on we were creating and deleting the auxiliary devices at the same time as calling sriov_enable() and sriov_disable() but found that when the user was doing unbind/bind operations we could end up with in-operative clients and occasional kernel panics because the devices and drivers were out-of-sync. Listening for the bind/unbind events allows the pds_core driver to make sure the auxiliary devices serving each of the VF drivers are kept in sync with the state of the VF PCI drivers. > 2. devm_* interfaces Can you elaborate a bit on why you see using the devm interfaces as bad? I don’t see the code complexity being any different than using the non-devm interfaces, and these give the added protection of making sure to not leak resources on driver removals, whether clean or on error. We are using these allocations for longer term driver structures, not for ephemeral short-term use buffers or fast-path operations, so the overhead should remain minimal. It seems to me this is the advertised use as described in the devres notes under Documentation. Thanks, sln
On Mon, Feb 20, 2023 at 03:53:02PM -0800, Shannon Nelson wrote: > On 2/19/23 2:11 AM, Leon Romanovsky wrote: > > On Fri, Feb 17, 2023 at 02:55:44PM -0800, Shannon Nelson wrote: > > > Summary: > > > -------- > > > This patchset implements new driver for use with the AMD/Pensando > > > Distributed Services Card (DSC), intended to provide core configuration > > > services through the auxiliary_bus for VFio and vDPA feature specific > > > drivers. > > > > Hi, > > > > I didn't look very deeply to this series, but three things caught my > > attention and IMHO they need to be changed/redesinged before someone > > can consider to merge it. > > > > 1. Use of bus_register_notifier to communicate between auxiliary devices. > > This whole concept makes aux logic in this driver looks horrid. The idea > > of auxiliary bus is separate existing device to sub-devices, while every > > such sub-device is controlled through relevant subsystem. Current > > implementation challenges this assumption by inventing own logic. > > 2. devm_* interfaces. It is bad. Please don't use them in such a complex > > driver. > > 3. Listen to PCI BOUND_DRIVER event can't be correct either. > > > > Thanks > > Hi Leon, > > Thanks for your comments. I’ll respond to 1 and 3 together. > > > 1. Use of bus_register_notifier to communicate between auxiliary devices. > > 3. Listen to PCI BOUND_DRIVER event can't be correct either > > We’re only using the notifier for the core driver to know when to create and > destroy auxiliary devices, not for communicate between auxiliary devices or > drivers – I agree, that would be ugly. > > We want to create the auxiliary device after a VF PCI device is bound to a > driver (BUS_NOTIFY_BOUND_DRIVER), and remove that auxiliary device just > before a VF is unbound from its PCI driver (BUS_NOTIFY_UNBIND_DRIVER); > bus_notify_register gives us access to these messages. I believe this is > not too far off from other examples that can be seen in > vfio/pci/vfio_pci_core, net/ethernet/ibm/emac, and net/mctp. > > Early on we were creating and deleting the auxiliary devices at the same > time as calling sriov_enable() and sriov_disable() but found that when the > user was doing unbind/bind operations we could end up with in-operative > clients and occasional kernel panics because the devices and drivers were > out-of-sync. Listening for the bind/unbind events allows the pds_core > driver to make sure the auxiliary devices serving each of the VF drivers are > kept in sync with the state of the VF PCI drivers. Auxiliary devices are intended to statically re-partition existing physical devices. You are not supposed to create such devices on the fly. So once you create VF physical device, it should already have aux device created too. This is traditional driver device model which you should follow and not reinvent your own schema, just to overcome bugs. Additionally, it is unclear how much we should invest in this review given the fact that pds VFIO series was NACKed. > > > > 2. devm_* interfaces > > Can you elaborate a bit on why you see using the devm interfaces as bad? I > don’t see the code complexity being any different than using the non-devm > interfaces, and these give the added protection of making sure to not leak > resources on driver removals, whether clean or on error. We are using these > allocations for longer term driver structures, not for ephemeral short-term > use buffers or fast-path operations, so the overhead should remain minimal. > It seems to me this is the advertised use as described in the devres notes > under Documentation. We had a very interesting talk in last LPC and overall agreement across various subsystem maintainers was to stay away from devm_* interfaces. https://lpc.events/event/16/contributions/1227/ We prefer explicit nature of kzalloc/kfree instead of implicit approach of devm_kzalloc. Thanks > > Thanks, > sln > >
On 2/20/23 10:53 PM, Leon Romanovsky wrote: > On Mon, Feb 20, 2023 at 03:53:02PM -0800, Shannon Nelson wrote: >> On 2/19/23 2:11 AM, Leon Romanovsky wrote: >>> On Fri, Feb 17, 2023 at 02:55:44PM -0800, Shannon Nelson wrote: >>>> Summary: >>>> -------- >>>> This patchset implements new driver for use with the AMD/Pensando >>>> Distributed Services Card (DSC), intended to provide core configuration >>>> services through the auxiliary_bus for VFio and vDPA feature specific >>>> drivers. >>> >>> Hi, >>> >>> I didn't look very deeply to this series, but three things caught my >>> attention and IMHO they need to be changed/redesinged before someone >>> can consider to merge it. >>> >>> 1. Use of bus_register_notifier to communicate between auxiliary devices. >>> This whole concept makes aux logic in this driver looks horrid. The idea >>> of auxiliary bus is separate existing device to sub-devices, while every >>> such sub-device is controlled through relevant subsystem. Current >>> implementation challenges this assumption by inventing own logic. >>> 2. devm_* interfaces. It is bad. Please don't use them in such a complex >>> driver. >>> 3. Listen to PCI BOUND_DRIVER event can't be correct either. >>> >>> Thanks >> >> Hi Leon, >> >> Thanks for your comments. I’ll respond to 1 and 3 together. >> >>> 1. Use of bus_register_notifier to communicate between auxiliary devices. >>> 3. Listen to PCI BOUND_DRIVER event can't be correct either >> >> We’re only using the notifier for the core driver to know when to create and >> destroy auxiliary devices, not for communicate between auxiliary devices or >> drivers – I agree, that would be ugly. >> >> We want to create the auxiliary device after a VF PCI device is bound to a >> driver (BUS_NOTIFY_BOUND_DRIVER), and remove that auxiliary device just >> before a VF is unbound from its PCI driver (BUS_NOTIFY_UNBIND_DRIVER); >> bus_notify_register gives us access to these messages. I believe this is >> not too far off from other examples that can be seen in >> vfio/pci/vfio_pci_core, net/ethernet/ibm/emac, and net/mctp. >> >> Early on we were creating and deleting the auxiliary devices at the same >> time as calling sriov_enable() and sriov_disable() but found that when the >> user was doing unbind/bind operations we could end up with in-operative >> clients and occasional kernel panics because the devices and drivers were >> out-of-sync. Listening for the bind/unbind events allows the pds_core >> driver to make sure the auxiliary devices serving each of the VF drivers are >> kept in sync with the state of the VF PCI drivers. > > Auxiliary devices are intended to statically re-partition existing physical devices. > You are not supposed to create such devices on the fly. So once you create VF physical > device, it should already have aux device created too. > > This is traditional driver device model which you should follow and not > reinvent your own schema, just to overcome bugs. Not so much a “bug”, I think, as an incomplete model which we addressed by using a hotplug model with the aux-devices. We felt we were following a part of the device model, but perhaps from the wrong angle: hot-plugging the aux-devices, which worked nicely to simplify the aux-driver/pci-driver relationship. However, I think the hole we fell into was expecting the client to be tied to a pci device on the other end, and this shouldn’t be a factor in the core’s management of the aux device. So, the aux-device built by the core needs to remain the constant, which is what we originally had: the aux devices created at pci_enable_sriov() time and torn down at pci_disable_sriov(). It’s easy enough to go back to that model, and that makes sense. Meanwhile, for those clients that do rely on the existence of a pci device, would you see that as a proper use of a bus listener to have the aux-driver subscribe and listen for its bind/unbind events? > > Additionally, it is unclear how much we should invest in this review > given the fact that pds VFIO series was NACKed. We haven’t given up on that one yet, and we have a vDPA client in mind as well possibly one or two others. > >> >> >>> 2. devm_* interfaces >> >> Can you elaborate a bit on why you see using the devm interfaces as bad? I >> don’t see the code complexity being any different than using the non-devm >> interfaces, and these give the added protection of making sure to not leak >> resources on driver removals, whether clean or on error. We are using these >> allocations for longer term driver structures, not for ephemeral short-term >> use buffers or fast-path operations, so the overhead should remain minimal. >> It seems to me this is the advertised use as described in the devres notes >> under Documentation. > > We had a very interesting talk in last LPC and overall agreement across > various subsystem maintainers was to stay away from devm_* interfaces. > https://lpc.events/event/16/contributions/1227/ > > We prefer explicit nature of kzalloc/kfree instead of implicit approach > of devm_kzalloc. Interesting – thanks, I missed that presentation. Sure, we can pull back on this. Thanks for your comments. sln
On Tue, Feb 21, 2023 at 02:03:24PM -0800, Shannon Nelson wrote: > On 2/20/23 10:53 PM, Leon Romanovsky wrote: > > On Mon, Feb 20, 2023 at 03:53:02PM -0800, Shannon Nelson wrote: > > > On 2/19/23 2:11 AM, Leon Romanovsky wrote: > > > > On Fri, Feb 17, 2023 at 02:55:44PM -0800, Shannon Nelson wrote: > > > > > Summary: > > > > > -------- > > > > > This patchset implements new driver for use with the AMD/Pensando > > > > > Distributed Services Card (DSC), intended to provide core configuration > > > > > services through the auxiliary_bus for VFio and vDPA feature specific > > > > > drivers. > > > > > > > > Hi, > > > > > > > > I didn't look very deeply to this series, but three things caught my > > > > attention and IMHO they need to be changed/redesinged before someone > > > > can consider to merge it. > > > > > > > > 1. Use of bus_register_notifier to communicate between auxiliary devices. > > > > This whole concept makes aux logic in this driver looks horrid. The idea > > > > of auxiliary bus is separate existing device to sub-devices, while every > > > > such sub-device is controlled through relevant subsystem. Current > > > > implementation challenges this assumption by inventing own logic. > > > > 2. devm_* interfaces. It is bad. Please don't use them in such a complex > > > > driver. > > > > 3. Listen to PCI BOUND_DRIVER event can't be correct either. > > > > > > > > Thanks > > > > > > Hi Leon, > > > > > > Thanks for your comments. I’ll respond to 1 and 3 together. > > > > > > > 1. Use of bus_register_notifier to communicate between auxiliary devices. > > > > 3. Listen to PCI BOUND_DRIVER event can't be correct either > > > > > > We’re only using the notifier for the core driver to know when to create and > > > destroy auxiliary devices, not for communicate between auxiliary devices or > > > drivers – I agree, that would be ugly. > > > > > > We want to create the auxiliary device after a VF PCI device is bound to a > > > driver (BUS_NOTIFY_BOUND_DRIVER), and remove that auxiliary device just > > > before a VF is unbound from its PCI driver (BUS_NOTIFY_UNBIND_DRIVER); > > > bus_notify_register gives us access to these messages. I believe this is > > > not too far off from other examples that can be seen in > > > vfio/pci/vfio_pci_core, net/ethernet/ibm/emac, and net/mctp. > > > > > > Early on we were creating and deleting the auxiliary devices at the same > > > time as calling sriov_enable() and sriov_disable() but found that when the > > > user was doing unbind/bind operations we could end up with in-operative > > > clients and occasional kernel panics because the devices and drivers were > > > out-of-sync. Listening for the bind/unbind events allows the pds_core > > > driver to make sure the auxiliary devices serving each of the VF drivers are > > > kept in sync with the state of the VF PCI drivers. > > > > Auxiliary devices are intended to statically re-partition existing physical devices. > > You are not supposed to create such devices on the fly. So once you create VF physical > > device, it should already have aux device created too. > > > > This is traditional driver device model which you should follow and not > > reinvent your own schema, just to overcome bugs. > > Not so much a “bug”, I think, as an incomplete model which we addressed by > using a hotplug model with the aux-devices. We felt we were following a > part of the device model, but perhaps from the wrong angle: hot-plugging the > aux-devices, which worked nicely to simplify the aux-driver/pci-driver > relationship. By looking (very briefly) on your pds_core and pds_vfio code, your notification scheme worked by chance as it lacks proper PCI and driver locks. You probably didn't stress enough PCI PF teardown. > However, I think the hole we fell into was expecting the > client to be tied to a pci device on the other end, and this shouldn’t be a > factor in the core’s management of the aux device. > > So, the aux-device built by the core needs to remain the constant, which is > what we originally had: the aux devices created at pci_enable_sriov() time > and torn down at pci_disable_sriov(). It’s easy enough to go back to that > model, and that makes sense. Not really, aux device creation is supposed to be in .probe() call of your PCI function, whenever it is PF or VF. After call to pci_enable_sriov(), the HW will add VFs, which will cause to PCI core to attempt and load pds_core .probe() callback. > > Meanwhile, for those clients that do rely on the existence of a pci device, > would you see that as a proper use of a bus listener to have the aux-driver > subscribe and listen for its bind/unbind events? No, these events must go. > > > > > > Additionally, it is unclear how much we should invest in this review > > given the fact that pds VFIO series was NACKed. > > We haven’t given up on that one yet, and we have a vDPA client in mind as > well possibly one or two others. I don't know about your plans and judge by what I saw in ML, VFIO was NACKed for second time already on the same ground. Thanks > > > > > > > > > > > > > > 2. devm_* interfaces > > > > > > Can you elaborate a bit on why you see using the devm interfaces as bad? I > > > don’t see the code complexity being any different than using the non-devm > > > interfaces, and these give the added protection of making sure to not leak > > > resources on driver removals, whether clean or on error. We are using these > > > allocations for longer term driver structures, not for ephemeral short-term > > > use buffers or fast-path operations, so the overhead should remain minimal. > > > It seems to me this is the advertised use as described in the devres notes > > > under Documentation. > > > > We had a very interesting talk in last LPC and overall agreement across > > various subsystem maintainers was to stay away from devm_* interfaces. > > https://lpc.events/event/16/contributions/1227/ > > > > We prefer explicit nature of kzalloc/kfree instead of implicit approach > > of devm_kzalloc. > > Interesting – thanks, I missed that presentation. Sure, we can pull back on > this. > > Thanks for your comments. > > sln >
On 2/17/23 2:55 PM, Shannon Nelson wrote: > Summary: > -------- > This patchset implements new driver for use with the AMD/Pensando > Distributed Services Card (DSC), intended to provide core configuration > services through the auxiliary_bus for VFio and vDPA feature specific > drivers. Thank you all for your responses and commentary on this initial review of our PDS drivers, we appreciate the constructive input. Obviously, the organization of our driver parts, PCI vs auxiliary and PF vs VF, doesn’t match what most folks were expecting to see. The primary issue seems to be that the modules responsible for the VF’s functions (e.g. the VFio or vDPA modules) are set up to be both PCI and auxiliary drivers, which doesn’t match other designs, and presents potential coordination/locking concerns. The expectation seems to be that the one Core module should handle both the VF and PF PCI driver work and from there set up the needed auxiliary devices, and things like vDPA modules should just be auxiliary drivers on one end and supply their respective features on the other end. Meanwhile, because the nature of VFio is to be a PCI driver, pds_vfio needs to have its aux stuff removed and needs to access the pds_core adminq directly by getting a pointer to the PF’s data, as done in other similar drivers. We think this reorganization of parts will resolve the complexity, bind/unbind, and most other points of discussion. I expect to get a first pass at this re-org done over the next few days and have an RFC for viewing sometime in the next week. Cheers, sln
Summary: -------- This patchset implements new driver for use with the AMD/Pensando Distributed Services Card (DSC), intended to provide core configuration services through the auxiliary_bus for VFio and vDPA feature specific drivers. To keep this patchset to a manageable size, the pds_vdpa and pds_vfio drivers have been split out into their own patchsets to be reviewed separately. Detail: ------- AMD/Pensando is making available a new set of devices for supporting vDPA, VFio, and potentially other features in the Distributed Services Card (DSC). These features are implemented through a PF that serves as a Core device for controlling and configuring its VF devices. These VF devices have separate drivers that use the auxiliary_bus to work through the Core device as the control path. Currently, the DSC supports standard ethernet operations using the ionic driver. This is not replaced by the Core-based devices - these new devices are in addition to the existing Ethernet device. Typical DSC configurations will include both PDS devices and Ionic Eth devices. The Core device is a new PCI PF device managed by a new driver 'pds_core'. It sets up auxiliary_bus devices for each VF for communicating with the drivers for the VF devices. The VFs may be for VFio or vDPA, and other services in the future; these VF types are selected as part of the DSC internal FW configurations, which is out of the scope of this patchset. The Core device sets up devlink parameters for enabling available feature sets. Once a feature set is enabled in the core device, auxiliary_bus devices are created for each VF that supports the feature. These auxiliary_bus devices are named by their feature plus VF PCI bdf so that the auxiliary device driver can find its related VF PCI driver instance. The VF's driver then connects to and uses this auxiliary_device to do control path configuration of the feature through the PF device. A cheap ASCII diagram of a vDPA instance looks something like this and can then be used with the vdpa kernel module to provide devices for virtio_vdpa kernel module for host interfaces, or vhost_vdpa kernel module for interfaces exported into your favorite VM. ,----------. | vdpa | '----------' | vdpa_dev ctl data | || pds_core.vDPA.2305 <---+ | || | | | || netdev | | | || | | | | || .------------. .------------. | pds_core | | pds_vdpa | '------------' '------------' || || 09:00.0 09:00.1 == PCI ========================================================= || || .----------. .----------. ,-----| PF |-----------| VF |-------, | '----------' -----------' | | DSC | | | ------------------------------------------------- Changes: v3: - changed names from "pensando" to "amd" and updated copyright strings - dropped the DEVLINK_PARAM_GENERIC_ID_FW_BANK for future development - changed the auxiliary device creation to be triggered by the PCI bus event BOUND_DRIVER, and torn down at UNBIND_DRIVER in order to properly handle users using the sysfs bind/unbind functions - dropped some noisy log messages - rebased to current net-next RFC to v2: https://lore.kernel.org/netdev/20221207004443.33779-1-shannon.nelson@amd.com/ - added separate devlink param patches for DEVLINK_PARAM_GENERIC_ID_ENABLE_MIGRATION and DEVLINK_PARAM_GENERIC_ID_FW_BANK, and dropped the driver specific implementations - updated descriptions for the new devlink parameters - dropped netdev support - dropped vDPA patches, will followup later - separated fw update and fw bank select into their own patches RFC: https://lore.kernel.org/netdev/20221118225656.48309-1-snelson@pensando.io/ Shannon Nelson (14): devlink: add enable_migration parameter pds_core: initial framework for pds_core driver pds_core: add devcmd device interfaces pds_core: health timer and workqueue pds_core: set up device and adminq pds_core: Add adminq processing and commands pds_core: add FW update feature to devlink pds_core: set up the VIF definitions and defaults pds_core: initial VF configuration pds_core: add auxiliary_bus devices pds_core: devlink params for enabling VIF support pds_core: add the aux client API pds_core: publish events to the clients pds_core: Kconfig and pds_core.rst .../device_drivers/ethernet/amd/pds_core.rst | 150 ++++ .../device_drivers/ethernet/index.rst | 1 + .../networking/devlink/devlink-params.rst | 3 + drivers/net/ethernet/amd/Kconfig | 12 + drivers/net/ethernet/amd/Makefile | 1 + drivers/net/ethernet/amd/pds_core/Makefile | 14 + drivers/net/ethernet/amd/pds_core/adminq.c | 299 ++++++++ drivers/net/ethernet/amd/pds_core/auxbus.c | 303 +++++++++ drivers/net/ethernet/amd/pds_core/core.c | 599 ++++++++++++++++ drivers/net/ethernet/amd/pds_core/core.h | 318 +++++++++ drivers/net/ethernet/amd/pds_core/debugfs.c | 262 +++++++ drivers/net/ethernet/amd/pds_core/dev.c | 358 ++++++++++ drivers/net/ethernet/amd/pds_core/devlink.c | 199 ++++++ drivers/net/ethernet/amd/pds_core/fw.c | 192 ++++++ drivers/net/ethernet/amd/pds_core/main.c | 435 ++++++++++++ include/linux/pds/pds_adminq.h | 643 ++++++++++++++++++ include/linux/pds/pds_auxbus.h | 85 +++ include/linux/pds/pds_common.h | 93 +++ include/linux/pds/pds_core_if.h | 580 ++++++++++++++++ include/linux/pds/pds_intr.h | 161 +++++ include/net/devlink.h | 4 + net/devlink/leftover.c | 5 + 22 files changed, 4717 insertions(+) create mode 100644 Documentation/networking/device_drivers/ethernet/amd/pds_core.rst create mode 100644 drivers/net/ethernet/amd/pds_core/Makefile create mode 100644 drivers/net/ethernet/amd/pds_core/adminq.c create mode 100644 drivers/net/ethernet/amd/pds_core/auxbus.c create mode 100644 drivers/net/ethernet/amd/pds_core/core.c create mode 100644 drivers/net/ethernet/amd/pds_core/core.h create mode 100644 drivers/net/ethernet/amd/pds_core/debugfs.c create mode 100644 drivers/net/ethernet/amd/pds_core/dev.c create mode 100644 drivers/net/ethernet/amd/pds_core/devlink.c create mode 100644 drivers/net/ethernet/amd/pds_core/fw.c create mode 100644 drivers/net/ethernet/amd/pds_core/main.c create mode 100644 include/linux/pds/pds_adminq.h create mode 100644 include/linux/pds/pds_auxbus.h create mode 100644 include/linux/pds/pds_common.h create mode 100644 include/linux/pds/pds_core_if.h create mode 100644 include/linux/pds/pds_intr.h