linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>,
	Linux PCI <linux-pci@vger.kernel.org>,
	"Ben Widawsky" <ben.widawsky@intel.com>,
	Bjorn Helgaas <helgaas@kernel.org>,
	"Chris Browy" <cbrowy@avery-design.com>,
	Linux ACPI <linux-acpi@vger.kernel.org>,
	"Schofield, Alison" <alison.schofield@intel.com>,
	Vishal L Verma <vishal.l.verma@intel.com>,
	"Weiny, Ira" <ira.weiny@intel.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	Linuxarm <linuxarm@huawei.com>, Fangjian <f.fangjian@huawei.com>
Subject: Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange
Date: Tue, 16 Mar 2021 16:57:48 +0000	[thread overview]
Message-ID: <20210316165748.00003f26@Huawei.com> (raw)
In-Reply-To: <20210316162952.00001ab7@Huawei.com>

On Tue, 16 Mar 2021 16:29:52 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:


> >   
> > > +               [0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, 0001) |
> > > +               FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, 0),
> > > +               [1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),
> > > +               [2] = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index)
> > > +       };
> > > +       u32 response[3];
> > > +       int ret;
> > > +
> > > +       ret = pcie_doe_exchange(doe, request, sizeof(request), response, sizeof(response));
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response[2]);
> > > +       *protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response[2]);
> > > +       *index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response[2]);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +/**
> > > + * pcie_doe_protocol_check() - check if this DOE mailbox supports specific protocol
> > > + * @doe: DOE state structure
> > > + * @vid: Vendor ID
> > > + * @protocol: Protocol number as defined by Vendor
> > > + * Returns: 0 on success, <0 on error
> > > + */
> > > +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol)    
> > 
> > Not clear to me that this is a comfortable API for a driver. I would
> > expect that at registration time all the supported protocols would be
> > retrieved and cached in the 'struct pcie_doe' context and then the
> > host driver could query from there without going back to the device
> > again.  
> 
> I'm not sure I follow.
> 
> Any driver will fall into one of the following categories:
> a) Already knows what protocols are available on a
>    given DOE instance perhaps because that's a characteristic of the hardware
>    supported, in which case it has no reason to check (unless driver writer
>    is paranoid)
> b) It has no way to know (e.g. class driver), then it makes sense to query
>    the DOE instance to find out what protocols are available.
> 
> Absolutely we could cache them, but it wouldn't change the interface
> presented to the driver. I think doing so at this stage is
> premature optimization.
> 
> We could present this at a different level and wrap it up as a
> find_doe that will return a DOE instance that supports the desired
> protocol, but then that puts the burden of reference counting etc
> for the different DOE instances on the core - the one thing I think
> we want to avoid.
> 
> So far we have no evidence any device will actually need that.
> 
> Of the existing protocols, only a few are allowed to coexist with
> each other and in well defined sets (CMA and IDE for example).
> 
> An alternative model we could look at (which is much more complex)
> is to have something like the following: 
> 
> struct pcie_doe_set - Central location which is responsible for
> all DOE mailboxes on a PCI device.
> 
> At init that scans all DOE mailboxes and builds a look up table
> from [vid, protocol] to [struct pcie_doe]
> Note this is 1 to 1, so if a protocol is supported on multiple
> mailboxes we use the first one.
> 
> pcie_doe_exchange(struct pcie_doe_set, u16 vid, u8 protocol...)
> Looks up the relevant DOE instance and does exchange on that.
> 
> So far I'm not convinced we should engage in this complexity.
> Nothing stops us adding it if and when it becomes apparent we
> actually need it.
> 
> An intermediate point would be to add basic list and reference
> counting infrastructure so that a driver can call
> 
> struct pcie_doe *pcie_doe_get(struct pci_dev, u16 vid, u8 protocol)
> void pci_doe_put(struct pci_doe *doe);
> 
> That means at least a list_head and possibly a lock being added
> to pci_dev. Not sure how Bjorn will feel about that.
> 
> I might see how bad this looks for v2.

Lifetime element of the DOE could be avoided by simply having

pcie_doe_register_all()
and
pcie_doe_unregister_all()

and so managing all DOE instances in one unit.

I'm not sure I like it, but certainly makes things simple.
After pcie_doe_register_all() call, all DOEs are ready to use
and we can have simple pcie_doe_find() to get one with
appropriate protocols.  There is never any need to specifically
release it because they are all cleaned up together in remove
/release path.

I'll put this together for a v2 and we can see how it shapes
up.

Jonathan



  reply	other threads:[~2021-03-16 16:59 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-10 18:03 [RFC PATCH 0/2] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
2021-03-10 18:03 ` [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange Jonathan Cameron
2021-03-15 19:45   ` Dan Williams
2021-03-16 16:29     ` Jonathan Cameron
2021-03-16 16:57       ` Jonathan Cameron [this message]
2021-03-16 18:14       ` Dan Williams
2021-03-16 23:26         ` Chris Browy
2021-03-18  1:30           ` Dan Williams
2021-03-18 14:25             ` Jonathan Cameron
2021-06-17 17:12               ` Jonathan Cameron
2021-06-17 19:48                 ` Chris Browy
2021-03-23 18:22         ` Jonathan Cameron
2021-03-23 18:57           ` Dan Williams
2021-03-10 18:03 ` [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE Jonathan Cameron
2021-03-10 18:14   ` Jonathan Cameron
2021-03-10 22:59     ` Chris Browy
2021-03-15 22:00     ` Dan Williams
2021-03-16 10:36       ` Jonathan Cameron
2021-03-17  1:42         ` Dan Williams
2021-03-17  1:55   ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210316165748.00003f26@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=alison.schofield@intel.com \
    --cc=ben.widawsky@intel.com \
    --cc=cbrowy@avery-design.com \
    --cc=dan.j.williams@intel.com \
    --cc=f.fangjian@huawei.com \
    --cc=helgaas@kernel.org \
    --cc=ira.weiny@intel.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=vishal.l.verma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).