Re: [PATCH v2 06/10] PCI/DOE: Allow mailbox creation without devres management

[Lukas Wunner <lukas@wunner.de>]

On Tue, Jan 24, 2023 at 12:15:43PM +0000, Jonathan Cameron wrote:
> On Mon, 23 Jan 2023 11:16:00 +0100 Lukas Wunner <lukas@wunner.de> wrote:
> > DOE mailbox creation is currently only possible through a devres-managed
> > API.  The lifetime of mailboxes thus ends with driver unbinding.
> > 
> > An upcoming commit will create DOE mailboxes upon device enumeration by
> > the PCI core.  Their lifetime shall not be limited by a driver.
> > 
> > Therefore rework pcim_doe_create_mb() into the non-devres-managed
> > pci_doe_create_mb().  Add pci_doe_destroy_mb() for mailbox destruction
> > on device removal.
[...]
> I'd like to understand why flushing in the tear down
> can't always be done as that makes the code more complex.

After sending out v2, I realized I had made a mistake:

In v2, on device removal I canceled any ongoing DOE exchanges
and declared the DOE mailbox dead before unbinding the driver
from a device.  That's the right thing to do for surprise removal
because you don't want to wait for an ongoing exchange to time out.

However we also have a code path for orderly device removal,
either via sysfs or by pressing the Attention Button (if present).
In that case, it should be legal for the driver to still perform
DOE exchanges in its ->remove() hook.

So in v3 I've changed the behavior to only cancel requests on
surprise removal.  By doing so, I was able to always flush on
mailbox destruction, as you've requested, and thereby simplify
the error paths.


> > +err_flush:
> > +	pci_doe_flush_mb(doe_mb);
> > +	xa_destroy(&doe_mb->prots);
> 
> Why the reorder wrt to the original devm managed cleanup?
> I'd expect this to happen on any error path after the xa_init.
> 
> It doesn't matter in practice because there isn't anything to
> do until after pci_doe_cache_protocols though.

Right, it's unnecessary to call xa_destroy() if
alloc_ordered_workqueue() failed because the xarray is still empty
at that point.  It doesn't need to be destroyed until it's been
populated by pci_doe_cache_protocols().

I've amended the commit message to explain that, but otherwise
did not change the code in v3.  Let me know if you have any
objections or feel strongly about moving xa_init().

Thanks,

Lukas