Received-SPF: pass (google.com: domain of linux-kernel+bounces-194068-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Wed, 29 May 2024 15:19:19 +0300 (EEST)
To: Alison Schofield <alison.schofield@intel.com>
cc: Davidlohr Bueso <dave@stgolabs.net>, 
    Jonathan Cameron <jonathan.cameron@huawei.com>, 
    Dave Jiang <dave.jiang@intel.com>, Vishal Verma <vishal.l.verma@intel.com>, 
    Ira Weiny <ira.weiny@intel.com>, Dan Williams <dan.j.williams@intel.com>, 
    Ben Widawsky <bwidawsk@kernel.org>, linux-cxl@vger.kernel.org, 
    LKML <linux-kernel@vger.kernel.org>, stable@vger.kernel.org
Subject: Re: [PATCH 1/1] cxl/pci: Convert PCIBIOS_* return codes to errnos
In-Reply-To: <ZlZYrsPfzAiFzNLM@aschofie-mobl2>
Message-ID: <78e5690b-832f-3da5-3500-141e9b155c09@linux.intel.com>
References: <20240527123403.13098-1-ilpo.jarvinen@linux.intel.com> <ZlZYrsPfzAiFzNLM@aschofie-mobl2>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; BOUNDARY="8323328-1595479625-1716984315=:1108"
Content-ID: <bd1af136-005a-4183-897f-28bf7f3ddd1b@linux.intel.com>

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-1595479625-1716984315=:1108
Content-Type: text/plain; CHARSET=ISO-8859-15
Content-Transfer-Encoding: QUOTED-PRINTABLE
Content-ID: <52392403-976d-88e3-05bc-7ad38cd8884a@linux.intel.com>

On Tue, 28 May 2024, Alison Schofield wrote:

> On Mon, May 27, 2024 at 03:34:02PM +0300, Ilpo J=E4rvinen wrote:
> > pci_{read,write}_config_*word() and pcie_capability_read_word() return
> > PCIBIOS_* codes, not usual errnos.
> >=20
> > 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.
>=20
>=20
> 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=20
returning PCIBIOS_* return codes that are > 0).  devm_cxl_enable_mem()=20
then tries to check for >0 and <0 but the <0 condition won't match=20
correctly because PCIBIOS_* is not <0 but >0, devm_cxl_enable_mem() then=20
return 0 where it should have returned an error.

The positive "error code" from wait_for_valid(), cxl_dvsec_rr_decode(),=20
and cxl_pci_ras_unmask leaks out of .probe().

--=20
 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=E4rvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  drivers/cxl/core/pci.c | 30 +++++++++++++++---------------
> >  drivers/cxl/pci.c      |  2 +-
> >  2 files changed, 16 insertions(+), 16 deletions(-)
> >=20
> > 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)
> >  =09=09=09=09=09   d + CXL_DVSEC_RANGE_SIZE_LOW(id),
> >  =09=09=09=09=09   &temp);
> >  =09=09if (rc)
> > -=09=09=09return rc;
> > +=09=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09=09valid =3D FIELD_GET(CXL_DVSEC_MEM_INFO_VALID, temp);
> >  =09=09if (valid)
> > @@ -155,7 +155,7 @@ static int cxl_dvsec_mem_range_active(struct cxl_de=
v_state *cxlds, int id)
> >  =09=09rc =3D pci_read_config_dword(
> >  =09=09=09pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(id), &temp);
> >  =09=09if (rc)
> > -=09=09=09return rc;
> > +=09=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09=09active =3D FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp);
> >  =09=09if (active)
> > @@ -188,7 +188,7 @@ int cxl_await_media_ready(struct cxl_dev_state *cxl=
ds)
> >  =09rc =3D pci_read_config_word(pdev,
> >  =09=09=09=09  d + CXL_DVSEC_CAP_OFFSET, &cap);
> >  =09if (rc)
> > -=09=09return rc;
> > +=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09hdm_count =3D FIELD_GET(CXL_DVSEC_HDM_COUNT_MASK, cap);
> >  =09for (i =3D 0; i < hdm_count; i++) {
> > @@ -225,7 +225,7 @@ static int wait_for_valid(struct pci_dev *pdev, int=
 d)
> >  =09 */
> >  =09rc =3D pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0),=
 &val);
