Hi Dawei,
On 30/05/25 18:20, Dawei Li wrote:
> HI Beleswar,
>
> Thanks for reviewing.
>
> On Fri, May 30, 2025 at 03:15:28PM +0530, Beleswar Prasad Padhi wrote:
>> Hi Dawei,
>>
>> On 19/05/25 20:38, Dawei Li wrote:
>>> Implement RPMSG_CREATE_EPT_FD_IOCTL, new uAPI for rpmsg ctrl, which
>>> shares most of operations of RPMSG_CREATE_EPT_IOCTL except that it
>>> returns fd representing eptdev to userspace directly.
>>>
>>> Possible calling procedures for userspace are:
>>> - fd = open("/dev/rpmsg_ctrlX")
>>> - ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &info);
>>> - fd_ep = info.fd
>>
>> We are returning a new fd to userspace from inside an IOCTL itself. Is this a
>> standard way of doing things in Kernel space? (see below related comment)
> Yes, anon_get_{fd,file} are used extensively in kernel for returning a new
> fd to userspace which is associated with an unique data structure in kernel
> space, in different ways:
>
> - via ioctl(), some examples are:
>
> - KVM ioctl(s)
> - KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
> - KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
> - KVM_CREATE_DEVICE -> kvm_ioctl_create_device
> - KVM_CREATE_VM -> kvm_dev_ioctl_create_vm
>
> - DMA buf/fence/sync ioctls
> - DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
> - SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
> - Couples of driver implement DMA buf by using anon file _implicitly_:
> - UDMABUF_CREATE -> udmabuf_ioctl_create
> - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate
>
> - gpiolib ioctls:
> - GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
> - GPIO_V2_GET_LINE_IOCTL
>
> - IOMMUFD ioctls:
>
> - VFIO Ioctls:
>
> - ....
>
>
> - via other specific syscalls:
> - epoll_create1
> - bpf
> - perf_event_open
> - inotify_init
> - ...
Thank you for the extensive list of examples!
>
>>> - operations on fd_ep(write, read, poll ioctl)
>>> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
>>> - close(fd_ep)
>>
>> Can we rely on the userspace to close() the fd_ep? (if not done, could be a
>> memory leak..).. Opposed to fd, which we can rely on the userspace to
>> close() since they initiated the open() call. I am just trying to understand
>> if
>> this is a standard way of doing things...
> Good question.
>
> When userland gets a fd from kernel, it's userland's duty to manage and
> release
> the resource when it's done with it, because kernel never knows when the fd
> and
> its resourcen are not needed by userland except process is on exiting. The
> fact
> remains true no matter how fd is generated from kernel:
> - open()
> - ioctl()
> - Other syscalls(epoll_create1, e.g, as listed above)
>
> As a result, kernel & driver provide fops->release() to achieve resource
> release when fd is not needed for userland, some callers of it maybe:
> - Userland call close() explicitly
> - Kernel does the dirty job when user process exits(if some fds are
> still opened):
> - Userland call exit() explicitly.
> - User process was killed by some signals.
>
> Maybe some comments/docs are needed in uAPI?
Perhaps yes. It makes sense to me now. Thanks for addressing my queries!
>
>>> - close(fd)
>>>
> [snip]
>
>>> +
>>> + if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
>>> + cmd == RPMSG_RELEASE_DEV_IOCTL) {
>>> + if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>> + return -EFAULT;
>>> +
>>> + memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>>> + chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>>> + chinfo.src = eptinfo.src;
>>> + chinfo.dst = eptinfo.dst;
>>> + } else if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {
>>
>> Maybe we can put this 'else if condition' in the first 'if' and treat other
>> conditions under 'else', as 'RPMSG_CREATE_EPT_FD_IOCTL' is the only
>> ioctl with a different struct type.
> Good point! I will try to address it in next respin.
Thanks,
Beleswar
>
>> Thanks,
>> Beleswar
>>
>>> + if (copy_from_user(&ept_fd_info, argp, sizeof(ept_fd_info)))
>>> + return -EFAULT;
>>> +
>>> + memcpy(chinfo.name, ept_fd_info.name, RPMSG_NAME_SIZE);
>>> + chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>>> + chinfo.src = ept_fd_info.src;
>>> + chinfo.dst = ept_fd_info.dst;
>>> + }
>>>
> [snip]
>
> Thanks,
>
> Dawei
