| Message ID | 20240809093442.646545-4-yanfei.xu@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for hdm docoder initialization from DVSEC ranges | expand |
Yanfei Xu wrote: > The right way is to checking Mem_info_valid bit for each applicable > DVSEC range against HDM_COUNT, not only for the DVSEC range 1. Also > the functions to check the Mem_info_valid bit are repeatedly > implemented, drop the rough one. Again, not a fix, as there is no evidence of a device in the wild that initializes memory_info_valid of range1 on a different timescale than range2. A better description of this patch is something like: "commit ce17ad0d5498 ("cxl: Wait Memory_Info_Valid before access memory related info") added another implementation of waiting for memory_info_valid without realizing it duplicated wait_for_valid()" ...I would also *only* do that cleanup and save changing the logic about when it is called to a separate patch. Neither of those would be marked as a fix because it depends on odd device behavior for it to be a problem in practice.
On 8/10/2024 3:13 AM, Dan Williams wrote: > Yanfei Xu wrote: >> The right way is to checking Mem_info_valid bit for each applicable >> DVSEC range against HDM_COUNT, not only for the DVSEC range 1. Also >> the functions to check the Mem_info_valid bit are repeatedly >> implemented, drop the rough one. > > Again, not a fix, as there is no evidence of a device in the wild that > initializes memory_info_valid of range1 on a different timescale than > range2. > > A better description of this patch is something like: > > "commit ce17ad0d5498 ("cxl: Wait Memory_Info_Valid before access memory > related info") added another implementation of waiting for > memory_info_valid without realizing it duplicated wait_for_valid()" > > ...I would also *only* do that cleanup and save changing the logic about > when it is called to a separate patch. Neither of those would be marked > as a fix because it depends on odd device behavior for it to be a > problem in practice. OK. Will split this patch into two and drop the "Fixes" Thanks, Yanfei
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 0915fc9e6d70..e822cc9ce315 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -211,37 +211,6 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds) } EXPORT_SYMBOL_NS_GPL(cxl_await_media_ready, CXL); -static int wait_for_valid(struct pci_dev *pdev, int d) -{ - u32 val; - int rc; - - /* - * Memory_Info_Valid: When set, indicates that the CXL Range 1 Size high - * and Size Low registers are valid. Must be set within 1 second of - * deassertion of reset to CXL device. Likely it is already set by the - * time this runs, but otherwise give a 1.5 second timeout in case of - * clock skew. - */ - rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val); - if (rc) - return rc; - - if (val & CXL_DVSEC_MEM_INFO_VALID) - return 0; - - msleep(1500); - - rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val); - if (rc) - return rc; - - if (val & CXL_DVSEC_MEM_INFO_VALID) - return 0; - - return -ETIMEDOUT; -} - static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val) { struct pci_dev *pdev = to_pci_dev(cxlds->dev); @@ -356,12 +325,6 @@ int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, if (!hdm_count || hdm_count > 2) return -EINVAL; - rc = wait_for_valid(pdev, d); - if (rc) { - dev_dbg(dev, "Failure awaiting MEM_INFO_VALID (%d)\n", rc); - return rc; - } - root = to_cxl_port(port->dev.parent); while (!is_cxl_root(root) && is_cxl_port(root->dev.parent)) root = to_cxl_port(root->dev.parent); @@ -389,6 +352,10 @@ int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, u64 base, size; u32 temp; + rc = cxl_dvsec_mem_range_valid(cxlds, i); + if (rc) + return rc; + rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_HIGH(i), &temp); if (rc)
The right way is to checking Mem_info_valid bit for each applicable DVSEC range against HDM_COUNT, not only for the DVSEC range 1. Also the functions to check the Mem_info_valid bit are repeatedly implemented, drop the rough one. Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info") Signed-off-by: Yanfei Xu <yanfei.xu@intel.com> --- drivers/cxl/core/pci.c | 41 ++++------------------------------------- 1 file changed, 4 insertions(+), 37 deletions(-)