diff mbox series

[4/4] cxl/pci: Define a common function get_cxl_dev()

Message ID 20240522150839.27578-5-Smita.KoralahalliChannabasappa@amd.com
State New, archived
Headers show
Series acpi/ghes, cper, cxl: Trace FW-First CXL Protocol Errors | expand

Commit Message

Smita Koralahalli May 22, 2024, 3:08 p.m. UTC
Refactor computation of cxlds to a common function get_cxl_dev() and reuse
the function in both cxl_handle_cper_event() and cxl_handle_prot_err().

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
 drivers/cxl/pci.c | 52 +++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

Comments

Dave Jiang May 22, 2024, 7:42 p.m. UTC | #1
On 5/22/24 8:08 AM, Smita Koralahalli wrote:
> Refactor computation of cxlds to a common function get_cxl_dev() and reuse
> the function in both cxl_handle_cper_event() and cxl_handle_prot_err().

I think just introduce the function where you open coded it instead of adding code and then deleting them in the same series.
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
>  drivers/cxl/pci.c | 52 +++++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 3e3c36983686..26e65e5b68cb 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -974,32 +974,43 @@ static struct pci_driver cxl_pci_driver = {
>  	},
>  };
>  
> +static struct cxl_dev_state *get_cxl_dev(u16 segment, u8 bus, u8 device,
> +					 u8 function)

get_cxlds() or get_cxl_device_state() would be better. 

DJ

> +{
> +	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> +	struct cxl_dev_state *cxlds;
> +	unsigned int devfn;
> +
> +	devfn = PCI_DEVFN(device, function);
> +	pdev = pci_get_domain_bus_and_slot(segment, bus, devfn);
> +
> +	if (!pdev)
> +		return NULL;
> +
> +	guard(device)(&pdev->dev);
> +	if (pdev->driver != &cxl_pci_driver)
> +		return NULL;
> +
> +	cxlds = pci_get_drvdata(pdev);
> +
> +	return cxlds;
> +}
> +
>  #define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
>  static void cxl_handle_cper_event(enum cxl_event_type ev_type,
>  				  struct cxl_cper_event_rec *rec)
>  {
>  	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> -	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>  	enum cxl_event_log_type log_type;
>  	struct cxl_dev_state *cxlds;
> -	unsigned int devfn;
>  	u32 hdr_flags;
>  
>  	pr_debug("CPER event %d for device %u:%u:%u.%u\n", ev_type,
>  		 device_id->segment_num, device_id->bus_num,
>  		 device_id->device_num, device_id->func_num);
>  
> -	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> -	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> -					   device_id->bus_num, devfn);
> -	if (!pdev)
> -		return;
> -
> -	guard(device)(&pdev->dev);
> -	if (pdev->driver != &cxl_pci_driver)
> -		return;
> -
> -	cxlds = pci_get_drvdata(pdev);
> +	cxlds = get_cxl_dev(device_id->segment_num, device_id->bus_num,
> +			    device_id->device_num, device_id->func_num);
>  	if (!cxlds)
>  		return;
>  
> @@ -1013,21 +1024,10 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type,
>  
>  static void cxl_handle_prot_err(struct cxl_cper_prot_err *p_err)
>  {
> -	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>  	struct cxl_dev_state *cxlds;
> -	unsigned int devfn;
>  
> -	devfn = PCI_DEVFN(p_err->device, p_err->function);
> -	pdev = pci_get_domain_bus_and_slot(p_err->segment,
> -					   p_err->bus, devfn);
> -	if (!pdev)
> -		return;
> -
> -	guard(device)(&pdev->dev);
> -	if (pdev->driver != &cxl_pci_driver)
> -		return;
> -
> -	cxlds = pci_get_drvdata(pdev);
> +	cxlds = get_cxl_dev(p_err->segment, p_err->bus,
> +			    p_err->device, p_err->function);
>  	if (!cxlds)
>  		return;
>
Alison Schofield May 23, 2024, 12:45 a.m. UTC | #2
On Wed, May 22, 2024 at 03:08:39PM +0000, Smita Koralahalli wrote:
> Refactor computation of cxlds to a common function get_cxl_dev() and reuse
> the function in both cxl_handle_cper_event() and cxl_handle_prot_err().
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
>  drivers/cxl/pci.c | 52 +++++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 3e3c36983686..26e65e5b68cb 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -974,32 +974,43 @@ static struct pci_driver cxl_pci_driver = {
>  	},
>  };
>  
> +static struct cxl_dev_state *get_cxl_dev(u16 segment, u8 bus, u8 device,
> +					 u8 function)
> +{
> +	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> +	struct cxl_dev_state *cxlds;
> +	unsigned int devfn;
> +
> +	devfn = PCI_DEVFN(device, function);
> +	pdev = pci_get_domain_bus_and_slot(segment, bus, devfn);
> +
> +	if (!pdev)
> +		return NULL;
> +
> +	guard(device)(&pdev->dev);
> +	if (pdev->driver != &cxl_pci_driver)
> +		return NULL;
> +
> +	cxlds = pci_get_drvdata(pdev);
> +
> +	return cxlds;

This can be:
	return pci_get_drvdata(pdev);


> +}

