Re: [PATCH] drm/amdgpu: Handle null atom context in VBIOS info ioctl
On Tue, Sep 5, 2023 at 12:15 PM Francis, David wrote: > > [AMD Official Use Only - General] > > > [AMD Official Use Only - General] > > Yeah we've had JIRAs (e.g. > https://ontrack-internal.amd.com/browse/SWDEV-409711 ) raised that ASAN can't > compile the thunk due to using = {0} . Using memset is definitely preferred > to save trouble later. > > Kent > > This is kernel code, not thunk. {} and {0} are extensively used throughout > the kernel in general and our driver in particular, so I don't see this > causing problems. Speaking for the kernel, we've seen problematic behavior when using {} vs {0} vs memset. memset always seems to do the right thing, the others don't. E.g., there was a series of patch sets to switch everything from {} to {0} or vice versa, we just traded one issue for another. For this patch, you can probably just drop that hunk as I don't see a need to change it. Whether you switch to memset or not is up to you. Alex > > David > > -Original Message- > From: amd-gfx On Behalf Of Alex > Deucher > Sent: Tuesday, September 5, 2023 10:53 AM > To: Francis, David > Cc: amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Handle null atom context in VBIOS info ioctl > > On Tue, Sep 5, 2023 at 10:50 AM David Francis wrote: > > On some APU systems, there is no atom context and so the > atom_context struct is null. > > Add a check to the VBIOS_INFO branch of amdgpu_info_ioctl > to handle this case, returning all zeroes. > > Signed-off-by: David Francis > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 19 --- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c > > index 3a48bec10aea..86748290ead7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c > @@ -947,16 +947,21 @@ int amdgpu_info_ioctl(struct drm_device *dev, void > > *data, struct drm_file *filp) > > ? -EFAULT : 0; > } > case AMDGPU_INFO_VBIOS_INFO: { > - struct drm_amdgpu_info_vbios vbios_info = {}; > + struct drm_amdgpu_info_vbios vbios_info = {0}; > > IIRC, these should be equivalent. That said, I believe memset is > generally preferred as not all compilers seem to handle these cases > correctly. > > Alex > > struct atom_context *atom_context; > > atom_context = adev->mode_info.atom_context; > - memcpy(vbios_info.name, atom_context->name, > > sizeof(atom_context->name)); > > - memcpy(vbios_info.vbios_pn, atom_context->vbios_pn, > > sizeof(atom_context->vbios_pn)); > > - vbios_info.version = atom_context->version; > - memcpy(vbios_info.vbios_ver_str, > atom_context->vbios_ver_str, > - > sizeof(atom_context->vbios_ver_str)); > - memcpy(vbios_info.date, atom_context->date, > > sizeof(atom_context->date)); > > + if (atom_context) { > + memcpy(vbios_info.name, atom_context->name, > + sizeof(atom_context->name)); > + memcpy(vbios_info.vbios_pn, > atom_context->vbios_pn, > + sizeof(atom_context->vbios_pn)); > + vbios_info.version = atom_context->version; > + memcpy(vbios_info.vbios_ver_str, atom_context- > vbios_ver_str, > + sizeof(atom_context->vbios_ver_str)); > + memcpy(vbios_info.date, atom_context->date, > + sizeof(atom_context->date)); > + } > > return copy_to_user(out, &vbios_info, > min((size_t)size, > sizeof(vbios_info))) ? -EFAULT : 0; > -- > 2.34.1 >
RE: [PATCH] drm/amdgpu: Handle null atom context in VBIOS info ioctl
[AMD Official Use Only - General] Sorry, meant to keep the JIRA part internal. As it stands, the amd/ folder alone has 458 {0}s (plus 55 {}s), and 741 memset-to-0s. So I guess it’s a matter of whether or not we’ll start enforcing memsets vs {0} in the near future. If we intend to switch over soon anyways, we may as well start using memset now and in all future patches. Kent From: Francis, David Sent: Tuesday, September 5, 2023 11:38 AM To: Russell, Kent ; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Handle null atom context in VBIOS info ioctl [AMD Official Use Only - General] [AMD Official Use Only - General] Yeah we've had JIRAs (e.g. https://ontrack-internal.amd.com/browse/SWDEV-409711 ) raised that ASAN can't compile the thunk due to using = {0} . Using memset is definitely preferred to save trouble later. Kent This is kernel code, not thunk. {} and {0} are extensively used throughout the kernel in general and our driver in particular, so I don't see this causing problems. David -Original Message- From: amd-gfx <mailto:amd-gfx-boun...@lists.freedesktop.org> On Behalf Of Alex Deucher Sent: Tuesday, September 5, 2023 10:53 AM To: Francis, David <mailto:david.fran...@amd.com> Cc: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> Subject: Re: [PATCH] drm/amdgpu: Handle null atom context in VBIOS info ioctl On Tue, Sep 5, 2023 at 10:50 AM David Francis <mailto:david.fran...@amd.com> wrote: On some APU systems, there is no atom context and so the atom_context struct is null. Add a check to the VBIOS_INFO branch of amdgpu_info_ioctl to handle this case, returning all zeroes. Signed-off-by: David Francis <mailto:david.fran...@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index 3a48bec10aea..86748290ead7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -947,16 +947,21 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) ? -EFAULT : 0; } case AMDGPU_INFO_VBIOS_INFO: { - struct drm_amdgpu_info_vbios vbios_info = {}; + struct drm_amdgpu_info_vbios vbios_info = {0}; IIRC, these should be equivalent. That said, I believe memset is generally preferred as not all compilers seem to handle these cases correctly. Alex struct atom_context *atom_context; atom_context = adev->mode_info.atom_context; - memcpy(vbios_info.name, atom_context->name, sizeof(atom_context->name)); - memcpy(vbios_info.vbios_pn, atom_context->vbios_pn, sizeof(atom_context->vbios_pn)); - vbios_info.version = atom_context->version; - memcpy(vbios_info.vbios_ver_str, atom_context->vbios_ver_str, - sizeof(atom_context->vbios_ver_str)); - memcpy(vbios_info.date, atom_context->date, sizeof(atom_context->date)); + if (atom_context) { + memcpy(vbios_info.name, atom_context->name, + sizeof(atom_context->name)); + memcpy(vbios_info.vbios_pn, atom_context->vbios_pn, + sizeof(atom_context->vbios_pn)); + vbios_info.version = atom_context->version; + memcpy(vbios_info.vbios_ver_str, atom_context- vbios_ver_str, + sizeof(atom_context->vbios_ver_str)); + memcpy(vbios_info.date, atom_context->date, + sizeof(atom_context->date)); + } return copy_to_user(out, &vbios_info, min((size_t)size, sizeof(vbios_info))) ? -EFAULT : 0; -- 2.34.1
Re: [PATCH] drm/amdgpu: Handle null atom context in VBIOS info ioctl
[AMD Official Use Only - General] [AMD Official Use Only - General] Yeah we've had JIRAs (e.g. https://ontrack-internal.amd.com/browse/SWDEV-409711 ) raised that ASAN can't compile the thunk due to using = {0} . Using memset is definitely preferred to save trouble later. Kent This is kernel code, not thunk. {} and {0} are extensively used throughout the kernel in general and our driver in particular, so I don't see this causing problems. David -Original Message- From: amd-gfx <mailto:amd-gfx-boun...@lists.freedesktop.org> On Behalf Of Alex Deucher Sent: Tuesday, September 5, 2023 10:53 AM To: Francis, David <mailto:david.fran...@amd.com> Cc: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> Subject: Re: [PATCH] drm/amdgpu: Handle null atom context in VBIOS info ioctl On Tue, Sep 5, 2023 at 10:50 AM David Francis <mailto:david.fran...@amd.com> wrote: On some APU systems, there is no atom context and so the atom_context struct is null. Add a check to the VBIOS_INFO branch of amdgpu_info_ioctl to handle this case, returning all zeroes. Signed-off-by: David Francis <mailto:david.fran...@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index 3a48bec10aea..86748290ead7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -947,16 +947,21 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) ? -EFAULT : 0; } case AMDGPU_INFO_VBIOS_INFO: { - struct drm_amdgpu_info_vbios vbios_info = {}; + struct drm_amdgpu_info_vbios vbios_info = {0}; IIRC, these should be equivalent. That said, I believe memset is generally preferred as not all compilers seem to handle these cases correctly. Alex struct atom_context *atom_context; atom_context = adev->mode_info.atom_context; - memcpy(vbios_info.name, atom_context->name, sizeof(atom_context->name)); - memcpy(vbios_info.vbios_pn, atom_context->vbios_pn, sizeof(atom_context->vbios_pn)); - vbios_info.version = atom_context->version; - memcpy(vbios_info.vbios_ver_str, atom_context->vbios_ver_str, - sizeof(atom_context->vbios_ver_str)); - memcpy(vbios_info.date, atom_context->date, sizeof(atom_context->date)); + if (atom_context) { + memcpy(vbios_info.name, atom_context->name, + sizeof(atom_context->name)); + memcpy(vbios_info.vbios_pn, atom_context->vbios_pn, + sizeof(atom_context->vbios_pn)); + vbios_info.version = atom_context->version; + memcpy(vbios_info.vbios_ver_str, atom_context- vbios_ver_str, + sizeof(atom_context->vbios_ver_str)); + memcpy(vbios_info.date, atom_context->date, + sizeof(atom_context->date)); + } return copy_to_user(out, &vbios_info, min((size_t)size, sizeof(vbios_info))) ? -EFAULT : 0; -- 2.34.1
Re: [PATCH] drm/amdgpu: Handle null atom context in VBIOS info ioctl
On Tue, Sep 5, 2023 at 10:50 AM David Francis wrote: > > On some APU systems, there is no atom context and so the > atom_context struct is null. > > Add a check to the VBIOS_INFO branch of amdgpu_info_ioctl > to handle this case, returning all zeroes. > > Signed-off-by: David Francis > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 19 --- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c > index 3a48bec10aea..86748290ead7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c > @@ -947,16 +947,21 @@ int amdgpu_info_ioctl(struct drm_device *dev, void > *data, struct drm_file *filp) > ? -EFAULT : 0; > } > case AMDGPU_INFO_VBIOS_INFO: { > - struct drm_amdgpu_info_vbios vbios_info = {}; > + struct drm_amdgpu_info_vbios vbios_info = {0}; IIRC, these should be equivalent. That said, I believe memset is generally preferred as not all compilers seem to handle these cases correctly. Alex > struct atom_context *atom_context; > > atom_context = adev->mode_info.atom_context; > - memcpy(vbios_info.name, atom_context->name, > sizeof(atom_context->name)); > - memcpy(vbios_info.vbios_pn, atom_context->vbios_pn, > sizeof(atom_context->vbios_pn)); > - vbios_info.version = atom_context->version; > - memcpy(vbios_info.vbios_ver_str, > atom_context->vbios_ver_str, > - > sizeof(atom_context->vbios_ver_str)); > - memcpy(vbios_info.date, atom_context->date, > sizeof(atom_context->date)); > + if (atom_context) { > + memcpy(vbios_info.name, atom_context->name, > + sizeof(atom_context->name)); > + memcpy(vbios_info.vbios_pn, > atom_context->vbios_pn, > + sizeof(atom_context->vbios_pn)); > + vbios_info.version = atom_context->version; > + memcpy(vbios_info.vbios_ver_str, > atom_context->vbios_ver_str, > + sizeof(atom_context->vbios_ver_str)); > + memcpy(vbios_info.date, atom_context->date, > + sizeof(atom_context->date)); > + } > > return copy_to_user(out, &vbios_info, > min((size_t)size, > sizeof(vbios_info))) ? -EFAULT : 0; > -- > 2.34.1 >
[PATCH] drm/amdgpu: Handle null atom context in VBIOS info ioctl
On some APU systems, there is no atom context and so the atom_context struct is null. Add a check to the VBIOS_INFO branch of amdgpu_info_ioctl to handle this case, returning all zeroes. Signed-off-by: David Francis --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index 3a48bec10aea..86748290ead7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -947,16 +947,21 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) ? -EFAULT : 0; } case AMDGPU_INFO_VBIOS_INFO: { - struct drm_amdgpu_info_vbios vbios_info = {}; + struct drm_amdgpu_info_vbios vbios_info = {0}; struct atom_context *atom_context; atom_context = adev->mode_info.atom_context; - memcpy(vbios_info.name, atom_context->name, sizeof(atom_context->name)); - memcpy(vbios_info.vbios_pn, atom_context->vbios_pn, sizeof(atom_context->vbios_pn)); - vbios_info.version = atom_context->version; - memcpy(vbios_info.vbios_ver_str, atom_context->vbios_ver_str, - sizeof(atom_context->vbios_ver_str)); - memcpy(vbios_info.date, atom_context->date, sizeof(atom_context->date)); + if (atom_context) { + memcpy(vbios_info.name, atom_context->name, + sizeof(atom_context->name)); + memcpy(vbios_info.vbios_pn, atom_context->vbios_pn, + sizeof(atom_context->vbios_pn)); + vbios_info.version = atom_context->version; + memcpy(vbios_info.vbios_ver_str, atom_context->vbios_ver_str, + sizeof(atom_context->vbios_ver_str)); + memcpy(vbios_info.date, atom_context->date, + sizeof(atom_context->date)); + } return copy_to_user(out, &vbios_info, min((size_t)size, sizeof(vbios_info))) ? -EFAULT : 0; -- 2.34.1