RE: [PATCH 07/22] RDMA/irdma: Register an auxiliary driver and implement private channel OPs

["Saleem, Shiraz" <shiraz.saleem@intel.com>]

> Subject: Re: [PATCH 07/22] RDMA/irdma: Register an auxiliary driver and
> implement private channel OPs
> 
> On Mon, Feb 01, 2021 at 05:06:58PM -0800, Dan Williams wrote:
> > On Mon, Feb 1, 2021 at 4:40 PM Saleem, Shiraz <shiraz.saleem@intel.com>
> wrote:
> > >
> > > > Subject: Re: [PATCH 07/22] RDMA/irdma: Register an auxiliary
> > > > driver and implement private channel OPs
> > > >
> > > > On Sat, Jan 30, 2021 at 01:19:36AM +0000, Saleem, Shiraz wrote:
> > > > > > Subject: Re: [PATCH 07/22] RDMA/irdma: Register an auxiliary
> > > > > > driver and implement private channel OPs
> > > > > >
> > > > > > On Wed, Jan 27, 2021 at 07:16:41PM -0400, Jason Gunthorpe wrote:
> > > > > > > On Wed, Jan 27, 2021 at 10:17:56PM +0000, Saleem, Shiraz wrote:
> > > > > > >
> > > > > > > > Even with another core PCI driver, there still needs to be
> > > > > > > > private communication channel between the aux rdma driver
> > > > > > > > and this PCI driver to pass things like QoS updates.
> > > > > > >
> > > > > > > Data pushed from the core driver to its aux drivers should
> > > > > > > either be done through new callbacks in a struct
> > > > > > > device_driver or by having a notifier chain scheme from the core
> driver.
> > > > > >
> > > > > > Right, and internal to driver/core device_lock will protect
> > > > > > from parallel probe/remove and PCI flows.
> > > > > >
> > > > >
> > > > > OK. We will hold the device_lock while issuing the .ops
> > > > > callbacks from core
> > > > driver.
> > > > > This should solve our synchronization issue.
> > > > >
> > > > > There have been a few discussions in this thread. And I would
> > > > > like to be clear on what to do.
> > > > >
> > > > > So we will,
> > > > >
> > > > > 1. Remove .open/.close, .peer_register/.peer_unregister 2.
> > > > > Protect ops callbacks issued from core driver to the aux driver
> > > > > with device_lock
> > > >
> > > > A notifier chain is probably better, honestly.
> > > >
> > > > Especially since you don't want to split the netdev side, a
> > > > notifier chain can be used by both cases equally.
> > > >
> > >
> > > The device_lock seems to be a simple solution to this synchronization
> problem.
> > > May I ask what makes the notifier scheme better to solve this?
> > >
> >
> > Only loosely following the arguments here, but one of the requirements
> > of the driver-op scheme is that the notifying agent needs to know the
> > target device. With the notifier-chain approach the target device
> > becomes anonymous to the notifier agent.
> 
> Yes, and you need to have an aux device in the first place. The netdev side has
> neither of this things. 

But we do. The ice PCI driver is thing spawning the aux device. And we are trying to do
something directed here specifically between the ice PCI driver and the irdma aux driver.
Seems the notifier chain approach, from the comment above, is less directed and when
you want to broadcast events from core driver to multiple registered subscribers.

I think there is going to be need for some ops even if we were to use notifier chains.
Such as ones that need a ret_code.



I think it would be a bit odd to have extensive callbacks that
> are for RDMA only, that suggests something in the core API is not general enough.
> 

Yes there are some domain specific ops. But it is within the boundary of how the aux bus should be used no? 

https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html
"The auxiliary_driver can also be encapsulated inside custom drivers that make the core device's functionality extensible by adding additional domain-specific ops as follows:"

struct my_driver {
        struct auxiliary_driver auxiliary_drv;
        const struct my_ops ops;
};