Re: [PATCHv4] cxl: avoid driver data for obtaining cxl_dev_state reference

[Dave Jiang <dave.jiang@intel.com>]

On 12/12/24 2:46 PM, Alison Schofield wrote:
> On Tue, Dec 03, 2024 at 04:21:12PM +0000, alucerop@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> CXL Type3 pci driver uses struct device driver_data for keeping
>> cxl_dev_state reference. Type1/2 drivers are not only about CXL so this
>> field should not be used when code requires cxl_dev_state to work with
>> and such a code used for Type2 support.
>>
>> Change cxl_dvsec_rr_decode for passing cxl_dev_state as a parameter.
>>
>> Seize the change for removing the unused cxl_port param.
>>
> 
> Dave,
> 
> Jonathan previously offered Reviewed-by tag so this looks good to
> apply. I've added another review tag and also massaged the 
> changelog for your consideration.
> 
> cxl/pci: Add Type 1/2 support to cxl_dvsec_rr_decode()
> 
> In cxl_dvsec_rr_decode() the pci driver expects to retrieve a cxlds,
> struct cxl_dev_state, from the driver_data field of struct device.
> While that works for Type 3, drivers for Type 1/2 devices may not
> put a cxlds in the driver_data field.
> 
> In preparation for supporting Type 1/2 devices, replace parameter
> 'struct device' with 'struct cxl_dev_state' in cxl_dvsec_rr_decode().
> 
> Remove the unused parameter 'cxl_port' in cxl_dvsec_rr_decode().
> 
> [as: massaged commit msg and log]
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Thanks! I'll take it with the commit log changes.

> 
> 
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>  drivers/cxl/core/pci.c        | 6 +++---
>>  drivers/cxl/cxl.h             | 3 ++-
>>  drivers/cxl/port.c            | 2 +-
>>  tools/testing/cxl/test/mock.c | 6 +++---
>>  4 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 5b46bc46aaa9..420e4be85a1f 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -291,11 +291,11 @@ static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
>>  	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
>>  }
>>  
>> -int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
>> +int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds,
>>  			struct cxl_endpoint_dvsec_info *info)
>>  {
>> -	struct pci_dev *pdev = to_pci_dev(dev);
>> -	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>> +	struct device *dev = cxlds->dev;
>>  	int hdm_count, rc, i, ranges = 0;
>>  	int d = cxlds->cxl_dvsec;
>>  	u16 cap, ctrl;
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index f6015f24ad38..fdac3ddb8635 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -821,7 +821,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>>  int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>>  				struct cxl_endpoint_dvsec_info *info);
>>  int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
>> -int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
>> +struct cxl_dev_state;
>> +int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds,
>>  			struct cxl_endpoint_dvsec_info *info);
>>  
>>  bool is_cxl_region(struct device *dev);
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index 24041cf85cfb..66e18fe55826 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -98,7 +98,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>>  	struct cxl_port *root;
>>  	int rc;
>>  
>> -	rc = cxl_dvsec_rr_decode(cxlds->dev, port, &info);
>> +	rc = cxl_dvsec_rr_decode(cxlds, &info);
>>  	if (rc < 0)
>>  		return rc;
>>  
>> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
>> index f4ce96cc11d4..4f82716cfc16 100644
>> --- a/tools/testing/cxl/test/mock.c
>> +++ b/tools/testing/cxl/test/mock.c
>> @@ -228,16 +228,16 @@ int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>>  }
>>  EXPORT_SYMBOL_NS_GPL(__wrap_cxl_hdm_decode_init, CXL);
>>  
>> -int __wrap_cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
>> +int __wrap_cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds,
>>  			       struct cxl_endpoint_dvsec_info *info)
>>  {
>>  	int rc = 0, index;
>>  	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
>>  
>> -	if (ops && ops->is_mock_dev(dev))
>> +	if (ops && ops->is_mock_dev(cxlds->dev))
>>  		rc = 0;
>>  	else
>> -		rc = cxl_dvsec_rr_decode(dev, port, info);
>> +		rc = cxl_dvsec_rr_decode(cxlds, info);
>>  	put_cxl_mock_ops(index);
>>  
>>  	return rc;
>> -- 
>> 2.17.1
>>
>>