Re: [PATCH v6 02/11] driver core: Add dma_cleanup callback in bus_type

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Feb 22, 2022 at 10:58:37AM +0000, Robin Murphy wrote:
> On 2022-02-21 23:48, Jason Gunthorpe wrote:
> > On Mon, Feb 21, 2022 at 08:43:33PM +0000, Robin Murphy wrote:
> > > On 2022-02-19 07:32, Christoph Hellwig wrote:
> > > > So we are back to the callback madness instead of the nice and simple
> > > > flag?  Sigh.
> > > 
> > > TBH, I *think* this part could be a fair bit simpler. It looks like this
> > > whole callback mess is effectively just to decrement
> > > group->owner_cnt, but
> > 
> > Right, the new callback is because of Greg's push to put all the work
> > into the existing bus callback. Having symetrical callbacks is
> > cleaner.
> 
> I'll continue to disagree that having tons more code purely for the sake of
> it is cleaner. The high-level requirements are fundamentally asymmetrical -
> ownership has to be actively claimed by the bus code at a point during probe
> where it can block probing if necessary, but it can be released anywhere at
> all during remove since that cannot fail. I don't personally see the value
> in a bunch of code bloat for no reason other than trying to pretend that an
> asymmetrical thing isn't.

Then we should put this in the share core code like most of us want.

If we are doing this distorted thing then it may as well make some
kind of self consistent sense with a configure/unconfigure op pair.

> group->owner?  Walking the list would only have to be done for *releasing*
> ownership and I'm pretty sure all the races there are benign - only
> probe/remove of the driver (or DMA API token) matching a current non-NULL
> owner matter; if two removes race, the first might end up releasing
> ownership "early", but the second is waiting to do that anyway so it's OK;
> if a remove races with a probe, the remove may end up leaving the owner set,
> but the probe is waiting to do that anyway so it's OK.

With a lockless algorithm the race is probably wrongly releasing an
ownership that probe just set in the multi-device group case.

Still not sure I see what you are thinking though..

How did we get from adding a few simple lines to dd.c into building
some complex lockless algorithm and hoping we did it right?

> > > It has to be s It should be pretty straightforward for
> > > iommu_bus_notifier to clear group->owner automatically upon an
> > > unbind of the matching driver when it's no longer bound to any other
> > > devices in the group either.
> > 
> > That not_bound/unbind notifier isn't currently triggred during
> > necessary failure paths of really_probe().
> 
> Eh? Just look at the context of patch #2, let alone the rest of the
> function, and tell me how, if we can't rely on BUS_NOTIFY_DRIVER_NOT_BOUND,
> calling .dma_cleanup *from the exact same place* is somehow more reliable?

Yeah, OK

> AFAICS, a notifier handling both BUS_NOTIFY_UNBOUND_DRIVER and
> BUS_NOTIFY_DRIVER_NOT_BOUND would be directly equivalent to the callers of
> .dma_cleanup here.

Yes, but why hide this in a notifier, it is still spaghetti

> > > use-case) then it should be up to VFIO to decide when it's finally
> > > finished with the whole group, rather than pretending we can keep
> > > track of nested ownership claims from inside the API.
> > 
> > What nesting?
> 
> The current implementation of iommu_group_claim_dma_owner() allows owner_cnt
> to increase beyond 1, and correspondingly requires
> iommu_group_release_dma_owner() to be called the same number of times. It
> doesn't appear that VFIO needs that, and I'm not sure I'd trust any other
> potential users to get it right either.

That isn't for "nesting" it is keeping track of multi-device
groups. Each count represents a device, not a nest.

> > > FWIW I have some ideas for re-converging .dma_configure in future
> > > which I think should probably be able to subsume this into a
> > > completely generic common path, given a common flag.
> > 
> > This would be great!
> 
> Indeed, so if we're enthusiastic about future cleanup that necessitates a
> generic flag, why not make the flag generic to start with?

Maybe when someone has patches to delete the bus ops completely they
can convince Greg. The good news is that it isn't much work to flip
the flag, Lu has already done it 3 times in the previous versions..

It has already been 8 weeks on this point, lets just move on please.

Jason