snip
Smita Koralahalli May 23, 2024, 9:37 p.m. UTC | #3
On 5/22/2024 12:42 PM, Dave Jiang wrote:
> 
> 
> On 5/22/24 8:08 AM, Smita Koralahalli wrote:
>> Refactor computation of cxlds to a common function get_cxl_dev() and reuse
>> the function in both cxl_handle_cper_event() and cxl_handle_prot_err().
> 
> I think just introduce the function where you open coded it instead of adding code and then deleting them in the same series.

Okay refactor first, then add trace support. Will change.

>>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>> ---
>>   drivers/cxl/pci.c | 52 +++++++++++++++++++++++------------------------
>>   1 file changed, 26 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index 3e3c36983686..26e65e5b68cb 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -974,32 +974,43 @@ static struct pci_driver cxl_pci_driver = {
>>   	},
>>   };
>>   
>> +static struct cxl_dev_state *get_cxl_dev(u16 segment, u8 bus, u8 device,
>> +					 u8 function)
> 
> get_cxlds() or get_cxl_device_state() would be better.

Okay will change
> 
> DJ
> 

Thanks
Smita

>> +{
>> +	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>> +	struct cxl_dev_state *cxlds;
>> +	unsigned int devfn;
>> +
>> +	devfn = PCI_DEVFN(device, function);
>> +	pdev = pci_get_domain_bus_and_slot(segment, bus, devfn);
>> +
>> +	if (!pdev)
>> +		return NULL;
>> +
>> +	guard(device)(&pdev->dev);
>> +	if (pdev->driver != &cxl_pci_driver)
>> +		return NULL;
>> +
>> +	cxlds = pci_get_drvdata(pdev);
>> +
>> +	return cxlds;
>> +}
>> +
>>   #define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
>>   static void cxl_handle_cper_event(enum cxl_event_type ev_type,
>>   				  struct cxl_cper_event_rec *rec)
>>   {
>>   	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
>> -	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>>   	enum cxl_event_log_type log_type;
>>   	struct cxl_dev_state *cxlds;
>> -	unsigned int devfn;
>>   	u32 hdr_flags;
>>   
>>   	pr_debug("CPER event %d for device %u:%u:%u.%u\n", ev_type,
>>   		 device_id->segment_num, device_id->bus_num,
>>   		 device_id->device_num, device_id->func_num);
>>   
>> -	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
>> -	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
>> -					   device_id->bus_num, devfn);
>> -	if (!pdev)
>> -		return;
>> -
>> -	guard(device)(&pdev->dev);
>> -	if (pdev->driver != &cxl_pci_driver)
>> -		return;
>> -
>> -	cxlds = pci_get_drvdata(pdev);
>> +	cxlds = get_cxl_dev(device_id->segment_num, device_id->bus_num,
>> +			    device_id->device_num, device_id->func_num);
>>   	if (!cxlds)
>>   		return;
>>   
>> @@ -1013,21 +1024,10 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type,
>>   
>>   static void cxl_handle_prot_err(struct cxl_cper_prot_err *p_err)
>>   {
>> -	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>>   	struct cxl_dev_state *cxlds;
>> -	unsigned int devfn;
>>   
>> -	devfn = PCI_DEVFN(p_err->device, p_err->function);
>> -	pdev = pci_get_domain_bus_and_slot(p_err->segment,
>> -					   p_err->bus, devfn);
>> -	if (!pdev)
>> -		return;
>> -
>> -	guard(device)(&pdev->dev);
>> -	if (pdev->driver != &cxl_pci_driver)
>> -		return;
>> -
>> -	cxlds = pci_get_drvdata(pdev);
>> +	cxlds = get_cxl_dev(p_err->segment, p_err->bus,
>> +			    p_err->device, p_err->function);
>>   	if (!cxlds)
>>   		return;
>>
Jonathan Cameron June 7, 2024, 3:40 p.m. UTC | #4
On Wed, 22 May 2024 15:08:39 +0000
Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote:

> Refactor computation of cxlds to a common function get_cxl_dev() and reuse
> the function in both cxl_handle_cper_event() and cxl_handle_prot_err().
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
>  drivers/cxl/pci.c | 52 +++++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 3e3c36983686..26e65e5b68cb 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -974,32 +974,43 @@ static struct pci_driver cxl_pci_driver = {
>  	},
>  };
>  
> +static struct cxl_dev_state *get_cxl_dev(u16 segment, u8 bus, u8 device,
> +					 u8 function)
I'd not expect a function with this name to return anything other
than a struct device *
get_cxl_devstate() maybe?

