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

[Alison Schofield <alison.schofield@intel.com>]

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
>