All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
@ 2021-05-08  6:47 Jiawei Gu
  2021-05-09 15:58 ` Alex Deucher
  0 siblings, 1 reply; 18+ messages in thread
From: Jiawei Gu @ 2021-05-08  6:47 UTC (permalink / raw)
  To: amd-gfx; +Cc: Jiawei Gu, keescook

20 should be serial char size now instead of 16.

Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
---
 include/uapi/drm/amdgpu_drm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 2b487a8d2727..1c20721f90da 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
 	__u8 vbios_pn[64];
 	__u32 version;
 	__u8 date[32];
-	__u8 serial[16];
+	__u8 serial[20];
 	__u32 dev_id;
 	__u32 rev_id;
 	__u32 sub_dev_id;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-08  6:47 [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios Jiawei Gu
@ 2021-05-09 15:58 ` Alex Deucher
  2021-05-10  6:33   ` Gu, JiaWei (Will)
  0 siblings, 1 reply; 18+ messages in thread
From: Alex Deucher @ 2021-05-09 15:58 UTC (permalink / raw)
  To: Jiawei Gu; +Cc: Kees Cook, amd-gfx list

On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com> wrote:
>
> 20 should be serial char size now instead of 16.
>
> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>

Please make sure this keeps proper 64 bit alignment in the structure.

Alex


> ---
>  include/uapi/drm/amdgpu_drm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 2b487a8d2727..1c20721f90da 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>         __u8 vbios_pn[64];
>         __u32 version;
>         __u8 date[32];
> -       __u8 serial[16];
> +       __u8 serial[20];
>         __u32 dev_id;
>         __u32 rev_id;
>         __u32 sub_dev_id;
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-09 15:58 ` Alex Deucher
@ 2021-05-10  6:33   ` Gu, JiaWei (Will)
  2021-05-10  6:53     ` Nieto, David M
  0 siblings, 1 reply; 18+ messages in thread
From: Gu, JiaWei (Will) @ 2021-05-10  6:33 UTC (permalink / raw)
  To: Alex Deucher, Nieto, David M; +Cc: Deng, Emily, Kees Cook, amd-gfx list

[AMD Official Use Only - Internal Distribution Only]

With a second thought, 
__u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.

Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex Deucher @Nieto, David M

Best regards,
Jiawei

-----Original Message-----
From: Alex Deucher <alexdeucher@gmail.com> 
Sent: Sunday, May 9, 2021 11:59 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com> wrote:
>
> 20 should be serial char size now instead of 16.
>
> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>

Please make sure this keeps proper 64 bit alignment in the structure.

Alex


> ---
>  include/uapi/drm/amdgpu_drm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/drm/amdgpu_drm.h 
> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da 
> 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>         __u8 vbios_pn[64];
>         __u32 version;
>         __u8 date[32];
> -       __u8 serial[16];
> +       __u8 serial[20];
>         __u32 dev_id;
>         __u32 rev_id;
>         __u32 sub_dev_id;
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJi
> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e6
> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3d
> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;rese
> rved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-10  6:33   ` Gu, JiaWei (Will)
@ 2021-05-10  6:53     ` Nieto, David M
  2021-05-10  7:13       ` Gu, JiaWei (Will)
  0 siblings, 1 reply; 18+ messages in thread
From: Nieto, David M @ 2021-05-10  6:53 UTC (permalink / raw)
  To: Gu, JiaWei (Will); +Cc: Alex Deucher, Deng, Emily, Kees Cook, amd-gfx list

No, this structure contains all the details of the vbios: date, serial number, name, etc.

The sysfs node only contains the vbios name string

> On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com> wrote:
> 
> [AMD Official Use Only - Internal Distribution Only]
> 
> With a second thought, 
> __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.
> 
> Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex Deucher @Nieto, David M
> 
> Best regards,
> Jiawei
> 
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com> 
> Sent: Sunday, May 9, 2021 11:59 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
> 
>> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com> wrote:
>> 
>> 20 should be serial char size now instead of 16.
>> 
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
> 
> Please make sure this keeps proper 64 bit alignment in the structure.
> 
> Alex
> 
> 
>> ---
>> include/uapi/drm/amdgpu_drm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da 
>> 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u8 serial[20];
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --
>> 2.17.1
>> 
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJi
>> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e6
>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3d
>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
>> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;rese
>> rved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-10  6:53     ` Nieto, David M
@ 2021-05-10  7:13       ` Gu, JiaWei (Will)
  2021-05-10 13:42         ` Nieto, David M
  0 siblings, 1 reply; 18+ messages in thread
From: Gu, JiaWei (Will) @ 2021-05-10  7:13 UTC (permalink / raw)
  To: Nieto, David M; +Cc: Alex Deucher, Deng, Emily, Kees Cook, amd-gfx list

[AMD Official Use Only - Internal Distribution Only]

Hi David,

What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios, not the whole struct.

struct drm_amdgpu_info_vbios {
	__u8 name[64];
	__u32 dbdf;
	__u8 vbios_pn[64];
	__u32 version;
	__u8 date[32];
	__u8 serial[16]; // jiawei: shall we delete this
	__u32 dev_id;
	__u32 rev_id;
	__u32 sub_dev_id;
	__u32 sub_ved_id;
};

serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's already a sysfs named serial_number, which exposes it already.

static ssize_t amdgpu_device_get_serial_number(struct device *dev,
		struct device_attribute *attr, char *buf)
{
	struct drm_device *ddev = dev_get_drvdata(dev);
	struct amdgpu_device *adev = ddev->dev_private;

	return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
}

Thanks,
Jiawei


-----Original Message-----
From: Nieto, David M <David.Nieto@amd.com> 
Sent: Monday, May 10, 2021 2:53 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

No, this structure contains all the details of the vbios: date, serial number, name, etc.

The sysfs node only contains the vbios name string

> On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com> wrote:
> 
> [AMD Official Use Only - Internal Distribution Only]
> 
> With a second thought,
> __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.
> 
> Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex 
> Deucher @Nieto, David M
> 
> Best regards,
> Jiawei
> 
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Sunday, May 9, 2021 11:59 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Kees Cook 
> <keescook@chromium.org>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in 
> drm_amdgpu_info_vbios
> 
>> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com> wrote:
>> 
>> 20 should be serial char size now instead of 16.
>> 
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
> 
> Please make sure this keeps proper 64 bit alignment in the structure.
> 
> Alex
> 
> 
>> ---
>> include/uapi/drm/amdgpu_drm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/include/uapi/drm/amdgpu_drm.h 
>> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
>> 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u8 serial[20];
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --
>> 2.17.1
>> 
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> t 
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>> i
>> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>> 6 
>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>> d 
>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>> C 
>> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>> e
>> rved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-10  7:13       ` Gu, JiaWei (Will)
@ 2021-05-10 13:42         ` Nieto, David M
  2021-05-10 13:48           ` Christian König
  0 siblings, 1 reply; 18+ messages in thread
From: Nieto, David M @ 2021-05-10 13:42 UTC (permalink / raw)
  To: Gu, JiaWei (Will); +Cc: Alex Deucher, Deng, Emily, Kees Cook, amd-gfx list


[-- Attachment #1.1: Type: text/plain, Size: 4327 bytes --]

Then the application would need to issue the ioctl and then open a sysfs file to get all the information it needs. It makes little sense from a programming perspective to add an incomplete interface in my opinion

________________________________
From: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
Sent: Monday, May 10, 2021 12:13:07 AM
To: Nieto, David M <David.Nieto@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com>
Subject: RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

[AMD Official Use Only - Internal Distribution Only]

Hi David,

What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios, not the whole struct.

struct drm_amdgpu_info_vbios {
        __u8 name[64];
        __u32 dbdf;
        __u8 vbios_pn[64];
        __u32 version;
        __u8 date[32];
        __u8 serial[16]; // jiawei: shall we delete this
        __u32 dev_id;
        __u32 rev_id;
        __u32 sub_dev_id;
        __u32 sub_ved_id;
};

serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's already a sysfs named serial_number, which exposes it already.

static ssize_t amdgpu_device_get_serial_number(struct device *dev,
                struct device_attribute *attr, char *buf)
{
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;

        return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
}

Thanks,
Jiawei


-----Original Message-----
From: Nieto, David M <David.Nieto@amd.com>
Sent: Monday, May 10, 2021 2:53 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

No, this structure contains all the details of the vbios: date, serial number, name, etc.

The sysfs node only contains the vbios name string

> On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> With a second thought,
> __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.
>
> Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> Deucher @Nieto, David M
>
> Best regards,
> Jiawei
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Sunday, May 9, 2021 11:59 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Kees Cook
> <keescook@chromium.org>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com> wrote:
>>
>> 20 should be serial char size now instead of 16.
>>
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
>
> Please make sure this keeps proper 64 bit alignment in the structure.
>
> Alex
>
>
>> ---
>> include/uapi/drm/amdgpu_drm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
>> 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u8 serial[20];
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> t
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>> i
>> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>> 6
>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>> d
>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>> C
>> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>> e
>> rved=0

[-- Attachment #1.2: Type: text/html, Size: 6831 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-10 13:42         ` Nieto, David M
@ 2021-05-10 13:48           ` Christian König
  2021-05-10 20:19             ` Nieto, David M
  0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2021-05-10 13:48 UTC (permalink / raw)
  To: Nieto, David M, Gu, JiaWei (Will)
  Cc: Alex Deucher, Deng, Emily, Kees Cook, amd-gfx list