> >  =09if (rc)
> > -=09=09return rc;
> > +=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09if (val & CXL_DVSEC_MEM_INFO_VALID)
> >  =09=09return 0;
> > @@ -234,7 +234,7 @@ static int wait_for_valid(struct pci_dev *pdev, int=
 d)
> > =20
> >  =09rc =3D pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0),=
 &val);
> >  =09if (rc)
> > -=09=09return rc;
> > +=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09if (val & CXL_DVSEC_MEM_INFO_VALID)
> >  =09=09return 0;
> > @@ -250,8 +250,8 @@ static int cxl_set_mem_enable(struct cxl_dev_state =
*cxlds, u16 val)
> >  =09int rc;
> > =20
> >  =09rc =3D pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl)=
;
> > -=09if (rc < 0)
> > -=09=09return rc;
> > +=09if (rc)
> > +=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09if ((ctrl & CXL_DVSEC_MEM_ENABLE) =3D=3D val)
> >  =09=09return 1;
> > @@ -259,8 +259,8 @@ static int cxl_set_mem_enable(struct cxl_dev_state =
*cxlds, u16 val)
> >  =09ctrl |=3D val;
> > =20
> >  =09rc =3D pci_write_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, ctrl)=
;
> > -=09if (rc < 0)
> > -=09=09return rc;
> > +=09if (rc)
> > +=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09return 0;
> >  }
> > @@ -336,11 +336,11 @@ int cxl_dvsec_rr_decode(struct device *dev, int d=
,
> > =20
> >  =09rc =3D pci_read_config_word(pdev, d + CXL_DVSEC_CAP_OFFSET, &cap);
> >  =09if (rc)
> > -=09=09return rc;
> > +=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09rc =3D pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl)=
;
> >  =09if (rc)
> > -=09=09return rc;
> > +=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09if (!(cap & CXL_DVSEC_MEM_CAPABLE)) {
> >  =09=09dev_dbg(dev, "Not MEM Capable\n");
> > @@ -379,14 +379,14 @@ int cxl_dvsec_rr_decode(struct device *dev, int d=
,
> >  =09=09rc =3D pci_read_config_dword(
> >  =09=09=09pdev, d + CXL_DVSEC_RANGE_SIZE_HIGH(i), &temp);
> >  =09=09if (rc)
> > -=09=09=09return rc;
> > +=09=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09=09size =3D (u64)temp << 32;
> > =20
> >  =09=09rc =3D pci_read_config_dword(
> >  =09=09=09pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp);
> >  =09=09if (rc)
> > -=09=09=09return rc;
> > +=09=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09=09size |=3D temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
> >  =09=09if (!size) {
> > @@ -400,14 +400,14 @@ int cxl_dvsec_rr_decode(struct device *dev, int d=
,
> >  =09=09rc =3D pci_read_config_dword(
> >  =09=09=09pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp);
> >  =09=09if (rc)
> > -=09=09=09return rc;
> > +=09=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09=09base =3D (u64)temp << 32;
> > =20
> >  =09=09rc =3D pci_read_config_dword(
> >  =09=09=09pdev, d + CXL_DVSEC_RANGE_BASE_LOW(i), &temp);
> >  =09=09if (rc)
> > -=09=09=09return rc;
> > +=09=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09=09base |=3D temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
> > =20
> > 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)
> > =20
> >  =09rc =3D pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap);
> >  =09if (rc)
> > -=09=09return rc;
> > +=09=09return pcibios_err_to_errno(rc);
> > =20
> >  =09if (cap & PCI_EXP_DEVCTL_URRE) {
> >  =09=09addr =3D cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_MASK_OFFSET;
> > --=20
> > 2.39.2
> >=20
>=20
--8323328-1595479625-1716984315=:1108--