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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue,  4 Jun 2024 11:21:51 +0800
Foryun Ma <foryun.ma@jaguarmicro.com> 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.
I would focus more on the code readability / clarity improvement
Alison highlighted.
Something like:

"Move the read of the DVSEC control register to immediately before is
used to slightly improve readability. Also avoids reading this register
if an error condition is hit before it is needed."

With that as the focus I'm fine with this one as a small improvement.
Dave is handling the tree though, so ultimately his decision on this
one like any other patch.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> Signed-off-by: Foryun Ma <foryun.ma@jaguarmicro.com>
> 
> ---
> V2->V3: move the check to after the comment
> 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..a663e7566c48 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;
> @@ -368,6 +364,10 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
>  	 * disabled, and they will remain moot after the HDM Decoder
>  	 * capability is enabled.
>  	 */
> +	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
> +	if (rc)
> +		return rc;
> +
>  	info->mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
>  	if (!info->mem_enabled)
>  		return 0;