| Message ID | 20250219062832.237881-2-dave@stgolabs.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Dirty shutdown followups | expand |
On 2/18/25 11:28 PM, Davidlohr Bueso wrote: > Add a helper to fetch the port/device GPF dvsecs. This is > currently only used for ports, but a later patch to export > dirty count to users will make use of the device one. > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > drivers/cxl/core/pci.c | 38 ++++++++++++++++++++++++++++---------- > drivers/cxl/cxl.h | 2 ++ > 2 files changed, 30 insertions(+), 10 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index a5c65f79db18..2226cca3382d 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -1072,6 +1072,27 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c) > #define GPF_TIMEOUT_BASE_MAX 2 > #define GPF_TIMEOUT_SCALE_MAX 7 /* 10 seconds */ > > +int cxl_gpf_get_dvsec(struct device *dev, bool port) Maybe consider enum instead of bool. That would make it more readable. if not, maybe rename the bool to is_port > +{ > + struct pci_dev *pdev; > + int dvsec; > + > + if (!dev_is_pci(dev)) > + return -EINVAL; Since this function is mostly a wrapper for pci_find_dvsec_capability(), why not have it return type to be u16 and just return 0 here? That way when you check you only need to verify if it's 0 (failed). > + > + pdev = to_pci_dev(dev); > + if (!pdev) > + return -EINVAL; No need to check here. to_pci_dev() does not return a NULL. > + > + dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL, > + port ? CXL_DVSEC_PORT_GPF : CXL_DVSEC_DEVICE_GPF); > + if (!dvsec) > + pci_warn(pdev, "%s GPF DVSEC not present\n", why not just dev_warn() since this is cxl code and not PCI core > + port ? "Port" : "Device"); > + return dvsec; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_gpf_get_dvsec, "CXL"); > + > static int update_gpf_port_dvsec(struct pci_dev *pdev, int dvsec, int phase) > { > u64 base, scale; > @@ -1116,26 +1137,23 @@ int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port) > { > struct pci_dev *pdev; > > - if (!dev_is_pci(dport_dev)) > - return 0; > - > - pdev = to_pci_dev(dport_dev); > - if (!pdev || !port) > + if (!port) > return -EINVAL; > > if (!port->gpf_dvsec) { > int dvsec; > > - dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL, > - CXL_DVSEC_PORT_GPF); > - if (!dvsec) { > - pci_warn(pdev, "Port GPF DVSEC not present\n"); > + dvsec = cxl_gpf_get_dvsec(dport_dev, true); > + if (dvsec <= 0) > return -EINVAL; > - } > > port->gpf_dvsec = dvsec; > } > > + pdev = to_pci_dev(dport_dev); > + if (!pdev) > + return -EINVAL; No need to check here. macro does not return NULL. > + > update_gpf_port_dvsec(pdev, port->gpf_dvsec, 1); > update_gpf_port_dvsec(pdev, port->gpf_dvsec, 2); > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 6baec4ba9141..acbbba41356d 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -901,4 +901,6 @@ bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port); > #define __mock static > #endif > > +int cxl_gpf_get_dvsec(struct device *dev, bool port); > + > #endif /* __CXL_H__ */
On Wed, 19 Feb 2025, Dave Jiang wrote: >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >> index a5c65f79db18..2226cca3382d 100644 >> --- a/drivers/cxl/core/pci.c >> +++ b/drivers/cxl/core/pci.c >> @@ -1072,6 +1072,27 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c) >> #define GPF_TIMEOUT_BASE_MAX 2 >> #define GPF_TIMEOUT_SCALE_MAX 7 /* 10 seconds */ >> >> +int cxl_gpf_get_dvsec(struct device *dev, bool port) > >Maybe consider enum instead of bool. That would make it more readable. if not, maybe rename the bool to is_port > >> +{ >> + struct pci_dev *pdev; >> + int dvsec; >> + >> + if (!dev_is_pci(dev)) >> + return -EINVAL; > >Since this function is mostly a wrapper for pci_find_dvsec_capability(), why not have it return type to be u16 and just return 0 here? That way when you check you only need to verify if it's 0 (failed). I kind of wanted to not have a u16 return just because it was ugly for cxl general calls, but yeah it is better to follow the pci call itself, given the name of this cxl helper is *dvsec*.
On 2/19/2025 2:28 PM, Davidlohr Bueso wrote: > Add a helper to fetch the port/device GPF dvsecs. This is > currently only used for ports, but a later patch to export > dirty count to users will make use of the device one. > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > drivers/cxl/core/pci.c | 38 ++++++++++++++++++++++++++++---------- > drivers/cxl/cxl.h | 2 ++ > 2 files changed, 30 insertions(+), 10 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index a5c65f79db18..2226cca3382d 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -1072,6 +1072,27 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c) > #define GPF_TIMEOUT_BASE_MAX 2 > #define GPF_TIMEOUT_SCALE_MAX 7 /* 10 seconds */ > > +int cxl_gpf_get_dvsec(struct device *dev, bool port) > +{ > + struct pci_dev *pdev; > + int dvsec; > + > + if (!dev_is_pci(dev)) > + return -EINVAL; > + > + pdev = to_pci_dev(dev); > + if (!pdev) > + return -EINVAL; My understanding is checking the returned value of to_pci_dev() is not needed, to_pci_dev() is a container_of() macro. Besides, above already checks if the device is a PCI device. > + > + dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL, > + port ? CXL_DVSEC_PORT_GPF : CXL_DVSEC_DEVICE_GPF); > + if (!dvsec) > + pci_warn(pdev, "%s GPF DVSEC not present\n", > + port ? "Port" : "Device"); > + return dvsec; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_gpf_get_dvsec, "CXL"); > + > static int update_gpf_port_dvsec(struct pci_dev *pdev, int dvsec, int phase) > { > u64 base, scale; > @@ -1116,26 +1137,23 @@ int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port) > { > struct pci_dev *pdev; > > - if (!dev_is_pci(dport_dev)) > - return 0; > - > - pdev = to_pci_dev(dport_dev); > - if (!pdev || !port) > + if (!port) > return -EINVAL; > > if (!port->gpf_dvsec) { > int dvsec; > > - dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL, > - CXL_DVSEC_PORT_GPF); > - if (!dvsec) { > - pci_warn(pdev, "Port GPF DVSEC not present\n"); > + dvsec = cxl_gpf_get_dvsec(dport_dev, true); > + if (dvsec <= 0) > return -EINVAL; > - } > > port->gpf_dvsec = dvsec; > } > > + pdev = to_pci_dev(dport_dev); > + if (!pdev) > + return -EINVAL; Same as above. Other looks good to me, feel free to add Reviewed-by: Li Ming <ming.li@zohomail.com>
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index a5c65f79db18..2226cca3382d 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -1072,6 +1072,27 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c) #define GPF_TIMEOUT_BASE_MAX 2 #define GPF_TIMEOUT_SCALE_MAX 7 /* 10 seconds */ +int cxl_gpf_get_dvsec(struct device *dev, bool port) +{ + struct pci_dev *pdev; + int dvsec; + + if (!dev_is_pci(dev)) + return -EINVAL; + + pdev = to_pci_dev(dev); + if (!pdev) + return -EINVAL; + + dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL, + port ? CXL_DVSEC_PORT_GPF : CXL_DVSEC_DEVICE_GPF); + if (!dvsec) + pci_warn(pdev, "%s GPF DVSEC not present\n", + port ? "Port" : "Device"); + return dvsec; +} +EXPORT_SYMBOL_NS_GPL(cxl_gpf_get_dvsec, "CXL"); + static int update_gpf_port_dvsec(struct pci_dev *pdev, int dvsec, int phase) { u64 base, scale; @@ -1116,26 +1137,23 @@ int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port) { struct pci_dev *pdev; - if (!dev_is_pci(dport_dev)) - return 0; - - pdev = to_pci_dev(dport_dev); - if (!pdev || !port) + if (!port) return -EINVAL; if (!port->gpf_dvsec) { int dvsec; - dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL, - CXL_DVSEC_PORT_GPF); - if (!dvsec) { - pci_warn(pdev, "Port GPF DVSEC not present\n"); + dvsec = cxl_gpf_get_dvsec(dport_dev, true); + if (dvsec <= 0) return -EINVAL; - } port->gpf_dvsec = dvsec; } + pdev = to_pci_dev(dport_dev); + if (!pdev) + return -EINVAL; + update_gpf_port_dvsec(pdev, port->gpf_dvsec, 1); update_gpf_port_dvsec(pdev, port->gpf_dvsec, 2); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 6baec4ba9141..acbbba41356d 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -901,4 +901,6 @@ bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port); #define __mock static #endif +int cxl_gpf_get_dvsec(struct device *dev, bool port); + #endif /* __CXL_H__ */
Add a helper to fetch the port/device GPF dvsecs. This is currently only used for ports, but a later patch to export dirty count to users will make use of the device one. Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- drivers/cxl/core/pci.c | 38 ++++++++++++++++++++++++++++---------- drivers/cxl/cxl.h | 2 ++ 2 files changed, 30 insertions(+), 10 deletions(-)