pci_{read,write}_config_*word() and pcie_capability_read_word() return PCIBIOS_* codes, not usual errnos.
Fix return value checks to handle PCIBIOS_* return codes correctly by dropping < 0 from the check and convert the PCIBIOS_* return codes into errnos using pcibios_err_to_errno() before returning them.
Fixes: ce17ad0d5498 ("cxl: Wait Memory_Info_Valid before access memory related info") Fixes: 34e37b4c432c ("cxl/port: Enable HDM Capability after validating DVSEC Ranges") Fixes: 14d788740774 ("cxl/mem: Consolidate CXL DVSEC Range enumeration in the core") Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info") Cc: stable@vger.kernel.org Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- drivers/cxl/core/pci.c | 30 +++++++++++++++--------------- drivers/cxl/pci.c | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 8567dd11eaac..9ca67d4e0a89 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -121,7 +121,7 @@ static int cxl_dvsec_mem_range_valid(struct cxl_dev_state *cxlds, int id) d + CXL_DVSEC_RANGE_SIZE_LOW(id), &temp); if (rc) - return rc; + return pcibios_err_to_errno(rc);
valid = FIELD_GET(CXL_DVSEC_MEM_INFO_VALID, temp); if (valid) @@ -155,7 +155,7 @@ static int cxl_dvsec_mem_range_active(struct cxl_dev_state *cxlds, int id) rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(id), &temp); if (rc) - return rc; + return pcibios_err_to_errno(rc);
active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp); if (active) @@ -188,7 +188,7 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds) rc = pci_read_config_word(pdev, d + CXL_DVSEC_CAP_OFFSET, &cap); if (rc) - return rc; + return pcibios_err_to_errno(rc);
hdm_count = FIELD_GET(CXL_DVSEC_HDM_COUNT_MASK, cap); for (i = 0; i < hdm_count; i++) { @@ -225,7 +225,7 @@ static int wait_for_valid(struct pci_dev *pdev, int d) */ rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val); if (rc) - return rc; + return pcibios_err_to_errno(rc);
if (val & CXL_DVSEC_MEM_INFO_VALID) return 0; @@ -234,7 +234,7 @@ static int wait_for_valid(struct pci_dev *pdev, int d)
rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val); if (rc) - return rc; + return pcibios_err_to_errno(rc);
if (val & CXL_DVSEC_MEM_INFO_VALID) return 0; @@ -250,8 +250,8 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val) int rc;
rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl); - if (rc < 0) - return rc; + if (rc) + return pcibios_err_to_errno(rc);
if ((ctrl & CXL_DVSEC_MEM_ENABLE) == val) return 1; @@ -259,8 +259,8 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val) ctrl |= val;
rc = pci_write_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, ctrl); - if (rc < 0) - return rc; + if (rc) + return pcibios_err_to_errno(rc);
return 0; } @@ -336,11 +336,11 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
rc = pci_read_config_word(pdev, d + CXL_DVSEC_CAP_OFFSET, &cap); if (rc) - return rc; + return pcibios_err_to_errno(rc);
rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl); if (rc) - return rc; + return pcibios_err_to_errno(rc);
if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { dev_dbg(dev, "Not MEM Capable\n"); @@ -379,14 +379,14 @@ int cxl_dvsec_rr_decode(struct device *dev, int d, rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_HIGH(i), &temp); if (rc) - return rc; + return pcibios_err_to_errno(rc);
size = (u64)temp << 32;
rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp); if (rc) - return rc; + return pcibios_err_to_errno(rc);
size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK; if (!size) { @@ -400,14 +400,14 @@ int cxl_dvsec_rr_decode(struct device *dev, int d, rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp); if (rc) - return rc; + return pcibios_err_to_errno(rc);
base = (u64)temp << 32;
rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_BASE_LOW(i), &temp); if (rc) - return rc; + return pcibios_err_to_errno(rc);
base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index e53646e9f2fb..0ec9cbc64896 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -540,7 +540,7 @@ static int cxl_pci_ras_unmask(struct pci_dev *pdev)
rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap); if (rc) - return rc; + return pcibios_err_to_errno(rc);
if (cap & PCI_EXP_DEVCTL_URRE) { addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_MASK_OFFSET;
On Mon, May 27, 2024 at 03:34:02PM +0300, Ilpo Järvinen wrote:
pci_{read,write}_config_*word() and pcie_capability_read_word() return PCIBIOS_* codes, not usual errnos.
Fix return value checks to handle PCIBIOS_* return codes correctly by dropping < 0 from the check and convert the PCIBIOS_* return codes into errnos using pcibios_err_to_errno() before returning them.
Do we ever make a bad decision based on the wrong rc value or is this a correction to the emitted dev_*() messaging, or both?
-- Alison
Fixes: ce17ad0d5498 ("cxl: Wait Memory_Info_Valid before access memory related info") Fixes: 34e37b4c432c ("cxl/port: Enable HDM Capability after validating DVSEC Ranges") Fixes: 14d788740774 ("cxl/mem: Consolidate CXL DVSEC Range enumeration in the core") Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info") Cc: stable@vger.kernel.org Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
drivers/cxl/core/pci.c | 30 +++++++++++++++--------------- drivers/cxl/pci.c | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 8567dd11eaac..9ca67d4e0a89 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -121,7 +121,7 @@ static int cxl_dvsec_mem_range_valid(struct cxl_dev_state *cxlds, int id) d + CXL_DVSEC_RANGE_SIZE_LOW(id), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);valid = FIELD_GET(CXL_DVSEC_MEM_INFO_VALID, temp); if (valid) @@ -155,7 +155,7 @@ static int cxl_dvsec_mem_range_active(struct cxl_dev_state *cxlds, int id) rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(id), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp); if (active) @@ -188,7 +188,7 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds) rc = pci_read_config_word(pdev, d + CXL_DVSEC_CAP_OFFSET, &cap); if (rc)
return rc;
return pcibios_err_to_errno(rc);hdm_count = FIELD_GET(CXL_DVSEC_HDM_COUNT_MASK, cap); for (i = 0; i < hdm_count; i++) { @@ -225,7 +225,7 @@ static int wait_for_valid(struct pci_dev *pdev, int d) */ rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val); if (rc)
return rc;
return pcibios_err_to_errno(rc);if (val & CXL_DVSEC_MEM_INFO_VALID) return 0; @@ -234,7 +234,7 @@ static int wait_for_valid(struct pci_dev *pdev, int d) rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val); if (rc)
return rc;
return pcibios_err_to_errno(rc);if (val & CXL_DVSEC_MEM_INFO_VALID) return 0; @@ -250,8 +250,8 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val) int rc; rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
- if (rc < 0)
return rc;
- if (rc)
return pcibios_err_to_errno(rc);if ((ctrl & CXL_DVSEC_MEM_ENABLE) == val) return 1; @@ -259,8 +259,8 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val) ctrl |= val; rc = pci_write_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, ctrl);
- if (rc < 0)
return rc;
- if (rc)
return pcibios_err_to_errno(rc);return 0; } @@ -336,11 +336,11 @@ int cxl_dvsec_rr_decode(struct device *dev, int d, rc = pci_read_config_word(pdev, d + CXL_DVSEC_CAP_OFFSET, &cap); if (rc)
return rc;
return pcibios_err_to_errno(rc);rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl); if (rc)
return rc;
return pcibios_err_to_errno(rc);if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { dev_dbg(dev, "Not MEM Capable\n"); @@ -379,14 +379,14 @@ int cxl_dvsec_rr_decode(struct device *dev, int d, rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_HIGH(i), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);size = (u64)temp << 32; rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK; if (!size) { @@ -400,14 +400,14 @@ int cxl_dvsec_rr_decode(struct device *dev, int d, rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);base = (u64)temp << 32; rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_BASE_LOW(i), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK; diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index e53646e9f2fb..0ec9cbc64896 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -540,7 +540,7 @@ static int cxl_pci_ras_unmask(struct pci_dev *pdev) rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap); if (rc)
return rc;
return pcibios_err_to_errno(rc);if (cap & PCI_EXP_DEVCTL_URRE) { addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_MASK_OFFSET; -- 2.39.2
On Tue, 28 May 2024, Alison Schofield wrote:
On Mon, May 27, 2024 at 03:34:02PM +0300, Ilpo Järvinen wrote:
pci_{read,write}_config_*word() and pcie_capability_read_word() return PCIBIOS_* codes, not usual errnos.
Fix return value checks to handle PCIBIOS_* return codes correctly by dropping < 0 from the check and convert the PCIBIOS_* return codes into errnos using pcibios_err_to_errno() before returning them.
Do we ever make a bad decision based on the wrong rc value or is this a correction to the emitted dev_*() messaging, or both?
There is potential for bad decision.
Eg. cxl_set_mem_enable() it can return 0, 1 and rc that is currently returning PCIBIOS_* return codes that are > 0). devm_cxl_enable_mem() then tries to check for >0 and <0 but the <0 condition won't match correctly because PCIBIOS_* is not <0 but >0, devm_cxl_enable_mem() then return 0 where it should have returned an error.
The positive "error code" from wait_for_valid(), cxl_dvsec_rr_decode(), and cxl_pci_ras_unmask leaks out of .probe().
On Wed, May 29, 2024 at 03:19:19PM +0300, Ilpo Järvinen wrote:
On Tue, 28 May 2024, Alison Schofield wrote:
On Mon, May 27, 2024 at 03:34:02PM +0300, Ilpo Järvinen wrote:
pci_{read,write}_config_*word() and pcie_capability_read_word() return PCIBIOS_* codes, not usual errnos.
Fix return value checks to handle PCIBIOS_* return codes correctly by dropping < 0 from the check and convert the PCIBIOS_* return codes into errnos using pcibios_err_to_errno() before returning them.
Do we ever make a bad decision based on the wrong rc value or is this a correction to the emitted dev_*() messaging, or both?
There is potential for bad decision.
Eg. cxl_set_mem_enable() it can return 0, 1 and rc that is currently returning PCIBIOS_* return codes that are > 0). devm_cxl_enable_mem() then tries to check for >0 and <0 but the <0 condition won't match correctly because PCIBIOS_* is not <0 but >0, devm_cxl_enable_mem() then return 0 where it should have returned an error.
The positive "error code" from wait_for_valid(), cxl_dvsec_rr_decode(), and cxl_pci_ras_unmask leaks out of .probe().
Ah, I see the fix is quite tidy but the impact is more complex. Please update the commit log to explain user visible impacts of this fix.
-- i.
Fixes: ce17ad0d5498 ("cxl: Wait Memory_Info_Valid before access memory related info") Fixes: 34e37b4c432c ("cxl/port: Enable HDM Capability after validating DVSEC Ranges") Fixes: 14d788740774 ("cxl/mem: Consolidate CXL DVSEC Range enumeration in the core") Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info") Cc: stable@vger.kernel.org Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
drivers/cxl/core/pci.c | 30 +++++++++++++++--------------- drivers/cxl/pci.c | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 8567dd11eaac..9ca67d4e0a89 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -121,7 +121,7 @@ static int cxl_dvsec_mem_range_valid(struct cxl_dev_state *cxlds, int id) d + CXL_DVSEC_RANGE_SIZE_LOW(id), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);valid = FIELD_GET(CXL_DVSEC_MEM_INFO_VALID, temp); if (valid) @@ -155,7 +155,7 @@ static int cxl_dvsec_mem_range_active(struct cxl_dev_state *cxlds, int id) rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(id), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp); if (active) @@ -188,7 +188,7 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds) rc = pci_read_config_word(pdev, d + CXL_DVSEC_CAP_OFFSET, &cap); if (rc)
return rc;
return pcibios_err_to_errno(rc);hdm_count = FIELD_GET(CXL_DVSEC_HDM_COUNT_MASK, cap); for (i = 0; i < hdm_count; i++) { @@ -225,7 +225,7 @@ static int wait_for_valid(struct pci_dev *pdev, int d) */ rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val); if (rc)
return rc;
return pcibios_err_to_errno(rc);if (val & CXL_DVSEC_MEM_INFO_VALID) return 0; @@ -234,7 +234,7 @@ static int wait_for_valid(struct pci_dev *pdev, int d) rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val); if (rc)
return rc;
return pcibios_err_to_errno(rc);if (val & CXL_DVSEC_MEM_INFO_VALID) return 0; @@ -250,8 +250,8 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val) int rc; rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
- if (rc < 0)
return rc;
- if (rc)
return pcibios_err_to_errno(rc);if ((ctrl & CXL_DVSEC_MEM_ENABLE) == val) return 1; @@ -259,8 +259,8 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val) ctrl |= val; rc = pci_write_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, ctrl);
- if (rc < 0)
return rc;
- if (rc)
return pcibios_err_to_errno(rc);return 0; } @@ -336,11 +336,11 @@ int cxl_dvsec_rr_decode(struct device *dev, int d, rc = pci_read_config_word(pdev, d + CXL_DVSEC_CAP_OFFSET, &cap); if (rc)
return rc;
return pcibios_err_to_errno(rc);rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl); if (rc)
return rc;
return pcibios_err_to_errno(rc);if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { dev_dbg(dev, "Not MEM Capable\n"); @@ -379,14 +379,14 @@ int cxl_dvsec_rr_decode(struct device *dev, int d, rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_HIGH(i), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);size = (u64)temp << 32; rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK; if (!size) { @@ -400,14 +400,14 @@ int cxl_dvsec_rr_decode(struct device *dev, int d, rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);base = (u64)temp << 32; rc = pci_read_config_dword( pdev, d + CXL_DVSEC_RANGE_BASE_LOW(i), &temp); if (rc)
return rc;
return pcibios_err_to_errno(rc);base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK; diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index e53646e9f2fb..0ec9cbc64896 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -540,7 +540,7 @@ static int cxl_pci_ras_unmask(struct pci_dev *pdev) rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap); if (rc)
return rc;
return pcibios_err_to_errno(rc);if (cap & PCI_EXP_DEVCTL_URRE) { addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_MASK_OFFSET; -- 2.39.2
Ilpo Järvinen wrote:
pci_{read,write}_config_*word() and pcie_capability_read_word() return PCIBIOS_* codes, not usual errnos.
Fix return value checks to handle PCIBIOS_* return codes correctly by dropping < 0 from the check and convert the PCIBIOS_* return codes into errnos using pcibios_err_to_errno() before returning them.
Fixes: ce17ad0d5498 ("cxl: Wait Memory_Info_Valid before access memory related info") Fixes: 34e37b4c432c ("cxl/port: Enable HDM Capability after validating DVSEC Ranges") Fixes: 14d788740774 ("cxl/mem: Consolidate CXL DVSEC Range enumeration in the core") Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info") Cc: stable@vger.kernel.org Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
Reviewed-by: Ira Weiny ira.weiny@intel.com
[snip]
linux-stable-mirror@lists.linaro.org
-
Alison Schofield -
Ilpo Järvinen -
Ira Weiny