diff mbox series

[v1,1/2] fpga: dfl: afu: Clear port errors in afu init

Message ID 20211019231545.47118-2-russell.h.weight@intel.com (mailing list archive)
State New
Headers show
Series fpga: dfl: Log and clear errors on driver init | expand

Commit Message

Russ Weight Oct. 19, 2021, 11:15 p.m. UTC 
When the AFU driver initializes, log any pre-existing errors and clear them.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/fpga/dfl-afu-error.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

Xu Yilun Oct. 20, 2021, 4:15 a.m. UTC | #1 
On Tue, Oct 19, 2021 at 04:15:44PM -0700, Russ Weight wrote:
> When the AFU driver initializes, log any pre-existing errors and clear them.

Please wrapper the commit message to pass checkpatch.pl

> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  drivers/fpga/dfl-afu-error.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> index ab7be6217368..0dc60bf49902 100644
> --- a/drivers/fpga/dfl-afu-error.c
> +++ b/drivers/fpga/dfl-afu-error.c
> @@ -47,13 +47,13 @@ static void afu_port_err_mask(struct device *dev, bool mask)
>  }
>  
>  /* clear port errors. */
> -static int afu_port_err_clear(struct device *dev, u64 err)
> +static int afu_port_err_clear(struct device *dev, u64 err, bool clear_all)

how about "clear_all_on_init"?

