Re: [PATCH v2 2/4] cxl/pci: Define a common function get_cxl_devstate()

[Ira Weiny <ira.weiny@intel.com>]

Smita Koralahalli wrote:
> Refactor computation of cxlds to a common function get_cxl_devstate().
> 
> The above function could then be reused in both FW-First Component and
> Protocol error reporting and handling.
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
> v2:
> 	Refactor before adding trace support.
> 	get_cxl_dev() -> get_cxl_devstate().
> 	Cleaned up get_cxl_devstate().
> ---
>  drivers/cxl/pci.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 37164174b5fb..915102f5113f 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1021,32 +1021,38 @@ static struct pci_driver cxl_pci_driver = {
>  	},
>  };
>  
> +static struct cxl_dev_state *get_cxl_devstate(u16 segment, u8 bus,
> +					      u8 device, u8 function)
> +{
> +	unsigned int devfn = PCI_DEVFN(device, function);
> +	struct pci_dev *pdev __free(pci_dev_put) =
> +		pci_get_domain_bus_and_slot(segment, bus, devfn);

This is a good cleanup.  The previous code should not have declared pdev
NULL.  However...

> +
> +	if (!pdev)
> +		return NULL;
> +
> +	guard(device)(&pdev->dev);
> +	if (pdev->driver != &cxl_pci_driver)
> +		return NULL;
> +
> +	return pci_get_drvdata(pdev);

The device lock is now dropped before the tracing is completed.  For this
simple code I'm not keen on having the lock be taken in the helper and
released later.  It seems best to just open code this in each caller.

Ira