Re: [PATCH V14 3/7] PCI/DOE: Add DOE mailbox support functions

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 19 Jul 2022 12:16:06 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> On Tue, Jul 19, 2022 at 05:35:53PM +0100, Jonathan Cameron wrote:
> > On Thu, 14 Jul 2022 20:04:20 -0700
> > ira.weiny@intel.com wrote:
> >   
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > Introduced in a PCIe r6.0, sec 6.30, DOE provides a config space based
> > > mailbox with standard protocol discovery.  Each mailbox is accessed
> > > through a DOE Extended Capability.
> > > 
> > > Each DOE mailbox must support the DOE discovery protocol in addition to
> > > any number of additional protocols.
> > > 
> > > Define core PCIe functionality to manage a single PCIe DOE mailbox at a
> > > defined config space offset.  Functionality includes iterating,
> > > creating, query of supported protocol, and task submission.  Destruction
> > > of the mailboxes is device managed.
> > > 
> > > Cc: "Li, Ming" <ming4.li@intel.com>
> > > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > > Cc: Matthew Wilcox <willy@infradead.org>
> > > Acked-by: Bjorn Helgaas <helgaas@kernel.org>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>  
> > Hi Ira,
> > 
> > Thanks for persisting with this!
> > 
> > So, I think this works, but there is at least one 'sleep' I can't
> > see a purpose for.  I think it's just a left over from refactoring.
> > 
> > A few other more trivial things inline.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> >   
> > >   
> > >>  # Endpoint library must be initialized before its users  
> > >  obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
> > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > > new file mode 100644
> > > index 000000000000..12c3762be22f
> > > --- /dev/null
> > > +++ b/drivers/pci/doe.c
> > > @@ -0,0 +1,546 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Data Object Exchange
> > > + *	PCIe r6.0, sec 6.30 DOE
> > > + *
> > > + * Copyright (C) 2021 Huawei
> > > + *	Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > + *
> > > + * Copyright (C) 2022 Intel Corporation
> > > + *	Ira Weiny <ira.weiny@intel.com>
> > > + */
> > > +
> > > +#define dev_fmt(fmt) "DOE: " fmt
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/jiffies.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/pci-doe.h>
> > > +#include <linux/workqueue.h>
> > > +
> > > +#define PCI_DOE_PROTOCOL_DISCOVERY 0
> > > +
> > > +#define PCI_DOE_BUSY_MAX_RETRIES 16  
> > Left over from removed code.  
> 
> I think Dan may have taken these.  If so I'll send a clean up.  If not I can
> spin.  Let me check.
> 
Absolutely. All tiny improvements, so fine to go in next cycle given late timing.

> > > +/**
> > > + * pci_doe_supports_prot() - Return if the DOE instance supports the given
> > > + *			     protocol
> > > + * @doe_mb: DOE mailbox capability to query
> > > + * @vid: Protocol Vendor ID
> > > + * @type: Protocol type
> > > + *
> > > + * RETURNS: True if the DOE mailbox supports the protocol specified
> > > + */
> > > +bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
> > > +{
> > > +	unsigned long index;
> > > +	void *entry;
> > > +
> > > +	/* The discovery protocol must always be supported */
> > > +	if (vid == PCI_VENDOR_ID_PCI_SIG && type == PCI_DOE_PROTOCOL_DISCOVERY)
> > > +		return true;  
> > 
> > Given how cheap this look up is now it's all in xarray, we could drop this
> > 'optimization'.  I'm fairly sure the discovery protocol will always be
> > discovered (spec says it must be returned when calling itself as the fist
> > protocol).  
> 
> No we can't because this is called before the xarray is populated with the
> discovery protocol.  This was actually added not as an optimization but to
> allow the discovery protocol to run through the common query path.
> 

Ah.  I was too lazy to check if that was the case :)

Jonathan