[PATCH] cxl/core/pci: Move reading of control register to immediately before usage

[PATCH] cxl/core/pci: Move reading of control register to immediately before usage

[Foryun Ma]

Relocate the reading of the DVSEC control register to immediately
before usage and avoid unnecessary PCI config access from the read
if DVSEC capability check, hdm_count check, or device validity check
results in failure.

Signed-off-by: Foryun Ma <foryun.ma@jaguarmicro.com>
---
 drivers/cxl/core/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 8567dd11eaac..627be83881e9 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -338,10 +338,6 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 	if (rc)
 		return rc;
 
-	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
-	if (rc)
-		return rc;
-
 	if (!(cap & CXL_DVSEC_MEM_CAPABLE)) {
 		dev_dbg(dev, "Not MEM Capable\n");
 		return -ENXIO;
@@ -363,6 +359,10 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 		return rc;
 	}
 
+	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
+	if (rc)
+		return rc;
+
 	/*
 	 * The current DVSEC values are moot if the memory capability is
 	 * disabled, and they will remain moot after the HDM Decoder
-- 
2.34.1

Re: [PATCH] cxl/core/pci: Move reading of control register to immediately before usage

[Alison Schofield]

On Thu, May 30, 2024 at 09:32:16AM +0800, Foryun Ma wrote:
> Relocate the reading of the DVSEC control register to immediately
> before usage and avoid unnecessary PCI config access from the read
> if DVSEC capability check, hdm_count check, or device validity check
> results in failure.
> 

Thanks again. I like DaveJ's suggested wording better than what I
offered.

Please follow-up with:
- needs to be [PATCH v2], a changelog, and recipients list update.

These are described in the kernel documentation for submitting patches [1]
Check out the section of Responding to Reviews also.

FWIW - here's a sample usage that gets the recipient list for a HEAD commit: 
$ git show HEAD | perl scripts/get_maintainer.pl --nogit-fallback --no-rolestats

[1] https://docs.kernel.org/process/submitting-patches.html

> Signed-off-by: Foryun Ma <foryun.ma@jaguarmicro.com>
> ---
>  drivers/cxl/core/pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 8567dd11eaac..627be83881e9 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -338,10 +338,6 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
>  	if (rc)
>  		return rc;
>  
> -	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
> -	if (rc)
> -		return rc;
> -
>  	if (!(cap & CXL_DVSEC_MEM_CAPABLE)) {
>  		dev_dbg(dev, "Not MEM Capable\n");
>  		return -ENXIO;
> @@ -363,6 +359,10 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
>  		return rc;
>  	}
>  
> +	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
> +	if (rc)
> +		return rc;
> +
>  	/*
>  	 * The current DVSEC values are moot if the memory capability is
>  	 * disabled, and they will remain moot after the HDM Decoder
> -- 
> 2.34.1
> 
> 

[PATCH v2] cxl/core/pci: Move reading of control register to immediately before usage

[Foryun Ma]

Relocate the reading of the DVSEC control register to immediately
before usage and avoid unnecessary PCI config access from the read
if DVSEC capability check, hdm_count check, or device validity check
results in failure.

Signed-off-by: Foryun Ma <foryun.ma@jaguarmicro.com>

V1->V2: change the subject and commit log, suggested by dave.jiang
	and add change log, suggested by Alison Schofield
---
 drivers/cxl/core/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 8567dd11eaac..627be83881e9 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -338,10 +338,6 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 	if (rc)
 		return rc;
 
-	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
-	if (rc)
-		return rc;
-
 	if (!(cap & CXL_DVSEC_MEM_CAPABLE)) {
 		dev_dbg(dev, "Not MEM Capable\n");
 		return -ENXIO;
@@ -363,6 +359,10 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 		return rc;
 	}
 
+	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
+	if (rc)
+		return rc;
+
 	/*
 	 * The current DVSEC values are moot if the memory capability is
 	 * disabled, and they will remain moot after the HDM Decoder
-- 
2.34.1

Re: [PATCH v2] cxl/core/pci: Move reading of control register to immediately before usage

[Dan Williams]

Foryun Ma wrote:
> Relocate the reading of the DVSEC control register to immediately
> before usage and avoid unnecessary PCI config access from the read
> if DVSEC capability check, hdm_count check, or device validity check
> results in failure.
> 
> Signed-off-by: Foryun Ma <foryun.ma@jaguarmicro.com>
> 
> V1->V2: change the subject and commit log, suggested by dave.jiang
> 	and add change log, suggested by Alison Schofield

I guess this does no harm, but it seems not worth the effort to post and
review a patch for the given justification. PCI config cycles are cheap,
and all of the noted failure conditions are unlikely.

At the same time, I admit it is nice to see new contributors get to
the point of being able to send valid / well-formed patches.

However, going forward, if a patch has no end user visible side effects,
and does not improve the readability of the code (i.e. is not a
cleanup), then it is probably best not to send the patch and add to the
review backlog.

Re: [PATCH v2] cxl/core/pci: Move reading of control register to immediately before usage

[Alison Schofield]

On Thu, May 30, 2024 at 02:47:51PM +0800, Foryun Ma wrote:
> Relocate the reading of the DVSEC control register to immediately
> before usage and avoid unnecessary PCI config access from the read
> if DVSEC capability check, hdm_count check, or device validity check
> results in failure.
> 
> Signed-off-by: Foryun Ma <foryun.ma@jaguarmicro.com>
> 
> V1->V2: change the subject and commit log, suggested by dave.jiang
> 	and add change log, suggested by Alison Schofield
> ---

I see the value in this patch as a cleanup because despite what a
reader well versed in this area might know (PCI config cycles are
cheap and these failure conditions are unlikely) the code breaks
a basic pattern - read then check, don't read, then check later.
You calling attention to it makes all of us better reviewers, and
if we don't fix it now we may see this patch again from another 
reader.

If you want to continue the submittal, please do:
- incorporate my feedback from v1: move the check to after the comment
- Post a v3 as a new message, not in reply to this message.

With that, I'll offer my Reviewed-by tag and this can be considered
for upstream merge.

-- Alison







>  drivers/cxl/core/pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 8567dd11eaac..627be83881e9 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -338,10 +338,6 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
>  	if (rc)
>  		return rc;
>  
> -	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
> -	if (rc)
> -		return rc;
> -
>  	if (!(cap & CXL_DVSEC_MEM_CAPABLE)) {
>  		dev_dbg(dev, "Not MEM Capable\n");
>  		return -ENXIO;
> @@ -363,6 +359,10 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
>  		return rc;
>  	}
>  
> +	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
> +	if (rc)
> +		return rc;
> +
>  	/*
>  	 * The current DVSEC values are moot if the memory capability is
>  	 * disabled, and they will remain moot after the HDM Decoder
> -- 
> 2.34.1
>