[-- Attachment #1.1: Type: text/plain, Size: 5179 bytes --]

Well we could add both as sysfs file(s).

Question here is rather what is the primary use case of this and if the 
application has the necessary access permissions to the sysfs files?

Regards,
Christian.

Am 10.05.21 um 15:42 schrieb Nieto, David M:
> Then the application would need to issue the ioctl and then open a 
> sysfs file to get all the information it needs. It makes little sense 
> from a programming perspective to add an incomplete interface in my 
> opinion
>
> ------------------------------------------------------------------------
> *From:* Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
> *Sent:* Monday, May 10, 2021 12:13:07 AM
> *To:* Nieto, David M <David.Nieto@amd.com>
> *Cc:* Alex Deucher <alexdeucher@gmail.com>; amd-gfx list 
> <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>; 
> Deng, Emily <Emily.Deng@amd.com>
> *Subject:* RE: [PATCH] drm/amdgpu: Align serial size in 
> drm_amdgpu_info_vbios
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi David,
>
> What I meant is to ONLY delete the serial[16] from 
> drm_amdgpu_info_vbios, not the whole struct.
>
> struct drm_amdgpu_info_vbios {
>         __u8 name[64];
>         __u32 dbdf;
>         __u8 vbios_pn[64];
>         __u32 version;
>         __u8 date[32];
>         __u8 serial[16]; // jiawei: shall we delete this
>         __u32 dev_id;
>         __u32 rev_id;
>         __u32 sub_dev_id;
>         __u32 sub_ved_id;
> };
>
> serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but 
> there's already a sysfs named serial_number, which exposes it already.
>
> static ssize_t amdgpu_device_get_serial_number(struct device *dev,
>                 struct device_attribute *attr, char *buf)
> {
>         struct drm_device *ddev = dev_get_drvdata(dev);
>         struct amdgpu_device *adev = ddev->dev_private;
>
>         return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
> }
>
> Thanks,
> Jiawei
>
>
> -----Original Message-----
> From: Nieto, David M <David.Nieto@amd.com>
> Sent: Monday, May 10, 2021 2:53 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
> Cc: Alex Deucher <alexdeucher@gmail.com>; amd-gfx list 
> <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>; 
> Deng, Emily <Emily.Deng@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in 
> drm_amdgpu_info_vbios
>
> No, this structure contains all the details of the vbios: date, serial 
> number, name, etc.
>
> The sysfs node only contains the vbios name string
>
> > On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com> wrote:
> >
> > [AMD Official Use Only - Internal Distribution Only]
> >
> > With a second thought,
> > __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs 
> serial_number already exposes it.
> >
> > Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> > Deucher @Nieto, David M
> >
> > Best regards,
> > Jiawei
> >
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@gmail.com>
> > Sent: Sunday, May 9, 2021 11:59 PM
> > To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
> > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Kees Cook
> > <keescook@chromium.org>
> > Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> > drm_amdgpu_info_vbios
> >
> >> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com> wrote:
> >>
> >> 20 should be serial char size now instead of 16.
> >>
> >> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
> >
> > Please make sure this keeps proper 64 bit alignment in the structure.
> >
> > Alex
> >
> >
> >> ---
> >> include/uapi/drm/amdgpu_drm.h | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/uapi/drm/amdgpu_drm.h
> >> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
> >> 100644
> >> --- a/include/uapi/drm/amdgpu_drm.h
> >> +++ b/include/uapi/drm/amdgpu_drm.h
> >> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
> >>        __u8 vbios_pn[64];
> >>        __u32 version;
> >>        __u8 date[32];
> >> -       __u8 serial[16];
> >> +       __u8 serial[20];
> >>        __u32 dev_id;
> >>        __u32 rev_id;
> >>        __u32 sub_dev_id;
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis>
> >> t
> >> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
> >> i
> >> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
> >> 6
> >> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
> >> d
> >> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> >> C
> >> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
> >> e
> >> rved=0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 10758 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-10 13:48           ` Christian König
@ 2021-05-10 20:19             ` Nieto, David M
  2021-05-11  2:23               ` Gu, JiaWei (Will)
  2021-05-11  7:51               ` Christian König
  0 siblings, 2 replies; 18+ messages in thread
From: Nieto, David M @ 2021-05-10 20:19 UTC (permalink / raw)
  To: Christian König, Gu, JiaWei (Will)
  Cc: Alex Deucher, Deng, Emily, Kees Cook, amd-gfx list


[-- Attachment #1.1: Type: text/plain, Size: 6652 bytes --]

One of the primary usecases is to add this information to the renderer string, I am not sure if there are other cases of UMD drivers accessing sysfs nodes, but I think if we think permissions, if a client is authenticated and opens the render device then it can use the IOCTL, it is unclear to me we can make a such an assumption for sysfs nodes…

I think there is value in having both tbh.

Regards,
David

From: Christian König <ckoenig.leichtzumerken@gmail.com>
Date: Monday, May 10, 2021 at 6:48 AM
To: "Nieto, David M" <David.Nieto@amd.com>, "Gu, JiaWei (Will)" <JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>, "Deng, Emily" <Emily.Deng@amd.com>, Kees Cook <keescook@chromium.org>, amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

Well we could add both as sysfs file(s).

Question here is rather what is the primary use case of this and if the application has the necessary access permissions to the sysfs files?

Regards,
Christian.
Am 10.05.21 um 15:42 schrieb Nieto, David M:
Then the application would need to issue the ioctl and then open a sysfs file to get all the information it needs. It makes little sense from a programming perspective to add an incomplete interface in my opinion

________________________________
From: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Sent: Monday, May 10, 2021 12:13:07 AM
To: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

[AMD Official Use Only - Internal Distribution Only]

Hi David,

What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios, not the whole struct.

struct drm_amdgpu_info_vbios {
        __u8 name[64];
        __u32 dbdf;
        __u8 vbios_pn[64];
        __u32 version;
        __u8 date[32];
        __u8 serial[16]; // jiawei: shall we delete this
        __u32 dev_id;
        __u32 rev_id;
        __u32 sub_dev_id;
        __u32 sub_ved_id;
};

serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's already a sysfs named serial_number, which exposes it already.

static ssize_t amdgpu_device_get_serial_number(struct device *dev,
                struct device_attribute *attr, char *buf)
{
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;

        return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
}

Thanks,
Jiawei


-----Original Message-----
From: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Sent: Monday, May 10, 2021 2:53 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

No, this structure contains all the details of the vbios: date, serial number, name, etc.

The sysfs node only contains the vbios name string

> On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> With a second thought,
> __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.
>
> Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> Deucher @Nieto, David M
>
> Best regards,
> Jiawei
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>
> Sent: Sunday, May 9, 2021 11:59 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
> <keescook@chromium.org><mailto:keescook@chromium.org>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com> wrote:
>>
>> 20 should be serial char size now instead of 16.
>>
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com>
>
> Please make sure this keeps proper 64 bit alignment in the structure.
>
> Alex
>
>
>> ---
>> include/uapi/drm/amdgpu_drm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
>> 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u8 serial[20];
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> t
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>> i
>> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>> 6
>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>> d
>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>> C
>> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>> e
>> rved=0



_______________________________________________

amd-gfx mailing list

amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>

https://lists.freedesktop.org/mailman/listinfo/amd-gfx<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7C3c0071a8a2a74127027408d913ba53e4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637562513264718308%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=g11aunC78b48N19Q%2FFXJBK%2B2Z0PZBWdqp%2FzhsPiqQ8Q%3D&reserved=0>



[-- Attachment #1.2: Type: text/html, Size: 12867 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-10 20:19             ` Nieto, David M
@ 2021-05-11  2:23               ` Gu, JiaWei (Will)
  2021-05-11  2:52                 ` Nieto, David M
  2021-05-11  7:51               ` Christian König
  1 sibling, 1 reply; 18+ messages in thread
From: Gu, JiaWei (Will) @ 2021-05-11  2:23 UTC (permalink / raw)
  To: Nieto, David M, Christian König
  Cc: Alex Deucher, Deng, Emily, Kees Cook, amd-gfx list


[-- Attachment #1.1: Type: text/plain, Size: 8037 bytes --]

[AMD Official Use Only - Internal Distribution Only]

Got it. Let's keep them both.

Another idea about drm_amdgpu_info_vbios is
Does it make more sense to fill the "serial info" with uint64_t adev->unique_id, instead of the current char[] in adev->serial?

Sorry about that I was not aware of adev->unique_id exists when I defined drm_amdgpu_info_vbios.
I think it's clearer to use original numeric variable than a string to expose serial.

How about that?

>> struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u64 serial;
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --

Best regards,
Jiawei


From: Nieto, David M <David.Nieto@amd.com>
Sent: Tuesday, May 11, 2021 4:20 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; Deng, Emily <Emily.Deng@amd.com>; Kees Cook <keescook@chromium.org>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

One of the primary usecases is to add this information to the renderer string, I am not sure if there are other cases of UMD drivers accessing sysfs nodes, but I think if we think permissions, if a client is authenticated and opens the render device then it can use the IOCTL, it is unclear to me we can make a such an assumption for sysfs nodes...

I think there is value in having both tbh.

Regards,
David

From: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>
Date: Monday, May 10, 2021 at 6:48 AM
To: "Nieto, David M" <David.Nieto@amd.com<mailto:David.Nieto@amd.com>>, "Gu, JiaWei (Will)" <JiaWei.Gu@amd.com<mailto:JiaWei.Gu@amd.com>>
Cc: Alex Deucher <alexdeucher@gmail.com<mailto:alexdeucher@gmail.com>>, "Deng, Emily" <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>, Kees Cook <keescook@chromium.org<mailto:keescook@chromium.org>>, amd-gfx list <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

Well we could add both as sysfs file(s).

Question here is rather what is the primary use case of this and if the application has the necessary access permissions to the sysfs files?

Regards,
Christian.
Am 10.05.21 um 15:42 schrieb Nieto, David M:
Then the application would need to issue the ioctl and then open a sysfs file to get all the information it needs. It makes little sense from a programming perspective to add an incomplete interface in my opinion

________________________________
From: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Sent: Monday, May 10, 2021 12:13:07 AM
To: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

[AMD Official Use Only - Internal Distribution Only]

Hi David,

What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios, not the whole struct.

struct drm_amdgpu_info_vbios {
        __u8 name[64];
        __u32 dbdf;
        __u8 vbios_pn[64];
        __u32 version;
        __u8 date[32];
        __u8 serial[16]; // jiawei: shall we delete this
        __u32 dev_id;
        __u32 rev_id;
        __u32 sub_dev_id;
        __u32 sub_ved_id;
};

serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's already a sysfs named serial_number, which exposes it already.

static ssize_t amdgpu_device_get_serial_number(struct device *dev,
                struct device_attribute *attr, char *buf)
{
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;

        return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
}

Thanks,
Jiawei


-----Original Message-----
From: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Sent: Monday, May 10, 2021 2:53 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

No, this structure contains all the details of the vbios: date, serial number, name, etc.

The sysfs node only contains the vbios name string

> On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> With a second thought,
> __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.
>
> Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> Deucher @Nieto, David M
>
> Best regards,
> Jiawei
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>
> Sent: Sunday, May 9, 2021 11:59 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
> <keescook@chromium.org><mailto:keescook@chromium.org>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com> wrote:
>>
>> 20 should be serial char size now instead of 16.
>>
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com>
>
> Please make sure this keeps proper 64 bit alignment in the structure.
>
> Alex
>
>
>> ---
>> include/uapi/drm/amdgpu_drm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
>> 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u8 serial[20];
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> t
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>> i
>> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>> 6
>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>> d
>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>> C
>> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>> e
>> rved=0


_______________________________________________

amd-gfx mailing list

amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>

https://lists.freedesktop.org/mailman/listinfo/amd-gfx<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7C3c0071a8a2a74127027408d913ba53e4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637562513264718308%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=g11aunC78b48N19Q%2FFXJBK%2B2Z0PZBWdqp%2FzhsPiqQ8Q%3D&reserved=0>


[-- Attachment #1.2: Type: text/html, Size: 15953 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-11  2:23               ` Gu, JiaWei (Will)
@ 2021-05-11  2:52                 ` Nieto, David M
  2021-05-11  3:01                   ` Gu, JiaWei (Will)
  0 siblings, 1 reply; 18+ messages in thread
From: Nieto, David M @ 2021-05-11  2:52 UTC (permalink / raw)
  To: Gu, JiaWei (Will), Christian König
  Cc: Alex Deucher, Deng, Emily, Kees Cook, amd-gfx list


[-- Attachment #1.1: Type: text/plain, Size: 8833 bytes --]

[AMD Official Use Only - Internal Distribution Only]

I agree that the serial number should be on number form, but I think we are still missing one field, which is the vbios name, which is located after the P/N, ASIC, PCI and memory type strings (skiping 0xD 0xA

David
________________________________
From: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
Sent: Monday, May 10, 2021 7:23 PM
To: Nieto, David M <David.Nieto@amd.com>; Christian König <ckoenig.leichtzumerken@gmail.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; Deng, Emily <Emily.Deng@amd.com>; Kees Cook <keescook@chromium.org>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios


[AMD Official Use Only - Internal Distribution Only]



Got it. Let’s keep them both.



Another idea about drm_amdgpu_info_vbios is

Does it make more sense to fill the “serial info” with uint64_t adev->unique_id, instead of the current char[] in adev->serial?



Sorry about that I was not aware of adev->unique_id exists when I defined drm_amdgpu_info_vbios.

I think it’s clearer to use original numeric variable than a string to expose serial.



How about that?



>> struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u64 serial;
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --



Best regards,

Jiawei





From: Nieto, David M <David.Nieto@amd.com>
Sent: Tuesday, May 11, 2021 4:20 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; Deng, Emily <Emily.Deng@amd.com>; Kees Cook <keescook@chromium.org>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



One of the primary usecases is to add this information to the renderer string, I am not sure if there are other cases of UMD drivers accessing sysfs nodes, but I think if we think permissions, if a client is authenticated and opens the render device then it can use the IOCTL, it is unclear to me we can make a such an assumption for sysfs nodes…



I think there is value in having both tbh.



Regards,

David



From: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>
Date: Monday, May 10, 2021 at 6:48 AM
To: "Nieto, David M" <David.Nieto@amd.com<mailto:David.Nieto@amd.com>>, "Gu, JiaWei (Will)" <JiaWei.Gu@amd.com<mailto:JiaWei.Gu@amd.com>>
Cc: Alex Deucher <alexdeucher@gmail.com<mailto:alexdeucher@gmail.com>>, "Deng, Emily" <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>, Kees Cook <keescook@chromium.org<mailto:keescook@chromium.org>>, amd-gfx list <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



Well we could add both as sysfs file(s).

Question here is rather what is the primary use case of this and if the application has the necessary access permissions to the sysfs files?

Regards,
Christian.

Am 10.05.21 um 15:42 schrieb Nieto, David M:

Then the application would need to issue the ioctl and then open a sysfs file to get all the information it needs. It makes little sense from a programming perspective to add an incomplete interface in my opinion



________________________________

From: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Sent: Monday, May 10, 2021 12:13:07 AM
To: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



[AMD Official Use Only - Internal Distribution Only]

Hi David,

What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios, not the whole struct.

struct drm_amdgpu_info_vbios {
        __u8 name[64];
        __u32 dbdf;
        __u8 vbios_pn[64];
        __u32 version;
        __u8 date[32];
        __u8 serial[16]; // jiawei: shall we delete this
        __u32 dev_id;
        __u32 rev_id;
        __u32 sub_dev_id;
        __u32 sub_ved_id;
};

serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's already a sysfs named serial_number, which exposes it already.

static ssize_t amdgpu_device_get_serial_number(struct device *dev,
                struct device_attribute *attr, char *buf)
{
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;

        return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
}

Thanks,
Jiawei


-----Original Message-----
From: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Sent: Monday, May 10, 2021 2:53 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

No, this structure contains all the details of the vbios: date, serial number, name, etc.

The sysfs node only contains the vbios name string

> On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> With a second thought,
> __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.
>
> Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> Deucher @Nieto, David M
>
> Best regards,
> Jiawei
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>
> Sent: Sunday, May 9, 2021 11:59 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
> <keescook@chromium.org><mailto:keescook@chromium.org>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com> wrote:
>>
>> 20 should be serial char size now instead of 16.
>>
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com>
>
> Please make sure this keeps proper 64 bit alignment in the structure.
>
> Alex
>
>
>> ---
>> include/uapi/drm/amdgpu_drm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
>> 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u8 serial[20];
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> t
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>> i
>> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>> 6
>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>> d
>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>> C
>> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>> e
>> rved=0



_______________________________________________

amd-gfx mailing list

amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>

https://lists.freedesktop.org/mailman/listinfo/amd-gfx<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7C3c0071a8a2a74127027408d913ba53e4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637562513264718308%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=g11aunC78b48N19Q%2FFXJBK%2B2Z0PZBWdqp%2FzhsPiqQ8Q%3D&reserved=0>



[-- Attachment #1.2: Type: text/html, Size: 15919 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-11  2:52                 ` Nieto, David M
@ 2021-05-11  3:01                   ` Gu, JiaWei (Will)
  0 siblings, 0 replies; 18+ messages in thread
From: Gu, JiaWei (Will) @ 2021-05-11  3:01 UTC (permalink / raw)
  To: Nieto, David M, Christian König
  Cc: Alex Deucher, Deng, Emily, Kees Cook, amd-gfx list


[-- Attachment #1.1: Type: text/plain, Size: 10263 bytes --]

[AMD Official Use Only - Internal Distribution Only]

Hi David,

The snippet of code we posted here is truncated.
Here's the complete current struct:

struct drm_amdgpu_info_vbios {
               __u8 name[64];
               __u32 dbdf;
               __u8 vbios_pn[64];
               __u32 version;
               __u8 date[32];
               __u8 serial[16];
               __u32 dev_id;
               __u32 rev_id;
               __u32 sub_dev_id;
               __u32 sub_ved_id;
};

We included vbios name already.

Best regards,
Jiawei

From: Nieto, David M <David.Nieto@amd.com>
Sent: Tuesday, May 11, 2021 10:52 AM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; Christian König <ckoenig.leichtzumerken@gmail.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; Deng, Emily <Emily.Deng@amd.com>; Kees Cook <keescook@chromium.org>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios


[AMD Official Use Only - Internal Distribution Only]

I agree that the serial number should be on number form, but I think we are still missing one field, which is the vbios name, which is located after the P/N, ASIC, PCI and memory type strings (skiping 0xD 0xA

David
________________________________
From: Gu, JiaWei (Will) <JiaWei.Gu@amd.com<mailto:JiaWei.Gu@amd.com>>
Sent: Monday, May 10, 2021 7:23 PM
To: Nieto, David M <David.Nieto@amd.com<mailto:David.Nieto@amd.com>>; Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>
Cc: Alex Deucher <alexdeucher@gmail.com<mailto:alexdeucher@gmail.com>>; Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>; Kees Cook <keescook@chromium.org<mailto:keescook@chromium.org>>; amd-gfx list <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios


[AMD Official Use Only - Internal Distribution Only]



Got it. Let's keep them both.



Another idea about drm_amdgpu_info_vbios is

Does it make more sense to fill the "serial info" with uint64_t adev->unique_id, instead of the current char[] in adev->serial?



Sorry about that I was not aware of adev->unique_id exists when I defined drm_amdgpu_info_vbios.

I think it's clearer to use original numeric variable than a string to expose serial.



How about that?



>> struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u64 serial;
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --



Best regards,

Jiawei





From: Nieto, David M <David.Nieto@amd.com<mailto:David.Nieto@amd.com>>
Sent: Tuesday, May 11, 2021 4:20 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>; Gu, JiaWei (Will) <JiaWei.Gu@amd.com<mailto:JiaWei.Gu@amd.com>>
Cc: Alex Deucher <alexdeucher@gmail.com<mailto:alexdeucher@gmail.com>>; Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>; Kees Cook <keescook@chromium.org<mailto:keescook@chromium.org>>; amd-gfx list <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



One of the primary usecases is to add this information to the renderer string, I am not sure if there are other cases of UMD drivers accessing sysfs nodes, but I think if we think permissions, if a client is authenticated and opens the render device then it can use the IOCTL, it is unclear to me we can make a such an assumption for sysfs nodes...



I think there is value in having both tbh.



Regards,

David



From: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>
Date: Monday, May 10, 2021 at 6:48 AM
To: "Nieto, David M" <David.Nieto@amd.com<mailto:David.Nieto@amd.com>>, "Gu, JiaWei (Will)" <JiaWei.Gu@amd.com<mailto:JiaWei.Gu@amd.com>>
Cc: Alex Deucher <alexdeucher@gmail.com<mailto:alexdeucher@gmail.com>>, "Deng, Emily" <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>, Kees Cook <keescook@chromium.org<mailto:keescook@chromium.org>>, amd-gfx list <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



Well we could add both as sysfs file(s).

Question here is rather what is the primary use case of this and if the application has the necessary access permissions to the sysfs files?

Regards,
Christian.

Am 10.05.21 um 15:42 schrieb Nieto, David M:

Then the application would need to issue the ioctl and then open a sysfs file to get all the information it needs. It makes little sense from a programming perspective to add an incomplete interface in my opinion



________________________________

From: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Sent: Monday, May 10, 2021 12:13:07 AM
To: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



[AMD Official Use Only - Internal Distribution Only]

Hi David,

What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios, not the whole struct.

struct drm_amdgpu_info_vbios {
        __u8 name[64];
        __u32 dbdf;
        __u8 vbios_pn[64];
        __u32 version;
        __u8 date[32];
        __u8 serial[16]; // jiawei: shall we delete this
        __u32 dev_id;
        __u32 rev_id;
        __u32 sub_dev_id;
        __u32 sub_ved_id;
};

serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's already a sysfs named serial_number, which exposes it already.

static ssize_t amdgpu_device_get_serial_number(struct device *dev,
                struct device_attribute *attr, char *buf)
{
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;

        return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
}

Thanks,
Jiawei


-----Original Message-----
From: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Sent: Monday, May 10, 2021 2:53 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

No, this structure contains all the details of the vbios: date, serial number, name, etc.

The sysfs node only contains the vbios name string

> On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> With a second thought,
> __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.
>
> Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> Deucher @Nieto, David M
>
> Best regards,
> Jiawei
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>
> Sent: Sunday, May 9, 2021 11:59 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
> <keescook@chromium.org><mailto:keescook@chromium.org>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com> wrote:
>>
>> 20 should be serial char size now instead of 16.
>>
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com>
>
> Please make sure this keeps proper 64 bit alignment in the structure.
>
> Alex
>
>
>> ---
>> include/uapi/drm/amdgpu_drm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
>> 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u8 serial[20];
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> t
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>> i
>> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>> 6
>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>> d
>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>> C
>> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>> e
>> rved=0



_______________________________________________

amd-gfx mailing list

amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>

https://lists.freedesktop.org/mailman/listinfo/amd-gfx<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7C3c0071a8a2a74127027408d913ba53e4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637562513264718308%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=g11aunC78b48N19Q%2FFXJBK%2B2Z0PZBWdqp%2FzhsPiqQ8Q%3D&reserved=0>



[-- Attachment #1.2: Type: text/html, Size: 21703 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-10 20:19             ` Nieto, David M
  2021-05-11  2:23               ` Gu, JiaWei (Will)
@ 2021-05-11  7:51               ` Christian König
  2021-05-11  8:18                 ` Marek Olšák
  1 sibling, 1 reply; 18+ messages in thread
From: Christian König @ 2021-05-11  7:51 UTC (permalink / raw)
  To: Nieto, David M, Gu, JiaWei (Will), Olsak, Marek
  Cc: Alex Deucher, Deng, Emily, Kees Cook, amd-gfx list


[-- Attachment #1.1: Type: text/plain, Size: 8185 bytes --]

Marek and other userspace folks need to decide that.

Basic question here is if Mesa is already accessing sysfs nodes for 
OpenGL or RADV. If that is the case then we should probably expose the 
information there as well.

If that isn't the case (which I think it is) then we should implement it 
as IOCTL.

Regards,
Christian.

Am 10.05.21 um 22:19 schrieb Nieto, David M:
>
> One of the primary usecases is to add this information to the renderer 
> string, I am not sure if there are other cases of UMD drivers 
> accessing sysfs nodes, but I think if we think permissions, if a 
> client is authenticated and opens the render device then it can use 
> the IOCTL, it is unclear to me we can make a such an assumption for 
> sysfs nodes…
>
> I think there is value in having both tbh.
>
> Regards,
>
> David
>
> *From: *Christian König <ckoenig.leichtzumerken@gmail.com>
> *Date: *Monday, May 10, 2021 at 6:48 AM
> *To: *"Nieto, David M" <David.Nieto@amd.com>, "Gu, JiaWei (Will)" 
> <JiaWei.Gu@amd.com>
> *Cc: *Alex Deucher <alexdeucher@gmail.com>, "Deng, Emily" 
> <Emily.Deng@amd.com>, Kees Cook <keescook@chromium.org>, amd-gfx list 
> <amd-gfx@lists.freedesktop.org>
> *Subject: *Re: [PATCH] drm/amdgpu: Align serial size in 
> drm_amdgpu_info_vbios
>
> Well we could add both as sysfs file(s).
>
> Question here is rather what is the primary use case of this and if 
> the application has the necessary access permissions to the sysfs files?
>
> Regards,
> Christian.
>
> Am 10.05.21 um 15:42 schrieb Nieto, David M:
>
>     Then the application would need to issue the ioctl and then open a
>     sysfs file to get all the information it needs. It makes little
>     sense from a programming perspective to add an incomplete
>     interface in my opinion
>
>     ------------------------------------------------------------------------
>
>     *From:*Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
>     <mailto:JiaWei.Gu@amd.com>
>     *Sent:* Monday, May 10, 2021 12:13:07 AM
>     *To:* Nieto, David M <David.Nieto@amd.com>
>     <mailto:David.Nieto@amd.com>
>     *Cc:* Alex Deucher <alexdeucher@gmail.com>
>     <mailto:alexdeucher@gmail.com>; amd-gfx list
>     <amd-gfx@lists.freedesktop.org>
>     <mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
>     <keescook@chromium.org> <mailto:keescook@chromium.org>; Deng,
>     Emily <Emily.Deng@amd.com> <mailto:Emily.Deng@amd.com>
>     *Subject:* RE: [PATCH] drm/amdgpu: Align serial size in
>     drm_amdgpu_info_vbios
>
>     [AMD Official Use Only - Internal Distribution Only]
>
>     Hi David,
>
>     What I meant is to ONLY delete the serial[16] from
>     drm_amdgpu_info_vbios, not the whole struct.
>
>     struct drm_amdgpu_info_vbios {
>             __u8 name[64];
>             __u32 dbdf;
>             __u8 vbios_pn[64];
>             __u32 version;
>             __u8 date[32];
>             __u8 serial[16]; // jiawei: shall we delete this
>             __u32 dev_id;
>             __u32 rev_id;
>             __u32 sub_dev_id;
>             __u32 sub_ved_id;
>     };
>
>     serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but
>     there's already a sysfs named serial_number, which exposes it already.
>
>     static ssize_t amdgpu_device_get_serial_number(struct device *dev,
>                     struct device_attribute *attr, char *buf)
>     {
>             struct drm_device *ddev = dev_get_drvdata(dev);
>             struct amdgpu_device *adev = ddev->dev_private;
>
>             return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
>     }
>
>     Thanks,
>     Jiawei
>
>
>     -----Original Message-----
>     From: Nieto, David M <David.Nieto@amd.com>
>     <mailto:David.Nieto@amd.com>
>     Sent: Monday, May 10, 2021 2:53 PM
>     To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <mailto:JiaWei.Gu@amd.com>
>     Cc: Alex Deucher <alexdeucher@gmail.com>
>     <mailto:alexdeucher@gmail.com>; amd-gfx list
>     <amd-gfx@lists.freedesktop.org>
>     <mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
>     <keescook@chromium.org> <mailto:keescook@chromium.org>; Deng,
>     Emily <Emily.Deng@amd.com> <mailto:Emily.Deng@amd.com>
>     Subject: Re: [PATCH] drm/amdgpu: Align serial size in
>     drm_amdgpu_info_vbios
>
>     No, this structure contains all the details of the vbios: date,
>     serial number, name, etc.
>
>     The sysfs node only contains the vbios name string
>
>     > On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
>     <mailto:JiaWei.Gu@amd.com> wrote:
>     >
>     > [AMD Official Use Only - Internal Distribution Only]
>     >
>     > With a second thought,
>     > __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant,
>     sysfs serial_number already exposes it.
>     >
>     > Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
>     > Deucher @Nieto, David M
>     >
>     > Best regards,
>     > Jiawei
>     >
>     > -----Original Message-----
>     > From: Alex Deucher <alexdeucher@gmail.com>
>     <mailto:alexdeucher@gmail.com>
>     > Sent: Sunday, May 9, 2021 11:59 PM
>     > To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <mailto:JiaWei.Gu@amd.com>
>     > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>     <mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
>     > <keescook@chromium.org> <mailto:keescook@chromium.org>
>     > Subject: Re: [PATCH] drm/amdgpu: Align serial size in
>     > drm_amdgpu_info_vbios
>     >
>     >> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com>
>     <mailto:Jiawei.Gu@amd.com> wrote:
>     >>
>     >> 20 should be serial char size now instead of 16.
>     >>
>     >> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
>     <mailto:Jiawei.Gu@amd.com>
>     >
>     > Please make sure this keeps proper 64 bit alignment in the
>     structure.
>     >
>     > Alex
>     >
>     >
>     >> ---
>     >> include/uapi/drm/amdgpu_drm.h | 2 +-
>     >> 1 file changed, 1 insertion(+), 1 deletion(-)
>     >>
>     >> diff --git a/include/uapi/drm/amdgpu_drm.h
>     >> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
>     >> 100644
>     >> --- a/include/uapi/drm/amdgpu_drm.h
>     >> +++ b/include/uapi/drm/amdgpu_drm.h
>     >> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>     >>        __u8 vbios_pn[64];
>     >>        __u32 version;
>     >>        __u8 date[32];
>     >> -       __u8 serial[16];
>     >> +       __u8 serial[20];
>     >>        __u32 dev_id;
>     >>        __u32 rev_id;
>     >>        __u32 sub_dev_id;
>     >> --
>     >> 2.17.1
>     >>
>     >> _______________________________________________
>     >> amd-gfx mailing list
>     >> amd-gfx@lists.freedesktop.org
>     <mailto:amd-gfx@lists.freedesktop.org>
>     >>
>     https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>     <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis>
>     >> t
>     >>
>     s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>     >> i
>     >>
>     awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>     >> 6
>     >>
>     08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>     >> d
>     >>
>     8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>     >> C
>     >>
>     1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>     >> e
>     >> rved=0
>
>
>
>     _______________________________________________
>
>     amd-gfx mailing list
>
>     amd-gfx@lists.freedesktop.org  <mailto:amd-gfx@lists.freedesktop.org>
>
>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx  <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7C3c0071a8a2a74127027408d913ba53e4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637562513264718308%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=g11aunC78b48N19Q%2FFXJBK%2B2Z0PZBWdqp%2FzhsPiqQ8Q%3D&reserved=0>
>
>
>


[-- Attachment #1.2: Type: text/html, Size: 17288 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-11  7:51               ` Christian König
@ 2021-05-11  8:18                 ` Marek Olšák
  2021-05-11 14:04                   ` Deucher, Alexander
  0 siblings, 1 reply; 18+ messages in thread
From: Marek Olšák @ 2021-05-11  8:18 UTC (permalink / raw)
  To: Christian König
  Cc: Kees Cook, Gu, JiaWei (Will),
	amd-gfx list, Deng, Emily, Alex Deucher, Nieto, David M


[-- Attachment #1.1: Type: text/plain, Size: 8062 bytes --]

Mesa doesn't use sysfs.

Note that this is a uapi, meaning that once it's in the kernel, it can't be
changed like that.

What's the use case for this new interface? Isn't it partially redundant
with the current device info structure, which seems to have the equivalent
of dev_id and rev_id?

Marek

On Tue, May 11, 2021 at 3:51 AM Christian König <
ckoenig.leichtzumerken@gmail.com> wrote:

> Marek and other userspace folks need to decide that.
>
> Basic question here is if Mesa is already accessing sysfs nodes for OpenGL
> or RADV. If that is the case then we should probably expose the information
> there as well.
>
> If that isn't the case (which I think it is) then we should implement it
> as IOCTL.
>
> Regards,
> Christian.
>
> Am 10.05.21 um 22:19 schrieb Nieto, David M:
>
> One of the primary usecases is to add this information to the renderer
> string, I am not sure if there are other cases of UMD drivers accessing
> sysfs nodes, but I think if we think permissions, if a client is
> authenticated and opens the render device then it can use the IOCTL, it is
> unclear to me we can make a such an assumption for sysfs nodes…
>
>
>
> I think there is value in having both tbh.
>
>
>
> Regards,
>
> David
>
>
>
> *From: *Christian König <ckoenig.leichtzumerken@gmail.com>
> <ckoenig.leichtzumerken@gmail.com>
> *Date: *Monday, May 10, 2021 at 6:48 AM
> *To: *"Nieto, David M" <David.Nieto@amd.com> <David.Nieto@amd.com>, "Gu,
> JiaWei (Will)" <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> *Cc: *Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>,
> "Deng, Emily" <Emily.Deng@amd.com> <Emily.Deng@amd.com>, Kees Cook
> <keescook@chromium.org> <keescook@chromium.org>, amd-gfx list
> <amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org>
> *Subject: *Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>
>
> Well we could add both as sysfs file(s).
>
> Question here is rather what is the primary use case of this and if the
> application has the necessary access permissions to the sysfs files?
>
> Regards,
> Christian.
>
> Am 10.05.21 um 15:42 schrieb Nieto, David M:
>
> Then the application would need to issue the ioctl and then open a sysfs
> file to get all the information it needs. It makes little sense from a
> programming perspective to add an incomplete interface in my opinion
>
>
> ------------------------------
>
> *From:* Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> *Sent:* Monday, May 10, 2021 12:13:07 AM
> *To:* Nieto, David M <David.Nieto@amd.com> <David.Nieto@amd.com>
> *Cc:* Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>;
> amd-gfx list <amd-gfx@lists.freedesktop.org>
> <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>
> <keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com>
> <Emily.Deng@amd.com>
> *Subject:* RE: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>
>
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi David,
>
> What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios,
> not the whole struct.
>
> struct drm_amdgpu_info_vbios {
>         __u8 name[64];
>         __u32 dbdf;
>         __u8 vbios_pn[64];
>         __u32 version;
>         __u8 date[32];
>         __u8 serial[16]; // jiawei: shall we delete this
>         __u32 dev_id;
>         __u32 rev_id;
>         __u32 sub_dev_id;
>         __u32 sub_ved_id;
> };
>
> serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's
> already a sysfs named serial_number, which exposes it already.
>
> static ssize_t amdgpu_device_get_serial_number(struct device *dev,
>                 struct device_attribute *attr, char *buf)
> {
>         struct drm_device *ddev = dev_get_drvdata(dev);
>         struct amdgpu_device *adev = ddev->dev_private;
>
>         return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
> }
>
> Thanks,
> Jiawei
>
>
> -----Original Message-----
> From: Nieto, David M <David.Nieto@amd.com> <David.Nieto@amd.com>
> Sent: Monday, May 10, 2021 2:53 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> Cc: Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>; amd-gfx
> list <amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org>;
> Kees Cook <keescook@chromium.org> <keescook@chromium.org>; Deng, Emily
> <Emily.Deng@amd.com> <Emily.Deng@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
>
> No, this structure contains all the details of the vbios: date, serial
> number, name, etc.
>
> The sysfs node only contains the vbios name string
>
> > On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
> <JiaWei.Gu@amd.com> wrote:
> >
> > [AMD Official Use Only - Internal Distribution Only]
> >
> > With a second thought,
> > __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs
> serial_number already exposes it.
> >
> > Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> > Deucher @Nieto, David M
> >
> > Best regards,
> > Jiawei
> >
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>
> > Sent: Sunday, May 9, 2021 11:59 PM
> > To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> <amd-gfx@lists.freedesktop.org>; Kees Cook
> > <keescook@chromium.org> <keescook@chromium.org>
> > Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> > drm_amdgpu_info_vbios
> >
> >> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com>
> <Jiawei.Gu@amd.com> wrote:
> >>
> >> 20 should be serial char size now instead of 16.
> >>
> >> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com> <Jiawei.Gu@amd.com>
> >
> > Please make sure this keeps proper 64 bit alignment in the structure.
> >
> > Alex
> >
> >
> >> ---
> >> include/uapi/drm/amdgpu_drm.h | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/uapi/drm/amdgpu_drm.h
> >> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
> >> 100644
> >> --- a/include/uapi/drm/amdgpu_drm.h
> >> +++ b/include/uapi/drm/amdgpu_drm.h
> >> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
> >>        __u8 vbios_pn[64];
> >>        __u32 version;
> >>        __u8 date[32];
> >> -       __u8 serial[16];
> >> +       __u8 serial[20];
> >>        __u32 dev_id;
> >>        __u32 rev_id;
> >>        __u32 sub_dev_id;
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> >> t
> >> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
> >> i
> >> awei.Gu%40amd.com%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
> >> 6
> >> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
> >> d
> >> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> >> C
> >> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
> >> e
> >> rved=0
>
>
>
> _______________________________________________
>
> amd-gfx mailing list
>
> amd-gfx@lists.freedesktop.org
>
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7C3c0071a8a2a74127027408d913ba53e4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637562513264718308%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=g11aunC78b48N19Q%2FFXJBK%2B2Z0PZBWdqp%2FzhsPiqQ8Q%3D&reserved=0>
>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 16259 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-11  8:18                 ` Marek Olšák
@ 2021-05-11 14:04                   ` Deucher, Alexander
  2021-05-11 14:07                     ` Christian König
  0 siblings, 1 reply; 18+ messages in thread
From: Deucher, Alexander @ 2021-05-11 14:04 UTC (permalink / raw)
  To: Marek Olšák, Christian König
  Cc: Kees Cook, Gu, JiaWei (Will),
	amd-gfx list, Deng,  Emily, Alex Deucher, Nieto, David M


[-- Attachment #1.1: Type: text/plain, Size: 9797 bytes --]

[AMD Public Use]

It's being used by umr and some other smi tools to provide vbios information for debugging.

Alex

________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Marek Olšák <maraeo@gmail.com>
Sent: Tuesday, May 11, 2021 4:18 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com>
Cc: Kees Cook <keescook@chromium.org>; Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; Deng, Emily <Emily.Deng@amd.com>; Alex Deucher <alexdeucher@gmail.com>; Nieto, David M <David.Nieto@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

Mesa doesn't use sysfs.

Note that this is a uapi, meaning that once it's in the kernel, it can't be changed like that.

What's the use case for this new interface? Isn't it partially redundant with the current device info structure, which seems to have the equivalent of dev_id and rev_id?

Marek

On Tue, May 11, 2021 at 3:51 AM Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>> wrote:
Marek and other userspace folks need to decide that.

Basic question here is if Mesa is already accessing sysfs nodes for OpenGL or RADV. If that is the case then we should probably expose the information there as well.

If that isn't the case (which I think it is) then we should implement it as IOCTL.

Regards,
Christian.

Am 10.05.21 um 22:19 schrieb Nieto, David M:

One of the primary usecases is to add this information to the renderer string, I am not sure if there are other cases of UMD drivers accessing sysfs nodes, but I think if we think permissions, if a client is authenticated and opens the render device then it can use the IOCTL, it is unclear to me we can make a such an assumption for sysfs nodes…



I think there is value in having both tbh.



Regards,

David



From: Christian König <ckoenig.leichtzumerken@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com>
Date: Monday, May 10, 2021 at 6:48 AM
To: "Nieto, David M" <David.Nieto@amd.com><mailto:David.Nieto@amd.com>, "Gu, JiaWei (Will)" <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>, "Deng, Emily" <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>, Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>, amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



Well we could add both as sysfs file(s).

Question here is rather what is the primary use case of this and if the application has the necessary access permissions to the sysfs files?

Regards,
Christian.

Am 10.05.21 um 15:42 schrieb Nieto, David M:

Then the application would need to issue the ioctl and then open a sysfs file to get all the information it needs. It makes little sense from a programming perspective to add an incomplete interface in my opinion



________________________________

From: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Sent: Monday, May 10, 2021 12:13:07 AM
To: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



[AMD Official Use Only - Internal Distribution Only]

Hi David,

What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios, not the whole struct.

struct drm_amdgpu_info_vbios {
        __u8 name[64];
        __u32 dbdf;
        __u8 vbios_pn[64];
        __u32 version;
        __u8 date[32];
        __u8 serial[16]; // jiawei: shall we delete this
        __u32 dev_id;
        __u32 rev_id;
        __u32 sub_dev_id;
        __u32 sub_ved_id;
};

serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's already a sysfs named serial_number, which exposes it already.

static ssize_t amdgpu_device_get_serial_number(struct device *dev,
                struct device_attribute *attr, char *buf)
{
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;

        return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
}

Thanks,
Jiawei


-----Original Message-----
From: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Sent: Monday, May 10, 2021 2:53 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

No, this structure contains all the details of the vbios: date, serial number, name, etc.

The sysfs node only contains the vbios name string

> On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> With a second thought,
> __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.
>
> Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> Deucher @Nieto, David M
>
> Best regards,
> Jiawei
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>
> Sent: Sunday, May 9, 2021 11:59 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
> <keescook@chromium.org><mailto:keescook@chromium.org>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com> wrote:
>>
>> 20 should be serial char size now instead of 16.
>>
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com>
>
> Please make sure this keeps proper 64 bit alignment in the structure.
>
> Alex
>
>
>> ---
>> include/uapi/drm/amdgpu_drm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
>> 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u8 serial[20];
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> t
>> s.freedesktop.org<https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fs.freedesktop.org%2F&data=04%7C01%7Calexander.deucher%40amd.com%7C3503a51f2fa04376040b08d914558033%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563179729003008%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5c730zpnFFOm3EgabDFoiYlsl2tsaxImaTlVfap%2BfHQ%3D&reserved=0>%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>> i
>> awei.Gu%40amd.com<https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2F40amd.com%2F&data=04%7C01%7Calexander.deucher%40amd.com%7C3503a51f2fa04376040b08d914558033%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563179729003008%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=FzFHfe095%2FDzOYR%2BZ3tAzlF2wDRYsD5kOt%2Bf37nNVP4%3D&reserved=0>%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>> 6
>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>> d
>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>> C
>> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>> e
>> rved=0



_______________________________________________

amd-gfx mailing list

amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>

https://lists.freedesktop.org/mailman/listinfo/amd-gfx<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Calexander.deucher%40amd.com%7C3503a51f2fa04376040b08d914558033%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563179729012969%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GA4tfIXNrbU98WUMQl%2Bkd28DyNgqjlZcIAryQtVIn%2Bw%3D&reserved=0>



_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Calexander.deucher%40amd.com%7C3503a51f2fa04376040b08d914558033%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563179729012969%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GA4tfIXNrbU98WUMQl%2Bkd28DyNgqjlZcIAryQtVIn%2Bw%3D&reserved=0>

[-- Attachment #1.2: Type: text/html, Size: 17207 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-11 14:04                   ` Deucher, Alexander
@ 2021-05-11 14:07                     ` Christian König
  2021-05-11 16:35                       ` Nieto, David M
  0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2021-05-11 14:07 UTC (permalink / raw)
  To: Deucher, Alexander, Marek Olšák
  Cc: Kees Cook, Gu, JiaWei (Will),
	amd-gfx list, Deng, Emily, Alex Deucher, Nieto, David M


[-- Attachment #1.1: Type: text/plain, Size: 12236 bytes --]

Yeah, but umr is making strong use of sysfs as well.

The only justification of this interface would be if we want to use it 
in Mesa.

And I agree with Marek that looks redundant with the device structure to 
me as well.

Christian.

Am 11.05.21 um 16:04 schrieb Deucher, Alexander:
>
> [AMD Public Use]
>
>
> It's being used by umr and some other smi tools to provide vbios 
> information for debugging.
>
> Alex
>
> ------------------------------------------------------------------------
> *From:* amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of 
> Marek Olšák <maraeo@gmail.com>
> *Sent:* Tuesday, May 11, 2021 4:18 AM
> *To:* Christian König <ckoenig.leichtzumerken@gmail.com>
> *Cc:* Kees Cook <keescook@chromium.org>; Gu, JiaWei (Will) 
> <JiaWei.Gu@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; 
> Deng, Emily <Emily.Deng@amd.com>; Alex Deucher 
> <alexdeucher@gmail.com>; Nieto, David M <David.Nieto@amd.com>
> *Subject:* Re: [PATCH] drm/amdgpu: Align serial size in 
> drm_amdgpu_info_vbios
> Mesa doesn't use sysfs.
>
> Note that this is a uapi, meaning that once it's in the kernel, it 
> can't be changed like that.
>
> What's the use case for this new interface? Isn't it partially 
> redundant with the current device info structure, which seems to have 
> the equivalent of dev_id and rev_id?
>
> Marek
>
> On Tue, May 11, 2021 at 3:51 AM Christian König 
> <ckoenig.leichtzumerken@gmail.com 
> <mailto:ckoenig.leichtzumerken@gmail.com>> wrote:
>
>     Marek and other userspace folks need to decide that.
>
>     Basic question here is if Mesa is already accessing sysfs nodes
>     for OpenGL or RADV. If that is the case then we should probably
>     expose the information there as well.
>
>     If that isn't the case (which I think it is) then we should
>     implement it as IOCTL.
>
>     Regards,
>     Christian.
>
>     Am 10.05.21 um 22:19 schrieb Nieto, David M:
>>
>>     One of the primary usecases is to add this information to the
>>     renderer string, I am not sure if there are other cases of UMD
>>     drivers accessing sysfs nodes, but I think if we think
>>     permissions, if a client is authenticated and opens the render
>>     device then it can use the IOCTL, it is unclear to me we can make
>>     a such an assumption for sysfs nodes…
>>
>>     I think there is value in having both tbh.
>>
>>     Regards,
>>
>>     David
>>
>>     *From: *Christian König <ckoenig.leichtzumerken@gmail.com>
>>     <mailto:ckoenig.leichtzumerken@gmail.com>
>>     *Date: *Monday, May 10, 2021 at 6:48 AM
>>     *To: *"Nieto, David M" <David.Nieto@amd.com>
>>     <mailto:David.Nieto@amd.com>, "Gu, JiaWei (Will)"
>>     <JiaWei.Gu@amd.com> <mailto:JiaWei.Gu@amd.com>
>>     *Cc: *Alex Deucher <alexdeucher@gmail.com>
>>     <mailto:alexdeucher@gmail.com>, "Deng, Emily"
>>     <Emily.Deng@amd.com> <mailto:Emily.Deng@amd.com>, Kees Cook
>>     <keescook@chromium.org> <mailto:keescook@chromium.org>, amd-gfx
>>     list <amd-gfx@lists.freedesktop.org>
>>     <mailto:amd-gfx@lists.freedesktop.org>
>>     *Subject: *Re: [PATCH] drm/amdgpu: Align serial size in
>>     drm_amdgpu_info_vbios
>>
>>     Well we could add both as sysfs file(s).
>>
>>     Question here is rather what is the primary use case of this and
>>     if the application has the necessary access permissions to the
>>     sysfs files?
>>
>>     Regards,
>>     Christian.
>>
>>     Am 10.05.21 um 15:42 schrieb Nieto, David M:
>>
>>         Then the application would need to issue the ioctl and then
>>         open a sysfs file to get all the information it needs. It
>>         makes little sense from a programming perspective to add an
>>         incomplete interface in my opinion
>>
>>         ------------------------------------------------------------------------
>>
>>         *From:*Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
>>         <mailto:JiaWei.Gu@amd.com>
>>         *Sent:* Monday, May 10, 2021 12:13:07 AM
>>         *To:* Nieto, David M <David.Nieto@amd.com>
>>         <mailto:David.Nieto@amd.com>
>>         *Cc:* Alex Deucher <alexdeucher@gmail.com>
>>         <mailto:alexdeucher@gmail.com>; amd-gfx list
>>         <amd-gfx@lists.freedesktop.org>
>>         <mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
>>         <keescook@chromium.org> <mailto:keescook@chromium.org>; Deng,
>>         Emily <Emily.Deng@amd.com> <mailto:Emily.Deng@amd.com>
>>         *Subject:* RE: [PATCH] drm/amdgpu: Align serial size in
>>         drm_amdgpu_info_vbios
>>
>>         [AMD Official Use Only - Internal Distribution Only]
>>
>>         Hi David,
>>
>>         What I meant is to ONLY delete the serial[16] from
>>         drm_amdgpu_info_vbios, not the whole struct.
>>
>>         struct drm_amdgpu_info_vbios {
>>                 __u8 name[64];
>>                 __u32 dbdf;
>>                 __u8 vbios_pn[64];
>>                 __u32 version;
>>                 __u8 date[32];
>>                 __u8 serial[16]; // jiawei: shall we delete this
>>                 __u32 dev_id;
>>                 __u32 rev_id;
>>                 __u32 sub_dev_id;
>>                 __u32 sub_ved_id;
>>         };
>>
>>         serial[16] in drm_amdgpu_info_vbios  copied from
>>         adev->serial, but there's already a sysfs named
>>         serial_number, which exposes it already.
>>
>>         static ssize_t amdgpu_device_get_serial_number(struct device
>>         *dev,
>>                         struct device_attribute *attr, char *buf)
>>         {
>>                 struct drm_device *ddev = dev_get_drvdata(dev);
>>                 struct amdgpu_device *adev = ddev->dev_private;
>>
>>                 return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
>>         }
>>
>>         Thanks,
>>         Jiawei
>>
>>
>>         -----Original Message-----
>>         From: Nieto, David M <David.Nieto@amd.com>
>>         <mailto:David.Nieto@amd.com>
>>         Sent: Monday, May 10, 2021 2:53 PM
>>         To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
>>         <mailto:JiaWei.Gu@amd.com>
>>         Cc: Alex Deucher <alexdeucher@gmail.com>
>>         <mailto:alexdeucher@gmail.com>; amd-gfx list
>>         <amd-gfx@lists.freedesktop.org>
>>         <mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
>>         <keescook@chromium.org> <mailto:keescook@chromium.org>; Deng,
>>         Emily <Emily.Deng@amd.com> <mailto:Emily.Deng@amd.com>
>>         Subject: Re: [PATCH] drm/amdgpu: Align serial size in
>>         drm_amdgpu_info_vbios
>>
>>         No, this structure contains all the details of the vbios:
>>         date, serial number, name, etc.
>>
>>         The sysfs node only contains the vbios name string
>>
>>         > On May 9, 2021, at 23:33, Gu, JiaWei (Will)
>>         <JiaWei.Gu@amd.com> <mailto:JiaWei.Gu@amd.com> wrote:
>>         >
>>         > [AMD Official Use Only - Internal Distribution Only]
>>         >
>>         > With a second thought,
>>         > __u8 serial[16] in drm_amdgpu_info_vbios is a bit
>>         redundant, sysfs serial_number already exposes it.
>>         >
>>         > Is it fine to abandon it from drm_amdgpu_info_vbios struct?
>>         @Alex
>>         > Deucher @Nieto, David M
>>         >
>>         > Best regards,
>>         > Jiawei
>>         >
>>         > -----Original Message-----
>>         > From: Alex Deucher <alexdeucher@gmail.com>
>>         <mailto:alexdeucher@gmail.com>
>>         > Sent: Sunday, May 9, 2021 11:59 PM
>>         > To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
>>         <mailto:JiaWei.Gu@amd.com>
>>         > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>         <mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
>>         > <keescook@chromium.org> <mailto:keescook@chromium.org>
>>         > Subject: Re: [PATCH] drm/amdgpu: Align serial size in
>>         > drm_amdgpu_info_vbios
>>         >
>>         >> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu
>>         <Jiawei.Gu@amd.com> <mailto:Jiawei.Gu@amd.com> wrote:
>>         >>
>>         >> 20 should be serial char size now instead of 16.
>>         >>
>>         >> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
>>         <mailto:Jiawei.Gu@amd.com>
>>         >
>>         > Please make sure this keeps proper 64 bit alignment in the
>>         structure.
>>         >
>>         > Alex
>>         >
>>         >
>>         >> ---
>>         >> include/uapi/drm/amdgpu_drm.h | 2 +-
>>         >> 1 file changed, 1 insertion(+), 1 deletion(-)
>>         >>
>>         >> diff --git a/include/uapi/drm/amdgpu_drm.h
>>         >> b/include/uapi/drm/amdgpu_drm.h index
>>         2b487a8d2727..1c20721f90da
>>         >> 100644
>>         >> --- a/include/uapi/drm/amdgpu_drm.h
>>         >> +++ b/include/uapi/drm/amdgpu_drm.h
>>         >> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>         >>        __u8 vbios_pn[64];
>>         >>        __u32 version;
>>         >>        __u8 date[32];
>>         >> -       __u8 serial[16];
>>         >> +       __u8 serial[20];
>>         >>        __u32 dev_id;
>>         >>        __u32 rev_id;
>>         >>        __u32 sub_dev_id;
>>         >> --
>>         >> 2.17.1
>>         >>
>>         >> _______________________________________________
>>         >> amd-gfx mailing list
>>         >> amd-gfx@lists.freedesktop.org
>>         <mailto:amd-gfx@lists.freedesktop.org>
>>         >>
>>         https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>>         <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis>
>>         >> t
>>         >> s.freedesktop.org
>>         <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fs.freedesktop.org%2F&data=04%7C01%7Calexander.deucher%40amd.com%7C3503a51f2fa04376040b08d914558033%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563179729003008%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5c730zpnFFOm3EgabDFoiYlsl2tsaxImaTlVfap%2BfHQ%3D&reserved=0>%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>>         >> i
>>         >> awei.Gu%40amd.com
>>         <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2F40amd.com%2F&data=04%7C01%7Calexander.deucher%40amd.com%7C3503a51f2fa04376040b08d914558033%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563179729003008%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=FzFHfe095%2FDzOYR%2BZ3tAzlF2wDRYsD5kOt%2Bf37nNVP4%3D&reserved=0>%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>>         >> 6
>>         >>
>>         08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>>         >> d
>>         >>
>>         8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>>         >> C
>>         >>
>>         1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>>         >> e
>>         >> rved=0
>>
>>
>>
>>         _______________________________________________
>>
>>         amd-gfx mailing list
>>
>>         amd-gfx@lists.freedesktop.org  <mailto:amd-gfx@lists.freedesktop.org>
>>
>>         https://lists.freedesktop.org/mailman/listinfo/amd-gfx  <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Calexander.deucher%40amd.com%7C3503a51f2fa04376040b08d914558033%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563179729012969%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GA4tfIXNrbU98WUMQl%2Bkd28DyNgqjlZcIAryQtVIn%2Bw%3D&reserved=0>
>>
>>
>>
>
>     _______________________________________________
>     amd-gfx mailing list
>     amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>     <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Calexander.deucher%40amd.com%7C3503a51f2fa04376040b08d914558033%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563179729012969%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GA4tfIXNrbU98WUMQl%2Bkd28DyNgqjlZcIAryQtVIn%2Bw%3D&reserved=0>
>


[-- Attachment #1.2: Type: text/html, Size: 27801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-11 14:07                     ` Christian König
@ 2021-05-11 16:35                       ` Nieto, David M
  2021-05-15  6:40                         ` Marek Olšák
  0 siblings, 1 reply; 18+ messages in thread
From: Nieto, David M @ 2021-05-11 16:35 UTC (permalink / raw)
  To: Christian König, Deucher, Alexander, Marek Olšák
  Cc: Alex Deucher, Deng, Emily, Gu, JiaWei (Will), Kees Cook, amd-gfx list


[-- Attachment #1.1: Type: text/plain, Size: 11610 bytes --]

[AMD Public Use]

The point of having the device ID in the structure is because we are reading it from the VBIOS header, not the asic registers. They should match, but an user may flash a VBIOS for a different devid and they may not match.

Regarding sysfs vs ioctl I see value in providing it in both ways, Mesa uses IOCTL and other DRM based tools may benefit as well. I recently went through the trouble of having to add sysfs string parsing for some data not available in ioctl, and while not very complicated, it is a programming inconvenience.

I understand that being uapi, changing it is not easy, but this is information extracted from a VBIOS header, something that has been kept stable for many years.

David
________________________________
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Tuesday, May 11, 2021 7:07 AM
To: Deucher, Alexander <Alexander.Deucher@amd.com>; Marek Olšák <maraeo@gmail.com>
Cc: Kees Cook <keescook@chromium.org>; Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; Deng, Emily <Emily.Deng@amd.com>; Alex Deucher <alexdeucher@gmail.com>; Nieto, David M <David.Nieto@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

Yeah, but umr is making strong use of sysfs as well.

The only justification of this interface would be if we want to use it in Mesa.

And I agree with Marek that looks redundant with the device structure to me as well.

Christian.

Am 11.05.21 um 16:04 schrieb Deucher, Alexander:

[AMD Public Use]

It's being used by umr and some other smi tools to provide vbios information for debugging.

Alex

________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org><mailto:amd-gfx-bounces@lists.freedesktop.org> on behalf of Marek Olšák <maraeo@gmail.com><mailto:maraeo@gmail.com>
Sent: Tuesday, May 11, 2021 4:18 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com>
Cc: Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>; Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

Mesa doesn't use sysfs.

Note that this is a uapi, meaning that once it's in the kernel, it can't be changed like that.

What's the use case for this new interface? Isn't it partially redundant with the current device info structure, which seems to have the equivalent of dev_id and rev_id?

Marek

On Tue, May 11, 2021 at 3:51 AM Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>> wrote:
Marek and other userspace folks need to decide that.

Basic question here is if Mesa is already accessing sysfs nodes for OpenGL or RADV. If that is the case then we should probably expose the information there as well.

If that isn't the case (which I think it is) then we should implement it as IOCTL.

Regards,
Christian.

Am 10.05.21 um 22:19 schrieb Nieto, David M:

One of the primary usecases is to add this information to the renderer string, I am not sure if there are other cases of UMD drivers accessing sysfs nodes, but I think if we think permissions, if a client is authenticated and opens the render device then it can use the IOCTL, it is unclear to me we can make a such an assumption for sysfs nodes…



I think there is value in having both tbh.



Regards,

David



From: Christian König <ckoenig.leichtzumerken@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com>
Date: Monday, May 10, 2021 at 6:48 AM
To: "Nieto, David M" <David.Nieto@amd.com><mailto:David.Nieto@amd.com>, "Gu, JiaWei (Will)" <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>, "Deng, Emily" <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>, Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>, amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



Well we could add both as sysfs file(s).

Question here is rather what is the primary use case of this and if the application has the necessary access permissions to the sysfs files?

Regards,
Christian.

Am 10.05.21 um 15:42 schrieb Nieto, David M:

Then the application would need to issue the ioctl and then open a sysfs file to get all the information it needs. It makes little sense from a programming perspective to add an incomplete interface in my opinion



________________________________

From: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Sent: Monday, May 10, 2021 12:13:07 AM
To: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: RE: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios



[AMD Official Use Only - Internal Distribution Only]

Hi David,

What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios, not the whole struct.

struct drm_amdgpu_info_vbios {
        __u8 name[64];
        __u32 dbdf;
        __u8 vbios_pn[64];
        __u32 version;
        __u8 date[32];
        __u8 serial[16]; // jiawei: shall we delete this
        __u32 dev_id;
        __u32 rev_id;
        __u32 sub_dev_id;
        __u32 sub_ved_id;
};

serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's already a sysfs named serial_number, which exposes it already.

static ssize_t amdgpu_device_get_serial_number(struct device *dev,
                struct device_attribute *attr, char *buf)
{
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;

        return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
}

Thanks,
Jiawei


-----Original Message-----
From: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>
Sent: Monday, May 10, 2021 2:53 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org><mailto:keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com><mailto:Emily.Deng@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

No, this structure contains all the details of the vbios: date, serial number, name, etc.

The sysfs node only contains the vbios name string

> On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> With a second thought,
> __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs serial_number already exposes it.
>
> Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> Deucher @Nieto, David M
>
> Best regards,
> Jiawei
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com><mailto:alexdeucher@gmail.com>
> Sent: Sunday, May 9, 2021 11:59 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com><mailto:JiaWei.Gu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
> <keescook@chromium.org><mailto:keescook@chromium.org>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com> wrote:
>>
>> 20 should be serial char size now instead of 16.
>>
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com><mailto:Jiawei.Gu@amd.com>
>
> Please make sure this keeps proper 64 bit alignment in the structure.
>
> Alex
>
>
>> ---
>> include/uapi/drm/amdgpu_drm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
>> 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>        __u8 vbios_pn[64];
>>        __u32 version;
>>        __u8 date[32];
>> -       __u8 serial[16];
>> +       __u8 serial[20];
>>        __u32 dev_id;
>>        __u32 rev_id;
>>        __u32 sub_dev_id;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> t
>> s.freedesktop.org<https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fs.freedesktop.org%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684906500%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gKeRCQ485gWespGfo1O01tZz8fDy1zPGHIDEvCqyp1I%3D&reserved=0>%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>> i
>> awei.Gu%40amd.com<https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2F40amd.com%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684916541%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8pM7WTh9%2F23KCF6DuXOc4%2Fi7rDUSxmH647BdpujXgWo%3D&reserved=0>%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>> 6
>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>> d
>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>> C
>> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>> e
>> rved=0



_______________________________________________

amd-gfx mailing list

amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>

https://lists.freedesktop.org/mailman/listinfo/amd-gfx<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684926488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Hdhx2ogsQt2C3OIKNC%2BaswtNkiIWbV6Ot8a%2B3YFqa7w%3D&reserved=0>



_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684926488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Hdhx2ogsQt2C3OIKNC%2BaswtNkiIWbV6Ot8a%2B3YFqa7w%3D&reserved=0>


[-- Attachment #1.2: Type: text/html, Size: 21886 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-11 16:35                       ` Nieto, David M
@ 2021-05-15  6:40                         ` Marek Olšák
  2021-05-15 13:22                           ` Christian König
  0 siblings, 1 reply; 18+ messages in thread
From: Marek Olšák @ 2021-05-15  6:40 UTC (permalink / raw)
  To: Nieto, David M
  Cc: Kees Cook, Christian König, Gu, JiaWei (Will),
	amd-gfx list, Deng, Emily, Deucher, Alexander, Alex Deucher


[-- Attachment #1.1: Type: text/plain, Size: 12102 bytes --]

1) Mesa doesn't plan to use this VBIOS query.

2) The patch is changing uapi, which is forbidden.

Marek

On Tue, May 11, 2021 at 12:35 PM Nieto, David M <David.Nieto@amd.com> wrote:

> [AMD Public Use]
>
> The point of having the device ID in the structure is because we are
> reading it from the VBIOS header, not the asic registers. They should
> match, but an user may flash a VBIOS for a different devid and they may not
> match.
>
> Regarding sysfs vs ioctl I see value in providing it in both ways, Mesa
> uses IOCTL and other DRM based tools may benefit as well. I recently went
> through the trouble of having to add sysfs string parsing for some data not
> available in ioctl, and while not very complicated, it is a programming
> inconvenience.
>
> I understand that being uapi, changing it is not easy, but this is
> information extracted from a VBIOS header, something that has been kept
> stable for many years.
>
> David
> ------------------------------
> *From:* Christian König <ckoenig.leichtzumerken@gmail.com>
> *Sent:* Tuesday, May 11, 2021 7:07 AM
> *To:* Deucher, Alexander <Alexander.Deucher@amd.com>; Marek Olšák <
> maraeo@gmail.com>
> *Cc:* Kees Cook <keescook@chromium.org>; Gu, JiaWei (Will) <
> JiaWei.Gu@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; Deng,
> Emily <Emily.Deng@amd.com>; Alex Deucher <alexdeucher@gmail.com>; Nieto,
> David M <David.Nieto@amd.com>
> *Subject:* Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
> Yeah, but umr is making strong use of sysfs as well.
>
> The only justification of this interface would be if we want to use it in
> Mesa.
>
> And I agree with Marek that looks redundant with the device structure to
> me as well.
>
> Christian.
>
> Am 11.05.21 um 16:04 schrieb Deucher, Alexander:
>
> [AMD Public Use]
>
> It's being used by umr and some other smi tools to provide vbios
> information for debugging.
>
> Alex
>
> ------------------------------
> *From:* amd-gfx <amd-gfx-bounces@lists.freedesktop.org>
> <amd-gfx-bounces@lists.freedesktop.org> on behalf of Marek Olšák
> <maraeo@gmail.com> <maraeo@gmail.com>
> *Sent:* Tuesday, May 11, 2021 4:18 AM
> *To:* Christian König <ckoenig.leichtzumerken@gmail.com>
> <ckoenig.leichtzumerken@gmail.com>
> *Cc:* Kees Cook <keescook@chromium.org> <keescook@chromium.org>; Gu,
> JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>; amd-gfx list
> <amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org>; Deng,
> Emily <Emily.Deng@amd.com> <Emily.Deng@amd.com>; Alex Deucher
> <alexdeucher@gmail.com> <alexdeucher@gmail.com>; Nieto, David M
> <David.Nieto@amd.com> <David.Nieto@amd.com>
> *Subject:* Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
> Mesa doesn't use sysfs.
>
> Note that this is a uapi, meaning that once it's in the kernel, it can't
> be changed like that.
>
> What's the use case for this new interface? Isn't it partially redundant
> with the current device info structure, which seems to have the equivalent
> of dev_id and rev_id?
>
> Marek
>
> On Tue, May 11, 2021 at 3:51 AM Christian König <
> ckoenig.leichtzumerken@gmail.com> wrote:
>
> Marek and other userspace folks need to decide that.
>
> Basic question here is if Mesa is already accessing sysfs nodes for OpenGL
> or RADV. If that is the case then we should probably expose the information
> there as well.
>
> If that isn't the case (which I think it is) then we should implement it
> as IOCTL.
>
> Regards,
> Christian.
>
> Am 10.05.21 um 22:19 schrieb Nieto, David M:
>
> One of the primary usecases is to add this information to the renderer
> string, I am not sure if there are other cases of UMD drivers accessing
> sysfs nodes, but I think if we think permissions, if a client is
> authenticated and opens the render device then it can use the IOCTL, it is
> unclear to me we can make a such an assumption for sysfs nodes…
>
>
>
> I think there is value in having both tbh.
>
>
>
> Regards,
>
> David
>
>
>
> *From: *Christian König <ckoenig.leichtzumerken@gmail.com>
> <ckoenig.leichtzumerken@gmail.com>
> *Date: *Monday, May 10, 2021 at 6:48 AM
> *To: *"Nieto, David M" <David.Nieto@amd.com> <David.Nieto@amd.com>, "Gu,
> JiaWei (Will)" <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> *Cc: *Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>,
> "Deng, Emily" <Emily.Deng@amd.com> <Emily.Deng@amd.com>, Kees Cook
> <keescook@chromium.org> <keescook@chromium.org>, amd-gfx list
> <amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org>
> *Subject: *Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>
>
> Well we could add both as sysfs file(s).
>
> Question here is rather what is the primary use case of this and if the
> application has the necessary access permissions to the sysfs files?
>
> Regards,
> Christian.
>
> Am 10.05.21 um 15:42 schrieb Nieto, David M:
>
> Then the application would need to issue the ioctl and then open a sysfs
> file to get all the information it needs. It makes little sense from a
> programming perspective to add an incomplete interface in my opinion
>
>
> ------------------------------
>
> *From:* Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> *Sent:* Monday, May 10, 2021 12:13:07 AM
> *To:* Nieto, David M <David.Nieto@amd.com> <David.Nieto@amd.com>
> *Cc:* Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>;
> amd-gfx list <amd-gfx@lists.freedesktop.org>
> <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>
> <keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com>
> <Emily.Deng@amd.com>
> *Subject:* RE: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
>
>
>
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi David,
>
> What I meant is to ONLY delete the serial[16] from drm_amdgpu_info_vbios,
> not the whole struct.
>
> struct drm_amdgpu_info_vbios {
>         __u8 name[64];
>         __u32 dbdf;
>         __u8 vbios_pn[64];
>         __u32 version;
>         __u8 date[32];
>         __u8 serial[16]; // jiawei: shall we delete this
>         __u32 dev_id;
>         __u32 rev_id;
>         __u32 sub_dev_id;
>         __u32 sub_ved_id;
> };
>
> serial[16] in drm_amdgpu_info_vbios  copied from adev->serial, but there's
> already a sysfs named serial_number, which exposes it already.
>
> static ssize_t amdgpu_device_get_serial_number(struct device *dev,
>                 struct device_attribute *attr, char *buf)
> {
>         struct drm_device *ddev = dev_get_drvdata(dev);
>         struct amdgpu_device *adev = ddev->dev_private;
>
>         return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial);
> }
>
> Thanks,
> Jiawei
>
>
> -----Original Message-----
> From: Nieto, David M <David.Nieto@amd.com> <David.Nieto@amd.com>
> Sent: Monday, May 10, 2021 2:53 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> Cc: Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>; amd-gfx
> list <amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org>;
> Kees Cook <keescook@chromium.org> <keescook@chromium.org>; Deng, Emily
> <Emily.Deng@amd.com> <Emily.Deng@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
>
> No, this structure contains all the details of the vbios: date, serial
> number, name, etc.
>
> The sysfs node only contains the vbios name string
>
> > On May 9, 2021, at 23:33, Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
> <JiaWei.Gu@amd.com> wrote:
> >
> > [AMD Official Use Only - Internal Distribution Only]
> >
> > With a second thought,
> > __u8 serial[16] in drm_amdgpu_info_vbios is a bit redundant, sysfs
> serial_number already exposes it.
> >
> > Is it fine to abandon it from drm_amdgpu_info_vbios struct? @Alex
> > Deucher @Nieto, David M
> >
> > Best regards,
> > Jiawei
> >
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>
> > Sent: Sunday, May 9, 2021 11:59 PM
> > To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> <amd-gfx@lists.freedesktop.org>; Kees Cook
> > <keescook@chromium.org> <keescook@chromium.org>
> > Subject: Re: [PATCH] drm/amdgpu: Align serial size in
> > drm_amdgpu_info_vbios
> >
> >> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu <Jiawei.Gu@amd.com>
> <Jiawei.Gu@amd.com> wrote:
> >>
> >> 20 should be serial char size now instead of 16.
> >>
> >> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com> <Jiawei.Gu@amd.com>
> >
> > Please make sure this keeps proper 64 bit alignment in the structure.
> >
> > Alex
> >
> >
> >> ---
> >> include/uapi/drm/amdgpu_drm.h | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/uapi/drm/amdgpu_drm.h
> >> b/include/uapi/drm/amdgpu_drm.h index 2b487a8d2727..1c20721f90da
> >> 100644
> >> --- a/include/uapi/drm/amdgpu_drm.h
> >> +++ b/include/uapi/drm/amdgpu_drm.h
> >> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
> >>        __u8 vbios_pn[64];
> >>        __u32 version;
> >>        __u8 date[32];
> >> -       __u8 serial[16];
> >> +       __u8 serial[20];
> >>        __u32 dev_id;
> >>        __u32 rev_id;
> >>        __u32 sub_dev_id;
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> >> t
> >> s.freedesktop.org
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fs.freedesktop.org%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684906500%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gKeRCQ485gWespGfo1O01tZz8fDy1zPGHIDEvCqyp1I%3D&reserved=0>
> %2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
> >> i
> >> awei.Gu%40amd.com
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2F40amd.com%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684916541%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8pM7WTh9%2F23KCF6DuXOc4%2Fi7rDUSxmH647BdpujXgWo%3D&reserved=0>
> %7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
> >> 6
> >> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
> >> d
> >> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> >> C
> >> 1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
> >> e
> >> rved=0
>
>
>
> _______________________________________________
>
> amd-gfx mailing list
>
> amd-gfx@lists.freedesktop.org
>
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684926488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Hdhx2ogsQt2C3OIKNC%2BaswtNkiIWbV6Ot8a%2B3YFqa7w%3D&reserved=0>
>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684926488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Hdhx2ogsQt2C3OIKNC%2BaswtNkiIWbV6Ot8a%2B3YFqa7w%3D&reserved=0>
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 19348 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
  2021-05-15  6:40                         ` Marek Olšák
@ 2021-05-15 13:22                           ` Christian König
  0 siblings, 0 replies; 18+ messages in thread
From: Christian König @ 2021-05-15 13:22 UTC (permalink / raw)
  To: Marek Olšák, Nieto, David M
  Cc: Kees Cook, Gu, JiaWei (Will),
	amd-gfx list, Deng, Emily, Deucher, Alexander, Alex Deucher


[-- Attachment #1.1: Type: text/plain, Size: 16286 bytes --]

The uAPI isn't merged upstream yet, so can still be changed.

But I agree that this should be squashed into the original patch while 
upstreaming.

When this is just for umr as open source client then the question is 
still on the stable if this really need to be an IOCTL?

Christian.

Am 15.05.21 um 08:40 schrieb Marek Olšák:
> 1) Mesa doesn't plan to use this VBIOS query.
>
> 2) The patch is changing uapi, which is forbidden.
>
> Marek
>
> On Tue, May 11, 2021 at 12:35 PM Nieto, David M <David.Nieto@amd.com 
> <mailto:David.Nieto@amd.com>> wrote:
>
>     [AMD Public Use]
>
>
>     The point of having the device ID in the structure is because we
>     are reading it from the VBIOS header, not the asic registers. They
>     should match, but an user may flash a VBIOS for a different devid
>     and they may not match.
>
>     Regarding sysfs vs ioctl I see value in providing it in both ways,
>     Mesa uses IOCTL and other DRM based tools may benefit as well. I
>     recently went through the trouble of having to add sysfs string
>     parsing for some data not available in ioctl, and while not very
>     complicated, it is a programming inconvenience.
>
>     I understand that being uapi, changing it is not easy, but this is
>     information extracted from a VBIOS header, something that has been
>     kept stable for many years.
>
>     David
>     ------------------------------------------------------------------------
>     *From:* Christian König <ckoenig.leichtzumerken@gmail.com
>     <mailto:ckoenig.leichtzumerken@gmail.com>>
>     *Sent:* Tuesday, May 11, 2021 7:07 AM
>     *To:* Deucher, Alexander <Alexander.Deucher@amd.com
>     <mailto:Alexander.Deucher@amd.com>>; Marek Olšák <maraeo@gmail.com
>     <mailto:maraeo@gmail.com>>
>     *Cc:* Kees Cook <keescook@chromium.org
>     <mailto:keescook@chromium.org>>; Gu, JiaWei (Will)
>     <JiaWei.Gu@amd.com <mailto:JiaWei.Gu@amd.com>>; amd-gfx list
>     <amd-gfx@lists.freedesktop.org
>     <mailto:amd-gfx@lists.freedesktop.org>>; Deng, Emily
>     <Emily.Deng@amd.com <mailto:Emily.Deng@amd.com>>; Alex Deucher
>     <alexdeucher@gmail.com <mailto:alexdeucher@gmail.com>>; Nieto,
>     David M <David.Nieto@amd.com <mailto:David.Nieto@amd.com>>
>     *Subject:* Re: [PATCH] drm/amdgpu: Align serial size in
>     drm_amdgpu_info_vbios
>     Yeah, but umr is making strong use of sysfs as well.
>
>     The only justification of this interface would be if we want to
>     use it in Mesa.
>
>     And I agree with Marek that looks redundant with the device
>     structure to me as well.
>
>     Christian.
>
>     Am 11.05.21 um 16:04 schrieb Deucher, Alexander:
>>
>>     [AMD Public Use]
>>
>>
>>     It's being used by umr and some other smi tools to provide vbios
>>     information for debugging.
>>
>>     Alex
>>
>>     ------------------------------------------------------------------------
>>     *From:* amd-gfx <amd-gfx-bounces@lists.freedesktop.org>
>>     <mailto:amd-gfx-bounces@lists.freedesktop.org> on behalf of Marek
>>     Olšák <maraeo@gmail.com> <mailto:maraeo@gmail.com>
>>     *Sent:* Tuesday, May 11, 2021 4:18 AM
>>     *To:* Christian König <ckoenig.leichtzumerken@gmail.com>
>>     <mailto:ckoenig.leichtzumerken@gmail.com>
>>     *Cc:* Kees Cook <keescook@chromium.org>
>>     <mailto:keescook@chromium.org>; Gu, JiaWei (Will)
>>     <JiaWei.Gu@amd.com> <mailto:JiaWei.Gu@amd.com>; amd-gfx list
>>     <amd-gfx@lists.freedesktop.org>
>>     <mailto:amd-gfx@lists.freedesktop.org>; Deng, Emily
>>     <Emily.Deng@amd.com> <mailto:Emily.Deng@amd.com>; Alex Deucher
>>     <alexdeucher@gmail.com> <mailto:alexdeucher@gmail.com>; Nieto,
>>     David M <David.Nieto@amd.com> <mailto:David.Nieto@amd.com>
>>     *Subject:* Re: [PATCH] drm/amdgpu: Align serial size in
>>     drm_amdgpu_info_vbios
>>     Mesa doesn't use sysfs.
>>
>>     Note that this is a uapi, meaning that once it's in the kernel,
>>     it can't be changed like that.
>>
>>     What's the use case for this new interface? Isn't it partially
>>     redundant with the current device info structure, which seems to
>>     have the equivalent of dev_id and rev_id?
>>
>>     Marek
>>
>>     On Tue, May 11, 2021 at 3:51 AM Christian König
>>     <ckoenig.leichtzumerken@gmail.com
>>     <mailto:ckoenig.leichtzumerken@gmail.com>> wrote:
>>
>>         Marek and other userspace folks need to decide that.
>>
>>         Basic question here is if Mesa is already accessing sysfs
>>         nodes for OpenGL or RADV. If that is the case then we should
>>         probably expose the information there as well.
>>
>>         If that isn't the case (which I think it is) then we should
>>         implement it as IOCTL.
>>
>>         Regards,
>>         Christian.
>>
>>         Am 10.05.21 um 22:19 schrieb Nieto, David M:
>>>
>>>         One of the primary usecases is to add this information to
>>>         the renderer string, I am not sure if there are other cases
>>>         of UMD drivers accessing sysfs nodes, but I think if we
>>>         think permissions, if a client is authenticated and opens
>>>         the render device then it can use the IOCTL, it is unclear
>>>         to me we can make a such an assumption for sysfs nodes…
>>>
>>>         I think there is value in having both tbh.
>>>
>>>         Regards,
>>>
>>>         David
>>>
>>>         *From: *Christian König <ckoenig.leichtzumerken@gmail.com>
>>>         <mailto:ckoenig.leichtzumerken@gmail.com>
>>>         *Date: *Monday, May 10, 2021 at 6:48 AM
>>>         *To: *"Nieto, David M" <David.Nieto@amd.com>
>>>         <mailto:David.Nieto@amd.com>, "Gu, JiaWei (Will)"
>>>         <JiaWei.Gu@amd.com> <mailto:JiaWei.Gu@amd.com>
>>>         *Cc: *Alex Deucher <alexdeucher@gmail.com>
>>>         <mailto:alexdeucher@gmail.com>, "Deng, Emily"
>>>         <Emily.Deng@amd.com> <mailto:Emily.Deng@amd.com>, Kees Cook
>>>         <keescook@chromium.org> <mailto:keescook@chromium.org>,
>>>         amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>         <mailto:amd-gfx@lists.freedesktop.org>
>>>         *Subject: *Re: [PATCH] drm/amdgpu: Align serial size in
>>>         drm_amdgpu_info_vbios
>>>
>>>         Well we could add both as sysfs file(s).
>>>
>>>         Question here is rather what is the primary use case of this
>>>         and if the application has the necessary access permissions
>>>         to the sysfs files?
>>>
>>>         Regards,
>>>         Christian.
>>>
>>>         Am 10.05.21 um 15:42 schrieb Nieto, David M:
>>>
>>>             Then the application would need to issue the ioctl and
>>>             then open a sysfs file to get all the information it
>>>             needs. It makes little sense from a programming
>>>             perspective to add an incomplete interface in my opinion
>>>
>>>             ------------------------------------------------------------------------
>>>
>>>             *From:*Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
>>>             <mailto:JiaWei.Gu@amd.com>
>>>             *Sent:* Monday, May 10, 2021 12:13:07 AM
>>>             *To:* Nieto, David M <David.Nieto@amd.com>
>>>             <mailto:David.Nieto@amd.com>
>>>             *Cc:* Alex Deucher <alexdeucher@gmail.com>
>>>             <mailto:alexdeucher@gmail.com>; amd-gfx list
>>>             <amd-gfx@lists.freedesktop.org>
>>>             <mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
>>>             <keescook@chromium.org> <mailto:keescook@chromium.org>;
>>>             Deng, Emily <Emily.Deng@amd.com> <mailto:Emily.Deng@amd.com>
>>>             *Subject:* RE: [PATCH] drm/amdgpu: Align serial size in
>>>             drm_amdgpu_info_vbios
>>>
>>>             [AMD Official Use Only - Internal Distribution Only]
>>>
>>>             Hi David,
>>>
>>>             What I meant is to ONLY delete the serial[16] from
>>>             drm_amdgpu_info_vbios, not the whole struct.
>>>
>>>             struct drm_amdgpu_info_vbios {
>>>                     __u8 name[64];
>>>                     __u32 dbdf;
>>>                     __u8 vbios_pn[64];
>>>                     __u32 version;
>>>                     __u8 date[32];
>>>                     __u8 serial[16]; // jiawei: shall we delete this
>>>                     __u32 dev_id;
>>>                     __u32 rev_id;
>>>                     __u32 sub_dev_id;
>>>                     __u32 sub_ved_id;
>>>             };
>>>
>>>             serial[16] in drm_amdgpu_info_vbios  copied from
>>>             adev->serial, but there's already a sysfs named
>>>             serial_number, which exposes it already.
>>>
>>>             static ssize_t amdgpu_device_get_serial_number(struct
>>>             device *dev,
>>>                             struct device_attribute *attr, char *buf)
>>>             {
>>>                     struct drm_device *ddev = dev_get_drvdata(dev);
>>>                     struct amdgpu_device *adev = ddev->dev_private;
>>>
>>>                     return snprintf(buf, PAGE_SIZE, "%s\n",
>>>             adev->serial);
>>>             }
>>>
>>>             Thanks,
>>>             Jiawei
>>>
>>>
>>>             -----Original Message-----
>>>             From: Nieto, David M <David.Nieto@amd.com>
>>>             <mailto:David.Nieto@amd.com>
>>>             Sent: Monday, May 10, 2021 2:53 PM
>>>             To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
>>>             <mailto:JiaWei.Gu@amd.com>
>>>             Cc: Alex Deucher <alexdeucher@gmail.com>
>>>             <mailto:alexdeucher@gmail.com>; amd-gfx list
>>>             <amd-gfx@lists.freedesktop.org>
>>>             <mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
>>>             <keescook@chromium.org> <mailto:keescook@chromium.org>;
>>>             Deng, Emily <Emily.Deng@amd.com> <mailto:Emily.Deng@amd.com>
>>>             Subject: Re: [PATCH] drm/amdgpu: Align serial size in
>>>             drm_amdgpu_info_vbios
>>>
>>>             No, this structure contains all the details of the
>>>             vbios: date, serial number, name, etc.
>>>
>>>             The sysfs node only contains the vbios name string
>>>
>>>             > On May 9, 2021, at 23:33, Gu, JiaWei (Will)
>>>             <JiaWei.Gu@amd.com> <mailto:JiaWei.Gu@amd.com> wrote:
>>>             >
>>>             > [AMD Official Use Only - Internal Distribution Only]
>>>             >
>>>             > With a second thought,
>>>             > __u8 serial[16] in drm_amdgpu_info_vbios is a bit
>>>             redundant, sysfs serial_number already exposes it.
>>>             >
>>>             > Is it fine to abandon it from drm_amdgpu_info_vbios
>>>             struct? @Alex
>>>             > Deucher @Nieto, David M
>>>             >
>>>             > Best regards,
>>>             > Jiawei
>>>             >
>>>             > -----Original Message-----
>>>             > From: Alex Deucher <alexdeucher@gmail.com>
>>>             <mailto:alexdeucher@gmail.com>
>>>             > Sent: Sunday, May 9, 2021 11:59 PM
>>>             > To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>
>>>             <mailto:JiaWei.Gu@amd.com>
>>>             > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>             <mailto:amd-gfx@lists.freedesktop.org>; Kees Cook
>>>             > <keescook@chromium.org> <mailto:keescook@chromium.org>
>>>             > Subject: Re: [PATCH] drm/amdgpu: Align serial size in
>>>             > drm_amdgpu_info_vbios
>>>             >
>>>             >> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu
>>>             <Jiawei.Gu@amd.com> <mailto:Jiawei.Gu@amd.com> wrote:
>>>             >>
>>>             >> 20 should be serial char size now instead of 16.
>>>             >>
>>>             >> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
>>>             <mailto:Jiawei.Gu@amd.com>
>>>             >
>>>             > Please make sure this keeps proper 64 bit alignment in
>>>             the structure.
>>>             >
>>>             > Alex
>>>             >
>>>             >
>>>             >> ---
>>>             >> include/uapi/drm/amdgpu_drm.h | 2 +-
>>>             >> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>             >>
>>>             >> diff --git a/include/uapi/drm/amdgpu_drm.h
>>>             >> b/include/uapi/drm/amdgpu_drm.h index
>>>             2b487a8d2727..1c20721f90da
>>>             >> 100644
>>>             >> --- a/include/uapi/drm/amdgpu_drm.h
>>>             >> +++ b/include/uapi/drm/amdgpu_drm.h
>>>             >> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>>             >>        __u8 vbios_pn[64];
>>>             >>        __u32 version;
>>>             >>        __u8 date[32];
>>>             >> -       __u8 serial[16];
>>>             >> +       __u8 serial[20];
>>>             >>        __u32 dev_id;
>>>             >>        __u32 rev_id;
>>>             >>        __u32 sub_dev_id;
>>>             >> --
>>>             >> 2.17.1
>>>             >>
>>>             >> _______________________________________________
>>>             >> amd-gfx mailing list
>>>             >> amd-gfx@lists.freedesktop.org
>>>             <mailto:amd-gfx@lists.freedesktop.org>
>>>             >>
>>>             https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>>>             <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis>
>>>             >> t
>>>             >> s.freedesktop.org
>>>             <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fs.freedesktop.org%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684906500%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gKeRCQ485gWespGfo1O01tZz8fDy1zPGHIDEvCqyp1I%3D&reserved=0>%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJ
>>>             >> i
>>>             >> awei.Gu%40amd.com
>>>             <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2F40amd.com%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684916541%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8pM7WTh9%2F23KCF6DuXOc4%2Fi7rDUSxmH647BdpujXgWo%3D&reserved=0>%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>>>             >> 6
>>>             >>
>>>             08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>>>             >> d
>>>             >>
>>>             8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>>>             >> C
>>>             >>
>>>             1000&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
>>>             >> e
>>>             >> rved=0
>>>
>>>
>>>
>>>             _______________________________________________
>>>
>>>             amd-gfx mailing list
>>>
>>>             amd-gfx@lists.freedesktop.org  <mailto:amd-gfx@lists.freedesktop.org>
>>>
>>>             https://lists.freedesktop.org/mailman/listinfo/amd-gfx  <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684926488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Hdhx2ogsQt2C3OIKNC%2BaswtNkiIWbV6Ot8a%2B3YFqa7w%3D&reserved=0>
>>>
>>>
>>>
>>
>>         _______________________________________________
>>         amd-gfx mailing list
>>         amd-gfx@lists.freedesktop.org
>>         <mailto:amd-gfx@lists.freedesktop.org>
>>         https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>         <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684926488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Hdhx2ogsQt2C3OIKNC%2BaswtNkiIWbV6Ot8a%2B3YFqa7w%3D&reserved=0>
>>
>


[-- Attachment #1.2: Type: text/html, Size: 40993 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-05-15 13:22 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-08  6:47 [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios Jiawei Gu
2021-05-09 15:58 ` Alex Deucher
2021-05-10  6:33   ` Gu, JiaWei (Will)
2021-05-10  6:53     ` Nieto, David M
2021-05-10  7:13       ` Gu, JiaWei (Will)
2021-05-10 13:42         ` Nieto, David M
2021-05-10 13:48           ` Christian König
2021-05-10 20:19             ` Nieto, David M
2021-05-11  2:23               ` Gu, JiaWei (Will)
2021-05-11  2:52                 ` Nieto, David M
2021-05-11  3:01                   ` Gu, JiaWei (Will)
2021-05-11  7:51               ` Christian König
2021-05-11  8:18                 ` Marek Olšák
2021-05-11 14:04                   ` Deucher, Alexander
2021-05-11 14:07                     ` Christian König
2021-05-11 16:35                       ` Nieto, David M
2021-05-15  6:40                         ` Marek Olšák
2021-05-15 13:22                           ` Christian König

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.