[PATCH] drm/amdgpu: update ras sysfs feature info

[Christian König]

Hi Tao,

yeah Kent is absolutely right. A must have requirement for sysfs is a 
stable interface.

But there are a couple of other violations for sysfs rules as well:
>   	s = scnprintf(buf, PAGE_SIZE, "feature mask: 0x%x\n", con->features);
>   
>   	for (i = 0; i < ras_block_count; i++) {
A must have for sysfs is one value per file. So printing a feature mask 
and then multiple feature values is not really correct.

Additional to that sysfs is design for tool interfacing, not for user 
interfacing. So printing text like "feature xy is enabled/disable" is 
not very convenient either.

What you should do is to create files like amdgpu_ras_feature_xy and 
then return 0/1 based on if the feature is enabled or not.

Regards,
Christian.

Am 08.08.19 um 14:51 schrieb Russell, Kent:
> +Chris Freehill
>
> While I can understand this change, this broke our SMI interface, which was expecting a specific string format for the ras/features file. This has happened a few times now, where changes to the RAS sysfs files has broke the SMI CLI and/or SMI LIB. Can we please get a stable interface and sysfs format set up before publishing patches? This is creating a lot of extra work for developers with the SMI to constantly keep up with the changes being made to sysfs files. Thank you.
>
>   Kent
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Zhang, Hawking
> Sent: Monday, August 5, 2019 4:15 AM
> To: Zhou1, Tao <Tao.Zhou1 at amd.com>; amd-gfx at lists.freedesktop.org; Pan, Xinhui <Xinhui.Pan at amd.com>
> Cc: Zhou1, Tao <Tao.Zhou1 at amd.com>
> Subject: RE: [PATCH] drm/amdgpu: update ras sysfs feature info
>
> Reviewed-by: Hawking Zhang <Hawking.Zhang at amd.com>
>
> Regards,
> Hawking
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Tao Zhou
> Sent: 2019年8月5日 16:04
> To: amd-gfx at lists.freedesktop.org; Pan, Xinhui <Xinhui.Pan at amd.com>; Zhang, Hawking <Hawking.Zhang at amd.com>
> Cc: Zhou1, Tao <Tao.Zhou1 at amd.com>
> Subject: [PATCH] drm/amdgpu: update ras sysfs feature info
>
> remove confused ras error type info
>
> Signed-off-by: Tao Zhou <tao.zhou1 at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 17 +++++------------
>   1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index d2e8a85f6e38..369651247b23 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -787,25 +787,18 @@ static ssize_t amdgpu_ras_sysfs_features_read(struct device *dev,
>   	struct amdgpu_device *adev = ddev->dev_private;
>   	struct ras_common_if head;
>   	int ras_block_count = AMDGPU_RAS_BLOCK_COUNT;
> -	int i;
> +	int i, enabled;
>   	ssize_t s;
> -	struct ras_manager *obj;
>   
>   	s = scnprintf(buf, PAGE_SIZE, "feature mask: 0x%x\n", con->features);
>   
>   	for (i = 0; i < ras_block_count; i++) {
>   		head.block = i;
> +		enabled = amdgpu_ras_is_feature_enabled(adev, &head);
>   
> -		if (amdgpu_ras_is_feature_enabled(adev, &head)) {
> -			obj = amdgpu_ras_find_obj(adev, &head);
> -			s += scnprintf(&buf[s], PAGE_SIZE - s,
> -					"%s: %s\n",
> -					ras_block_str(i),
> -					ras_err_str(obj->head.type));
> -		} else
> -			s += scnprintf(&buf[s], PAGE_SIZE - s,
> -					"%s: disabled\n",
> -					ras_block_str(i));
> +		s += scnprintf(&buf[s], PAGE_SIZE - s,
> +				"%s ras feature mask: %s\n",
> +				ras_block_str(i), enabled?"on":"off");
>   	}
>   
>   	return s;