>  {
>  	struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
>  	struct platform_device *pdev = to_platform_device(dev);
> +	u64 v, port_error, port_first_error;
>  	void __iomem *base_err, *base_hdr;
>  	int enable_ret = 0, ret = -EBUSY;
> -	u64 v;
>  
>  	base_err = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
>  	base_hdr = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> @@ -88,16 +88,21 @@ static int afu_port_err_clear(struct device *dev, u64 err)
>  	__afu_port_err_mask(dev, true);
>  
>  	/* Clear errors if err input matches with current port errors.*/
> -	v = readq(base_err + PORT_ERROR);
> +	port_error = readq(base_err + PORT_ERROR);
>  
> -	if (v == err) {
> -		writeq(v, base_err + PORT_ERROR);
> +	if (clear_all || port_error == err) {
> +		port_first_error = readq(base_err + PORT_FIRST_ERROR);
>  
> -		v = readq(base_err + PORT_FIRST_ERROR);
> -		writeq(v, base_err + PORT_FIRST_ERROR);
> +		if (clear_all && (port_error || port_first_error))
> +			dev_warn(dev,
> +				 "Port Error: 0x%llx, First Error 0x%llx\n",
> +				 port_error, port_first_error);

The log could be more specific that we are reporting errors on device
initialization.

Thanks,
Yilun

> +
> +		writeq(port_error, base_err + PORT_ERROR);
> +		writeq(port_first_error, base_err + PORT_FIRST_ERROR);
>  	} else {
>  		dev_warn(dev, "%s: received 0x%llx, expected 0x%llx\n",
> -			 __func__, v, err);
> +			 __func__, port_error, err);
>  		ret = -EINVAL;
>  	}
>  
> @@ -137,7 +142,7 @@ static ssize_t errors_store(struct device *dev, struct device_attribute *attr,
>  	if (kstrtou64(buff, 0, &value))
>  		return -EINVAL;
>  
> -	ret = afu_port_err_clear(dev, value);
> +	ret = afu_port_err_clear(dev, value, false);
>  
>  	return ret ? ret : count;
>  }
> @@ -211,7 +216,8 @@ const struct attribute_group port_err_group = {
>  static int port_err_init(struct platform_device *pdev,
>  			 struct dfl_feature *feature)
>  {
> -	afu_port_err_mask(&pdev->dev, false);
> +	if (afu_port_err_clear(&pdev->dev, 0, true))
> +		afu_port_err_mask(&pdev->dev, false);
>  
>  	return 0;
>  }
> -- 
> 2.25.1
Tom Rix Oct. 20, 2021, 12:18 p.m. UTC | #2 
On 10/19/21 4:15 PM, Russ Weight wrote:
> When the AFU driver initializes, log any pre-existing errors and clear them.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>   drivers/fpga/dfl-afu-error.c | 26 ++++++++++++++++----------
>   1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> index ab7be6217368..0dc60bf49902 100644
> --- a/drivers/fpga/dfl-afu-error.c
> +++ b/drivers/fpga/dfl-afu-error.c
> @@ -47,13 +47,13 @@ static void afu_port_err_mask(struct device *dev, bool mask)
>   }
>   
>   /* clear port errors. */
> -static int afu_port_err_clear(struct device *dev, u64 err)
> +static int afu_port_err_clear(struct device *dev, u64 err, bool clear_all)
>   {
>   	struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
>   	struct platform_device *pdev = to_platform_device(dev);
> +	u64 v, port_error, port_first_error;
v is only used now by the read of PORT_HDR_STS, could v be changed to a 
more descriptive variable like hdr_sts ?
>   	void __iomem *base_err, *base_hdr;
>   	int enable_ret = 0, ret = -EBUSY;
> -	u64 v;
>   
>   	base_err = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
>   	base_hdr = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> @@ -88,16 +88,21 @@ static int afu_port_err_clear(struct device *dev, u64 err)
>   	__afu_port_err_mask(dev, true);
>   
>   	/* Clear errors if err input matches with current port errors.*/
> -	v = readq(base_err + PORT_ERROR);
> +	port_error = readq(base_err + PORT_ERROR);
>   
> -	if (v == err) {
> -		writeq(v, base_err + PORT_ERROR);
> +	if (clear_all || port_error == err) {
> +		port_first_error = readq(base_err + PORT_FIRST_ERROR);
>   
> -		v = readq(base_err + PORT_FIRST_ERROR);
> -		writeq(v, base_err + PORT_FIRST_ERROR);
> +		if (clear_all && (port_error || port_first_error))

likely with clear_all that this dev_warn will over report.

how about removing clear_all && from if-check ?

> +			dev_warn(dev,
> +				 "Port Error: 0x%llx, First Error 0x%llx\n",
> +				 port_error, port_first_error);
> +
> +		writeq(port_error, base_err + PORT_ERROR);
> +		writeq(port_first_error, base_err + PORT_FIRST_ERROR);
>   	} else {
>   		dev_warn(dev, "%s: received 0x%llx, expected 0x%llx\n",
> -			 __func__, v, err);
> +			 __func__, port_error, err);
>   		ret = -EINVAL;
>   	}
>   
> @@ -137,7 +142,7 @@ static ssize_t errors_store(struct device *dev, struct device_attribute *attr,
>   	if (kstrtou64(buff, 0, &value))
>   		return -EINVAL;
>   
> -	ret = afu_port_err_clear(dev, value);
> +	ret = afu_port_err_clear(dev, value, false);
>   
>   	return ret ? ret : count;
>   }
> @@ -211,7 +216,8 @@ const struct attribute_group port_err_group = {
>   static int port_err_init(struct platform_device *pdev,
>   			 struct dfl_feature *feature)
>   {
> -	afu_port_err_mask(&pdev->dev, false);
> +	if (afu_port_err_clear(&pdev->dev, 0, true))
> +		afu_port_err_mask(&pdev->dev, false);

There is a __afu_port_err_mask at the end of afu_port_err_clear so this 
call isn't needed.

Tom

>   
>   	return 0;
>   }
Russ Weight Oct. 20, 2021, 4:02 p.m. UTC | #3 
On 10/20/21 5:18 AM, Tom Rix wrote:
>
> On 10/19/21 4:15 PM, Russ Weight wrote:
>> When the AFU driver initializes, log any pre-existing errors and clear them.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>>   drivers/fpga/dfl-afu-error.c | 26 ++++++++++++++++----------
>>   1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
>> index ab7be6217368..0dc60bf49902 100644
>> --- a/drivers/fpga/dfl-afu-error.c
>> +++ b/drivers/fpga/dfl-afu-error.c
>> @@ -47,13 +47,13 @@ static void afu_port_err_mask(struct device *dev, bool mask)
>>   }
>>     /* clear port errors. */
>> -static int afu_port_err_clear(struct device *dev, u64 err)
>> +static int afu_port_err_clear(struct device *dev, u64 err, bool clear_all)
>>   {
>>       struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
>>       struct platform_device *pdev = to_platform_device(dev);
>> +    u64 v, port_error, port_first_error;
> v is only used now by the read of PORT_HDR_STS, could v be changed to a more descriptive variable like hdr_sts ?
>>       void __iomem *base_err, *base_hdr;
>>       int enable_ret = 0, ret = -EBUSY;
>> -    u64 v;
>>         base_err = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
>>       base_hdr = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
>> @@ -88,16 +88,21 @@ static int afu_port_err_clear(struct device *dev, u64 err)
>>       __afu_port_err_mask(dev, true);
>>         /* Clear errors if err input matches with current port errors.*/
>> -    v = readq(base_err + PORT_ERROR);
>> +    port_error = readq(base_err + PORT_ERROR);
>>   -    if (v == err) {
>> -        writeq(v, base_err + PORT_ERROR);
>> +    if (clear_all || port_error == err) {
>> +        port_first_error = readq(base_err + PORT_FIRST_ERROR);
>>   -        v = readq(base_err + PORT_FIRST_ERROR);
>> -        writeq(v, base_err + PORT_FIRST_ERROR);
>> +        if (clear_all && (port_error || port_first_error))
>
> likely with clear_all that this dev_warn will over report.
>
> how about removing clear_all && from if-check ?
I think that would make it report more often? clear_all is only set
at the time that the driver initializes. With the current condition,
errors will only be logged when the driver loads, if and only if
there are actually errors to report.


>
>> +            dev_warn(dev,
>> +                 "Port Error: 0x%llx, First Error 0x%llx\n",
>> +                 port_error, port_first_error);
>> +
>> +        writeq(port_error, base_err + PORT_ERROR);
>> +        writeq(port_first_error, base_err + PORT_FIRST_ERROR);
>>       } else {
>>           dev_warn(dev, "%s: received 0x%llx, expected 0x%llx\n",
>> -             __func__, v, err);
>> +             __func__, port_error, err);
>>           ret = -EINVAL;
>>       }
>>   @@ -137,7 +142,7 @@ static ssize_t errors_store(struct device *dev, struct device_attribute *attr,
>>       if (kstrtou64(buff, 0, &value))
>>           return -EINVAL;
>>   -    ret = afu_port_err_clear(dev, value);
>> +    ret = afu_port_err_clear(dev, value, false);
>>         return ret ? ret : count;
>>   }
>> @@ -211,7 +216,8 @@ const struct attribute_group port_err_group = {
>>   static int port_err_init(struct platform_device *pdev,
>>                struct dfl_feature *feature)
>>   {
>> -    afu_port_err_mask(&pdev->dev, false);
>> +    if (afu_port_err_clear(&pdev->dev, 0, true))
>> +        afu_port_err_mask(&pdev->dev, false);
>
> There is a __afu_port_err_mask at the end of afu_port_err_clear so this call isn't needed.

The condition statements says to only clear the mask explicitly if afu_port_err_clear()
fails. If it succeeds, then the explicit call will not happen, because as you have
pointed out, afu_port_err_clear() clear the mask.

Thanks,
- Russ

>
> Tom
>
>>         return 0;
>>   }
>
diff mbox series

Patch

diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
index ab7be6217368..0dc60bf49902 100644
--- a/drivers/fpga/dfl-afu-error.c
+++ b/drivers/fpga/dfl-afu-error.c
@@ -47,13 +47,13 @@  static void afu_port_err_mask(struct device *dev, bool mask)
 }
 
 /* clear port errors. */
-static int afu_port_err_clear(struct device *dev, u64 err)
+static int afu_port_err_clear(struct device *dev, u64 err, bool clear_all)
 {
 	struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
 	struct platform_device *pdev = to_platform_device(dev);
+	u64 v, port_error, port_first_error;
 	void __iomem *base_err, *base_hdr;
 	int enable_ret = 0, ret = -EBUSY;
-	u64 v;
 
 	base_err = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
 	base_hdr = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
@@ -88,16 +88,21 @@  static int afu_port_err_clear(struct device *dev, u64 err)
 	__afu_port_err_mask(dev, true);
 
 	/* Clear errors if err input matches with current port errors.*/
-	v = readq(base_err + PORT_ERROR);
+	port_error = readq(base_err + PORT_ERROR);
 
-	if (v == err) {
-		writeq(v, base_err + PORT_ERROR);
+	if (clear_all || port_error == err) {
+		port_first_error = readq(base_err + PORT_FIRST_ERROR);
 
-		v = readq(base_err + PORT_FIRST_ERROR);
-		writeq(v, base_err + PORT_FIRST_ERROR);
+		if (clear_all && (port_error || port_first_error))
+			dev_warn(dev,
+				 "Port Error: 0x%llx, First Error 0x%llx\n",
+				 port_error, port_first_error);
+
+		writeq(port_error, base_err + PORT_ERROR);
+		writeq(port_first_error, base_err + PORT_FIRST_ERROR);
 	} else {
 		dev_warn(dev, "%s: received 0x%llx, expected 0x%llx\n",
-			 __func__, v, err);
+			 __func__, port_error, err);
 		ret = -EINVAL;
 	}
 
@@ -137,7 +142,7 @@  static ssize_t errors_store(struct device *dev, struct device_attribute *attr,
 	if (kstrtou64(buff, 0, &value))
 		return -EINVAL;
 
-	ret = afu_port_err_clear(dev, value);
+	ret = afu_port_err_clear(dev, value, false);
 
 	return ret ? ret : count;
 }
@@ -211,7 +216,8 @@  const struct attribute_group port_err_group = {
 static int port_err_init(struct platform_device *pdev,
 			 struct dfl_feature *feature)
 {
-	afu_port_err_mask(&pdev->dev, false);
+	if (afu_port_err_clear(&pdev->dev, 0, true))
+		afu_port_err_mask(&pdev->dev, false);
 
 	return 0;
 }