diff mbox series

cxl: fix cdat_available state post error

Message ID 167364543268.908947.8539778012050364814.stgit@djiang5-mobl3.local
State New, archived
Headers show
Series cxl: fix cdat_available state post error | expand

Commit Message

Dave Jiang Jan. 13, 2023, 9:30 p.m. UTC
Given that the sysfs attributre for CDAT_read() checks port->cdat_avilable,
it indicates that the binary backing cdat must be present.

port->cdat_available is set to true in read_cdat_data() before the data is
actually read. When error happens during read, this state is never set to
false. Move the setting of the attribute to after everything is done to
ensure it's always true.

Fixes: c97006046c79 ("cxl/port: Read CDAT table")
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/pci.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ira Weiny Jan. 13, 2023, 11:37 p.m. UTC | #1
On Fri, Jan 13, 2023 at 02:30:34PM -0700, Jiang, Dave wrote:
> Given that the sysfs attributre for CDAT_read() checks port->cdat_avilable,
> it indicates that the binary backing cdat must be present.
> 
> port->cdat_available is set to true in read_cdat_data() before the data is
> actually read. When error happens during read, this state is never set to
> false. Move the setting of the attribute to after everything is done to
> ensure it's always true.

Actually port->cdat_available does not indicate that the data was read.  Only
if the sysfs entry should be there:

...
 * @cdat_available: Should a CDAT attribute be available in sysfs
...

The table pointer should be checked to see if the data is actually available.
If data is not available the sysfs will return no data.

Ira

> 
> Fixes: c97006046c79 ("cxl/port: Read CDAT table")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/pci.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 1d1492440287..91cdd6a00964 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -601,8 +601,6 @@ void read_cdat_data(struct cxl_port *port)
>  		return;
>  	}
>  
> -	port->cdat_available = true;
> -
>  	if (cxl_cdat_get_length(dev, cdat_doe, &cdat_length)) {
>  		dev_dbg(dev, "No CDAT length\n");
>  		return;
> @@ -621,6 +619,8 @@ void read_cdat_data(struct cxl_port *port)
>  		port->cdat.length = 0;
>  		dev_err(dev, "CDAT data read error\n");
>  	}
> +
> +	port->cdat_available = true;
>  }
>  EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>  
> 
>
Dave Jiang Jan. 13, 2023, 11:38 p.m. UTC | #2
On 1/13/23 4:37 PM, Ira Weiny wrote:
> On Fri, Jan 13, 2023 at 02:30:34PM -0700, Jiang, Dave wrote:
>> Given that the sysfs attributre for CDAT_read() checks port->cdat_avilable,
>> it indicates that the binary backing cdat must be present.
>>
>> port->cdat_available is set to true in read_cdat_data() before the data is
>> actually read. When error happens during read, this state is never set to
>> false. Move the setting of the attribute to after everything is done to
>> ensure it's always true.
> 
> Actually port->cdat_available does not indicate that the data was read.  Only
> if the sysfs entry should be there:
> 
> ...
>   * @cdat_available: Should a CDAT attribute be available in sysfs
> ...
> 
> The table pointer should be checked to see if the data is actually available.
> If data is not available the sysfs will return no data.

Ok that makes sense. I only verified to see if cdat_available was 
checked in the sysfs function but didn't read further. Patch dropped.

> 
> Ira
> 
>>
>> Fixes: c97006046c79 ("cxl/port: Read CDAT table")
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/pci.c |    4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 1d1492440287..91cdd6a00964 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -601,8 +601,6 @@ void read_cdat_data(struct cxl_port *port)
>>   		return;
>>   	}
>>   
>> -	port->cdat_available = true;
>> -
>>   	if (cxl_cdat_get_length(dev, cdat_doe, &cdat_length)) {
>>   		dev_dbg(dev, "No CDAT length\n");
>>   		return;
>> @@ -621,6 +619,8 @@ void read_cdat_data(struct cxl_port *port)
>>   		port->cdat.length = 0;
>>   		dev_err(dev, "CDAT data read error\n");
>>   	}
>> +
>> +	port->cdat_available = true;
>>   }
>>   EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>>   
>>
>>
diff mbox series

Patch

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 1d1492440287..91cdd6a00964 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -601,8 +601,6 @@  void read_cdat_data(struct cxl_port *port)
 		return;
 	}
 
-	port->cdat_available = true;
-
 	if (cxl_cdat_get_length(dev, cdat_doe, &cdat_length)) {
 		dev_dbg(dev, "No CDAT length\n");
 		return;
@@ -621,6 +619,8 @@  void read_cdat_data(struct cxl_port *port)
 		port->cdat.length = 0;
 		dev_err(dev, "CDAT data read error\n");
 	}
+
+	port->cdat_available = true;
 }
 EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);