| Message ID | 168193567959.1178687.13133878561024203176.stgit@djiang5-mobl3 |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Add support for QTG ID retrieval for CXL subsystem | expand |
On Wed, 19 Apr 2023 13:21:19 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Move read_cdat_data() from endpoint probe to general port probe to > allow reading of CDAT data for CXL switches as well as CXL device. > Add wrapper support for cxl_test to bypass the cdat reading. > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> It might be worth a wrapper at somepoint for that dance from port to the PCI device with the actual DOE etc. Such a wrapp would provide somewhere to add a bit of documentation on why the uport might be a platform device (memX) or might be a PCI device thus explaining the two different way it is handled. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > v4: > - Remove cxl_test wrapper. (Ira) > --- > drivers/cxl/core/pci.c | 20 +++++++++++++++----- > drivers/cxl/port.c | 6 +++--- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 9c7e2f69d9ca..1c415b26e866 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -546,16 +546,26 @@ static unsigned char cdat_checksum(void *buf, size_t size) > */ > void read_cdat_data(struct cxl_port *port) > { > - struct pci_doe_mb *cdat_doe; > - struct device *dev = &port->dev; > struct device *uport = port->uport; > - struct cxl_memdev *cxlmd = to_cxl_memdev(uport); > - struct cxl_dev_state *cxlds = cxlmd->cxlds; > - struct pci_dev *pdev = to_pci_dev(cxlds->dev); > + struct device *dev = &port->dev; > + struct cxl_dev_state *cxlds; > + struct pci_doe_mb *cdat_doe; > + struct cxl_memdev *cxlmd; > + struct pci_dev *pdev; > size_t cdat_length; > void *cdat_table; > int rc; > > + if (is_cxl_memdev(uport)) { > + cxlmd = to_cxl_memdev(uport); > + cxlds = cxlmd->cxlds; > + pdev = to_pci_dev(cxlds->dev); > + } else if (dev_is_pci(uport)) { > + pdev = to_pci_dev(uport); > + } else { > + return; > + } > + > cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL, > CXL_DOE_PROTOCOL_TABLE_ACCESS); > if (!cdat_doe) { > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > index 22a7ab2bae7c..615e0ef6b440 100644 > --- a/drivers/cxl/port.c > +++ b/drivers/cxl/port.c > @@ -93,9 +93,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) > if (IS_ERR(cxlhdm)) > return PTR_ERR(cxlhdm); > > - /* Cache the data early to ensure is_visible() works */ > - read_cdat_data(port); > - > get_device(&cxlmd->dev); > rc = devm_add_action_or_reset(&port->dev, schedule_detach, cxlmd); > if (rc) > @@ -135,6 +132,9 @@ static int cxl_port_probe(struct device *dev) > { > struct cxl_port *port = to_cxl_port(dev); > > + /* Cache the data early to ensure is_visible() works */ > + read_cdat_data(port); > + > if (is_cxl_endpoint(port)) > return cxl_endpoint_port_probe(port); > return cxl_switch_port_probe(port); > >
Dave Jiang wrote: > Move read_cdat_data() from endpoint probe to general port probe to > allow reading of CDAT data for CXL switches as well as CXL device. > Add wrapper support for cxl_test to bypass the cdat reading. > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > v4: > - Remove cxl_test wrapper. (Ira) > --- > drivers/cxl/core/pci.c | 20 +++++++++++++++----- > drivers/cxl/port.c | 6 +++--- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 9c7e2f69d9ca..1c415b26e866 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -546,16 +546,26 @@ static unsigned char cdat_checksum(void *buf, size_t size) > */ > void read_cdat_data(struct cxl_port *port) > { > - struct pci_doe_mb *cdat_doe; > - struct device *dev = &port->dev; > struct device *uport = port->uport; > - struct cxl_memdev *cxlmd = to_cxl_memdev(uport); > - struct cxl_dev_state *cxlds = cxlmd->cxlds; > - struct pci_dev *pdev = to_pci_dev(cxlds->dev); > + struct device *dev = &port->dev; > + struct cxl_dev_state *cxlds; > + struct pci_doe_mb *cdat_doe; > + struct cxl_memdev *cxlmd; > + struct pci_dev *pdev; > size_t cdat_length; > void *cdat_table; > int rc; > > + if (is_cxl_memdev(uport)) { > + cxlmd = to_cxl_memdev(uport); > + cxlds = cxlmd->cxlds; > + pdev = to_pci_dev(cxlds->dev); Per this fix [1], there's no need to reference cxlds, the parent of the memory device is the device this wants, and needs to be careful that not all 'struct cxl_memdev' instances are hosted by pci devices. [1]: http://lore.kernel.org/r/168213190748.708404.16215095414060364800.stgit@dwillia2-xfh.jf.intel.com Otherwise, looks good to me.
On 4/24/23 3:08 PM, Dan Williams wrote: > Dave Jiang wrote: >> Move read_cdat_data() from endpoint probe to general port probe to >> allow reading of CDAT data for CXL switches as well as CXL device. >> Add wrapper support for cxl_test to bypass the cdat reading. >> >> Reviewed-by: Ira Weiny <ira.weiny@intel.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> >> --- >> v4: >> - Remove cxl_test wrapper. (Ira) >> --- >> drivers/cxl/core/pci.c | 20 +++++++++++++++----- >> drivers/cxl/port.c | 6 +++--- >> 2 files changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >> index 9c7e2f69d9ca..1c415b26e866 100644 >> --- a/drivers/cxl/core/pci.c >> +++ b/drivers/cxl/core/pci.c >> @@ -546,16 +546,26 @@ static unsigned char cdat_checksum(void *buf, size_t size) >> */ >> void read_cdat_data(struct cxl_port *port) >> { >> - struct pci_doe_mb *cdat_doe; >> - struct device *dev = &port->dev; >> struct device *uport = port->uport; >> - struct cxl_memdev *cxlmd = to_cxl_memdev(uport); >> - struct cxl_dev_state *cxlds = cxlmd->cxlds; >> - struct pci_dev *pdev = to_pci_dev(cxlds->dev); >> + struct device *dev = &port->dev; >> + struct cxl_dev_state *cxlds; >> + struct pci_doe_mb *cdat_doe; >> + struct cxl_memdev *cxlmd; >> + struct pci_dev *pdev; >> size_t cdat_length; >> void *cdat_table; >> int rc; >> >> + if (is_cxl_memdev(uport)) { >> + cxlmd = to_cxl_memdev(uport); >> + cxlds = cxlmd->cxlds; >> + pdev = to_pci_dev(cxlds->dev); > > Per this fix [1], there's no need to reference cxlds, the parent of the > memory device is the device this wants, and needs to be careful that not > all 'struct cxl_memdev' instances are hosted by pci devices. > > [1]: http://lore.kernel.org/r/168213190748.708404.16215095414060364800.stgit@dwillia2-xfh.jf.intel.com Ok will pull this fix patch in and rebase against that. > > Otherwise, looks good to me.
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 9c7e2f69d9ca..1c415b26e866 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -546,16 +546,26 @@ static unsigned char cdat_checksum(void *buf, size_t size) */ void read_cdat_data(struct cxl_port *port) { - struct pci_doe_mb *cdat_doe; - struct device *dev = &port->dev; struct device *uport = port->uport; - struct cxl_memdev *cxlmd = to_cxl_memdev(uport); - struct cxl_dev_state *cxlds = cxlmd->cxlds; - struct pci_dev *pdev = to_pci_dev(cxlds->dev); + struct device *dev = &port->dev; + struct cxl_dev_state *cxlds; + struct pci_doe_mb *cdat_doe; + struct cxl_memdev *cxlmd; + struct pci_dev *pdev; size_t cdat_length; void *cdat_table; int rc; + if (is_cxl_memdev(uport)) { + cxlmd = to_cxl_memdev(uport); + cxlds = cxlmd->cxlds; + pdev = to_pci_dev(cxlds->dev); + } else if (dev_is_pci(uport)) { + pdev = to_pci_dev(uport); + } else { + return; + } + cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DOE_PROTOCOL_TABLE_ACCESS); if (!cdat_doe) { diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c index 22a7ab2bae7c..615e0ef6b440 100644 --- a/drivers/cxl/port.c +++ b/drivers/cxl/port.c @@ -93,9 +93,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) if (IS_ERR(cxlhdm)) return PTR_ERR(cxlhdm); - /* Cache the data early to ensure is_visible() works */ - read_cdat_data(port); - get_device(&cxlmd->dev); rc = devm_add_action_or_reset(&port->dev, schedule_detach, cxlmd); if (rc) @@ -135,6 +132,9 @@ static int cxl_port_probe(struct device *dev) { struct cxl_port *port = to_cxl_port(dev); + /* Cache the data early to ensure is_visible() works */ + read_cdat_data(port); + if (is_cxl_endpoint(port)) return cxl_endpoint_port_probe(port); return cxl_switch_port_probe(port);