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

[Russ Weight <russell.h.weight@intel.com>]

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;
>>   }
>