> +{
> +	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> +	struct cxl_dev_state *cxlds;
> +	unsigned int devfn;
> +
> +	devfn = PCI_DEVFN(device, function);
Might as well do
	unsigned int devfn = PCI_DEVFN(device, function);

> +	pdev = pci_get_domain_bus_and_slot(segment, bus, devfn);

	struct pci_dev *pdev __free(pci_dev_put) =
		pci_get_domain_bus_and_slot(segment, bus, devfn);

see fwctl thread for a reference to Linus expressing a preference for
keeping construct and destructor definitions together even when
that means relaxing c conventions that we are so used to!

Obviously this is copied from below, but might as well tidy it up
whilst here.

However, do the devfn change above and it is in the definitions block...




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

I think the function is small enough having cxlds just so it's obvious
what is being returned is unnecessary.
> +}
> +
>  #define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
>  static void cxl_handle_cper_event(enum cxl_event_type ev_type,
>  				  struct cxl_cper_event_rec *rec)
>  {
>  	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> -	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>  	enum cxl_event_log_type log_type;
>  	struct cxl_dev_state *cxlds;
> -	unsigned int devfn;
>  	u32 hdr_flags;
>  
>  	pr_debug("CPER event %d for device %u:%u:%u.%u\n", ev_type,
>  		 device_id->segment_num, device_id->bus_num,
>  		 device_id->device_num, device_id->func_num);
>  
> -	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> -	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> -					   device_id->bus_num, devfn);
> -	if (!pdev)
> -		return;
> -
> -	guard(device)(&pdev->dev);
> -	if (pdev->driver != &cxl_pci_driver)
> -		return;
> -
> -	cxlds = pci_get_drvdata(pdev);
> +	cxlds = get_cxl_dev(device_id->segment_num, device_id->bus_num,
> +			    device_id->device_num, device_id->func_num);
>  	if (!cxlds)
>  		return;
>  
> @@ -1013,21 +1024,10 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type,
>  
>  static void cxl_handle_prot_err(struct cxl_cper_prot_err *p_err)
>  {
> -	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>  	struct cxl_dev_state *cxlds;
> -	unsigned int devfn;
>  
> -	devfn = PCI_DEVFN(p_err->device, p_err->function);
> -	pdev = pci_get_domain_bus_and_slot(p_err->segment,
> -					   p_err->bus, devfn);
> -	if (!pdev)
> -		return;
> -
> -	guard(device)(&pdev->dev);
> -	if (pdev->driver != &cxl_pci_driver)
> -		return;
> -
> -	cxlds = pci_get_drvdata(pdev);
> +	cxlds = get_cxl_dev(p_err->segment, p_err->bus,
> +			    p_err->device, p_err->function);
>  	if (!cxlds)
>  		return;
>
diff mbox series

Patch

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 3e3c36983686..26e65e5b68cb 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -974,32 +974,43 @@  static struct pci_driver cxl_pci_driver = {
 	},
 };
 
+static struct cxl_dev_state *get_cxl_dev(u16 segment, u8 bus, u8 device,
+					 u8 function)
+{
+	struct pci_dev *pdev __free(pci_dev_put) = NULL;
+	struct cxl_dev_state *cxlds;
+	unsigned int devfn;
+
+	devfn = PCI_DEVFN(device, function);
+	pdev = pci_get_domain_bus_and_slot(segment, bus, devfn);
+
+	if (!pdev)
+		return NULL;
+
+	guard(device)(&pdev->dev);
+	if (pdev->driver != &cxl_pci_driver)
+		return NULL;
+
+	cxlds = pci_get_drvdata(pdev);
+
+	return cxlds;
+}
+
 #define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
 static void cxl_handle_cper_event(enum cxl_event_type ev_type,
 				  struct cxl_cper_event_rec *rec)
 {
 	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
-	struct pci_dev *pdev __free(pci_dev_put) = NULL;
 	enum cxl_event_log_type log_type;
 	struct cxl_dev_state *cxlds;
-	unsigned int devfn;
 	u32 hdr_flags;
 
 	pr_debug("CPER event %d for device %u:%u:%u.%u\n", ev_type,
 		 device_id->segment_num, device_id->bus_num,
 		 device_id->device_num, device_id->func_num);
 
-	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
-	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
-					   device_id->bus_num, devfn);
-	if (!pdev)
-		return;
-
-	guard(device)(&pdev->dev);
-	if (pdev->driver != &cxl_pci_driver)
-		return;
-
-	cxlds = pci_get_drvdata(pdev);
+	cxlds = get_cxl_dev(device_id->segment_num, device_id->bus_num,
+			    device_id->device_num, device_id->func_num);
 	if (!cxlds)
 		return;
 
@@ -1013,21 +1024,10 @@  static void cxl_handle_cper_event(enum cxl_event_type ev_type,
 
 static void cxl_handle_prot_err(struct cxl_cper_prot_err *p_err)
 {
-	struct pci_dev *pdev __free(pci_dev_put) = NULL;
 	struct cxl_dev_state *cxlds;
-	unsigned int devfn;
 
-	devfn = PCI_DEVFN(p_err->device, p_err->function);
-	pdev = pci_get_domain_bus_and_slot(p_err->segment,
-					   p_err->bus, devfn);
-	if (!pdev)
-		return;
-
-	guard(device)(&pdev->dev);
-	if (pdev->driver != &cxl_pci_driver)
-		return;
-
-	cxlds = pci_get_drvdata(pdev);
+	cxlds = get_cxl_dev(p_err->segment, p_err->bus,
+			    p_err->device, p_err->function);
 	if (!cxlds)
 		return;