On 2025-08-05 15:56, Deucher, Alexander wrote: > [Public] > >> -----Original Message----- >> From: Lazar, Lijo <lijo.la...@amd.com> >> Sent: Wednesday, July 9, 2025 5:02 AM >> To: Koenig, Christian <christian.koe...@amd.com>; Deucher, Alexander >> <alexander.deuc...@amd.com>; McRae, Geoffrey >> <geoffrey.mc...@amd.com>; Kuehling, Felix <felix.kuehl...@amd.com> >> Cc: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Subject: Re: [PATCH v2 1/1] drm/amdkfd: return -ENOTTY for unsupported >> IOCTLs >> >> >> >> On 7/9/2025 2:09 PM, Christian König wrote: >>> On 09.07.25 06:56, Lazar, Lijo wrote: >>>> On 7/8/2025 8:40 PM, Deucher, Alexander wrote: >>>>> [Public] >>>>> >>>>> >>>>> I seem to recall -ENOTSUPP being frowned upon for IOCTLs. >>>>> >>>>> >>>> Going by documentation - >>>> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html >>>> >>> Good point. >>> >>>> EOPNOTSUPP: >>>> Feature (like PRIME, modesetting, GEM) is not supported by the driver. >>>> >>>> "Note that ENOTTY has the slightly unintuitive meaning of “this IOCTL >>>> does not exist”, and is used exactly as such in DRM" >>>> >>>> Since KFD ioctls could eventually be supported in drm node, >>> That's certainly not going to happen. >>> >>> We are currently in the process of deprecating the KFD IOCTLs and either >>> using >> the existing DRM render node ones or coming up with new IOCTL/additions to >> the >> existing ones. >> I really meant to convey this to justify using drm documentation as the >> background >> for picking error codes for KFD ones also. At least for any new error code >> returns, >> definitions will remain consistent across both. > In this case, I think -ENOTTY makes sense per the documentation. Patch is: > Acked-by: Alex Deucher <alexander.deuc...@amd.com>
I agree. Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com> > > >> Thanks, >> Lijo >> >>> Background is that some of the KFD IOCTLs have design flaws which are unfix >> able. >>> Regards, >>> Christian. >>> >>>> it seems >>>> better to go with ENOTTY. >>>> >>>> Thanks, >>>> Lijo >>>> >>>>> Alex >>>>> >>>>> >>>>> >>>>> *From:*McRae, Geoffrey <geoffrey.mc...@amd.com> >>>>> *Sent:* Tuesday, July 8, 2025 5:13 AM >>>>> *To:* Koenig, Christian <christian.koe...@amd.com>; Kuehling, Felix >>>>> <felix.kuehl...@amd.com> >>>>> *Cc:* Deucher, Alexander <alexander.deuc...@amd.com>; amd- >>>>> g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>>> *Subject:* Re: [PATCH v2 1/1] drm/amdkfd: return -ENOTTY for >>>>> unsupported IOCTLs >>>>> >>>>> >>>>> >>>>> [AMD Official Use Only - AMD Internal Distribution Only] >>>>> >>>>> >>>>> >>>>> I am happy to use EOPNOTSUPP but I must point out that this is not >>>>> the pattern used across the kernel, the standard is to use ENOTTY, >>>>> which is also the default that fs/ioctl.c returns when no handler is >>>>> present. >>>>> Userspace tooling such as strace and glibc specifically expectect >>>>> ENOTTY to indicate invalid or unsupported IOCTL. >>>>> >>>>> -------------------------------------------------------------------- >>>>> ---- >>>>> >>>>> *From:*Koenig, Christian <christian.koe...@amd.com >>>>> <mailto:christian.koe...@amd.com>> >>>>> *Sent:* Tuesday, 8 July 2025 5:01 PM >>>>> *To:* McRae, Geoffrey <geoffrey.mc...@amd.com >>>>> <mailto:geoffrey.mc...@amd.com>>; Kuehling, Felix >>>>> <felix.kuehl...@amd.com <mailto:felix.kuehl...@amd.com>> >>>>> *Cc:* Deucher, Alexander <alexander.deuc...@amd.com >>>>> <mailto:alexander.deuc...@amd.com>>; amd-...@lists.freedesktop.org >>>>> <mailto:amd-...@lists.freedesktop.org> >>>>> <amd-...@lists.freedesktop.org >>>>> <mailto:amd-...@lists.freedesktop.org>>; >>>>> dri-devel@lists.freedesktop.org >>>>> <mailto:dri-devel@lists.freedesktop.org> <dri- >>>>> de...@lists.freedesktop.org >>>>> <mailto:dri-devel@lists.freedesktop.org>> >>>>> *Subject:* Re: [PATCH v2 1/1] drm/amdkfd: return -ENOTTY for >>>>> unsupported IOCTLs >>>>> >>>>> >>>>> >>>>> On 08.07.25 06:22, Geoffrey McRae wrote: >>>>>> Some kfd ioctls may not be available depending on the kernel >>>>>> version the user is running, as such we need to report -ENOTTY so >>>>>> userland can determine the cause of the ioctl failure. >>>>> In general sounds like a good idea, but ENOTTY is potentially a bit >>>>> misleading. >>>>> >>>>> We usually use EOPNOTSUPP for that even if its not the original >>>>> meaning of that error code. >>>>> >>>>> Regards, >>>>> Christian. >>>>> >>>>>> Signed-off-by: Geoffrey McRae <geoffrey.mc...@amd.com >>>>>> <mailto:geoffrey.mc...@amd.com>> >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 8 ++++++-- >>>>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>>>>> index a2149afa5803..36396b7318e7 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>>>>> @@ -3253,8 +3253,10 @@ static long kfd_ioctl(struct file *filep, >>>>>> unsigned int cmd, unsigned long arg) >>>>>> int retcode = -EINVAL; >>>>>> bool ptrace_attached = false; >>>>>> >>>>>> - if (nr >= AMDKFD_CORE_IOCTL_COUNT) >>>>>> + if (nr >= AMDKFD_CORE_IOCTL_COUNT) { >>>>>> + retcode = -ENOTTY; >>>>>> goto err_i1; >>>>>> + } >>>>>> >>>>>> if ((nr >= AMDKFD_COMMAND_START) && (nr < >>>>>> AMDKFD_COMMAND_END)) { >>>>>> u32 amdkfd_size; >>>>>> @@ -3267,8 +3269,10 @@ static long kfd_ioctl(struct file *filep, >>>>>> unsigned int cmd, unsigned long arg) >>>>>> asize = amdkfd_size; >>>>>> >>>>>> cmd = ioctl->cmd; >>>>>> - } else >>>>>> + } else { >>>>>> + retcode = -ENOTTY; >>>>>> goto err_i1; >>>>>> + } >>>>>> >>>>>> dev_dbg(kfd_device, "ioctl cmd 0x%x (#0x%x), arg 0x%lx\n", >>>>>> cmd, nr, arg); >>>>>>
