Re: [PATCH v3 net-next 00/14] pds_core driver

[Shannon Nelson <shannon.nelson@amd.com>]

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