Re: [PATCH v3] dma-buf: Add a capabilities directory
On Tue, May 31, 2022 at 07:53:50AM -0500, Jason Ekstrand wrote: > On Mon, 2022-05-30 at 10:26 +0200, Greg KH wrote: > > On Mon, May 30, 2022 at 08:15:04AM +, Simon Ser wrote: > > > On Monday, May 30th, 2022 at 09:20, Greg KH > > > wrote: > > > > > > > > > +static struct attribute *dma_buf_caps_attrs[] = { > > > > > > + _buf_sync_file_import_export_attr.attr, > > > > > > + NULL, > > > > > > +}; > > > > > > + > > > > > > +static const struct attribute_group dma_buf_caps_attr_group > > > > > > = { > > > > > > + .attrs = dma_buf_caps_attrs, > > > > > > +}; > > > > > > > > > > Didn't we had macros for those? I think I have seen something > > > > > for that. > > > > > > > > Yes, please use ATTRIBUTE_GROUPS() > > > > > > This doesn't allow the user to set a group name, and creates an > > > unused > > > "_groups" variable, causing warnings. > > > > Then set a group name. > > > > But you really want to almost always be using lists of groups, which > > is > > why that macro works that way. > > I think I see the confusion here. The ATTRIBUTE_GROUPS() macro is > intended for device drivers and to be used with add_device(). However, > this is dma-buf so there is no device and no add_device() call to hook. > Unless there are other magic macros to use in this case, I think we're > stuck doing it manually. Have a list of attribute groups and add it to the kobject when you create it so they all get created at the same time. Don't do piece-meal "add one, and then another, and then another" as that just gets messy and complex and impossible to unwind the error conditions from. sysfs_create_groups() is what you need to use here. I need to drop sysfs_create_group() one day... thanks, greg k-h
Re: [PATCH v3] dma-buf: Add a capabilities directory
On Mon, 2022-05-30 at 10:26 +0200, Greg KH wrote: > On Mon, May 30, 2022 at 08:15:04AM +, Simon Ser wrote: > > On Monday, May 30th, 2022 at 09:20, Greg KH > > wrote: > > > > > > > +static struct attribute *dma_buf_caps_attrs[] = { > > > > > + _buf_sync_file_import_export_attr.attr, > > > > > + NULL, > > > > > +}; > > > > > + > > > > > +static const struct attribute_group dma_buf_caps_attr_group > > > > > = { > > > > > + .attrs = dma_buf_caps_attrs, > > > > > +}; > > > > > > > > Didn't we had macros for those? I think I have seen something > > > > for that. > > > > > > Yes, please use ATTRIBUTE_GROUPS() > > > > This doesn't allow the user to set a group name, and creates an > > unused > > "_groups" variable, causing warnings. > > Then set a group name. > > But you really want to almost always be using lists of groups, which > is > why that macro works that way. I think I see the confusion here. The ATTRIBUTE_GROUPS() macro is intended for device drivers and to be used with add_device(). However, this is dma-buf so there is no device and no add_device() call to hook. Unless there are other magic macros to use in this case, I think we're stuck doing it manually. --Jason > > > > > > > + > > > > > +static struct kobject *dma_buf_caps_kobj; > > > > > + > > > > > +int dma_buf_init_sysfs_capabilities(struct kset *kset) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + dma_buf_caps_kobj = kobject_create_and_add("caps", > > > > > >kobj); > > > > > + if (!dma_buf_caps_kobj) > > > > > + return -ENOMEM; > > > > > + > > > > > + ret = sysfs_create_group(dma_buf_caps_kobj, > > > > > _buf_caps_attr_group); > > > > > > Why do we have "raw" kobjects here? > > > > > > A group can have a name, which puts it in the subdirectory of the > > > object > > > it is attached to. Please do that and do not create a new > > > kobject. > > > > I see, I'll switch to sysfs_create_group with a group name in the > > next version. > > No, do not do that, add it to the list of groups for the original > kobject. > > thanks, > > greg k-h
Re: [PATCH v3] dma-buf: Add a capabilities directory
On Mon, May 30, 2022 at 08:15:04AM +, Simon Ser wrote: > On Monday, May 30th, 2022 at 09:20, Greg KH > wrote: > > > > > +static struct attribute *dma_buf_caps_attrs[] = { > > > > + _buf_sync_file_import_export_attr.attr, > > > > + NULL, > > > > +}; > > > > + > > > > +static const struct attribute_group dma_buf_caps_attr_group = { > > > > + .attrs = dma_buf_caps_attrs, > > > > +}; > > > > > > Didn't we had macros for those? I think I have seen something for that. > > > > Yes, please use ATTRIBUTE_GROUPS() > > This doesn't allow the user to set a group name, and creates an unused > "_groups" variable, causing warnings. Then set a group name. But you really want to almost always be using lists of groups, which is why that macro works that way. > > > > > + > > > > +static struct kobject *dma_buf_caps_kobj; > > > > + > > > > +int dma_buf_init_sysfs_capabilities(struct kset *kset) > > > > +{ > > > > + int ret; > > > > + > > > > + dma_buf_caps_kobj = kobject_create_and_add("caps", >kobj); > > > > + if (!dma_buf_caps_kobj) > > > > + return -ENOMEM; > > > > + > > > > + ret = sysfs_create_group(dma_buf_caps_kobj, > > > > _buf_caps_attr_group); > > > > Why do we have "raw" kobjects here? > > > > A group can have a name, which puts it in the subdirectory of the object > > it is attached to. Please do that and do not create a new kobject. > > I see, I'll switch to sysfs_create_group with a group name in the next > version. No, do not do that, add it to the list of groups for the original kobject. thanks, greg k-h
Re: [PATCH v3] dma-buf: Add a capabilities directory
On Monday, May 30th, 2022 at 09:20, Greg KH wrote: > > > +static struct attribute *dma_buf_caps_attrs[] = { > > > + _buf_sync_file_import_export_attr.attr, > > > + NULL, > > > +}; > > > + > > > +static const struct attribute_group dma_buf_caps_attr_group = { > > > + .attrs = dma_buf_caps_attrs, > > > +}; > > > > Didn't we had macros for those? I think I have seen something for that. > > Yes, please use ATTRIBUTE_GROUPS() This doesn't allow the user to set a group name, and creates an unused "_groups" variable, causing warnings. > > > + > > > +static struct kobject *dma_buf_caps_kobj; > > > + > > > +int dma_buf_init_sysfs_capabilities(struct kset *kset) > > > +{ > > > + int ret; > > > + > > > + dma_buf_caps_kobj = kobject_create_and_add("caps", >kobj); > > > + if (!dma_buf_caps_kobj) > > > + return -ENOMEM; > > > + > > > + ret = sysfs_create_group(dma_buf_caps_kobj, _buf_caps_attr_group); > > Why do we have "raw" kobjects here? > > A group can have a name, which puts it in the subdirectory of the object > it is attached to. Please do that and do not create a new kobject. I see, I'll switch to sysfs_create_group with a group name in the next version. Thanks for the pointers!
Re: [PATCH v3] dma-buf: Add a capabilities directory
On Monday, May 30th, 2022 at 09:09, Christian König wrote: > I find a separate directory rather unusual, but can't come up with any > better idea either. > > IIRC the security module had a mask file with names for the supported > capabilities. Are you referring to /sys/kernel/security/lsm? This sounds more painful to parse from user-space. Instead of a simple stat(), user-space would need to read the file, split on commas, and compare strings.
Re: [PATCH v3] dma-buf: Add a capabilities directory
On Mon, May 30, 2022 at 09:09:30AM +0200, Christian König wrote: > Am 27.05.22 um 09:34 schrieb Simon Ser: > > To discover support for new DMA-BUF IOCTLs, user-space has no > > choice but to try to perform the IOCTL on an existing DMA-BUF. > > However, user-space may want to figure out whether or not the > > IOCTL is available before it has a DMA-BUF at hand, e.g. at > > initialization time in a Wayland compositor. > > > > Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF > > subsystem to advertise supported features. Add a > > sync_file_import_export entry which indicates that importing and > > exporting sync_files from/to DMA-BUFs is supported. > > I find a separate directory rather unusual, but can't come up with any > better idea either. > > IIRC the security module had a mask file with names for the supported > capabilities. > > > > > v2: Add missing files lost in a rebase > > > > v3: > > - Create separate file in Documentation/ABI/testing/, add it to > >MAINTAINERS > > - Fix kernel version (Daniel) > > - Remove unnecessary brackets (Jason) > > - Fix SPDX comment style > > > > Signed-off-by: Simon Ser > > Reviewed-by: Jason Ekstrand > > Cc: Daniel Vetter > > Cc: Bas Nieuwenhuizen > > Cc: Christian König > > Cc: Greg KH > > --- > > .../ABI/testing/sysfs-kernel-dmabuf-caps | 13 + > > MAINTAINERS | 1 + > > drivers/dma-buf/Makefile | 2 +- > > drivers/dma-buf/dma-buf-sysfs-caps.c | 51 +++ > > drivers/dma-buf/dma-buf-sysfs-caps.h | 16 ++ > > drivers/dma-buf/dma-buf-sysfs-stats.c | 16 ++ > > drivers/dma-buf/dma-buf-sysfs-stats.h | 6 ++- > > drivers/dma-buf/dma-buf.c | 43 ++-- > > include/uapi/linux/dma-buf.h | 6 +++ > > 9 files changed, 134 insertions(+), 20 deletions(-) > > create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.c > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.h > > > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > > b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > > new file mode 100644 > > index ..f83af422fd18 > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > > @@ -0,0 +1,13 @@ > > +What: /sys/kernel/dmabuf/caps > > +Date: May 2022 > > +KernelVersion: v5.20 > > +Contact: Simon Ser > > +Description: This directory advertises DMA-BUF capabilities > > supported by the > > + kernel. > > + > > +What: /sys/kernel/dmabuf/caps/sync_file_import_export > > +Date: May 2022 > > +KernelVersion: v5.20 > > +Contact: Simon Ser > > +Description: This file is read-only and advertises support for > > importing and > > + exporting sync_files from/to DMA-BUFs. > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 11da16bfa123..8966686f7231 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -5871,6 +5871,7 @@ L:dri-devel@lists.freedesktop.org > > L:linaro-mm-...@lists.linaro.org (moderated for non-subscribers) > > S:Maintained > > T:git git://anongit.freedesktop.org/drm/drm-misc > > +F: Documentation/ABI/testing/sysfs-kernel-dmabuf-caps > > F:Documentation/driver-api/dma-buf.rst > > F:drivers/dma-buf/ > > F:include/linux/*fence.h > > diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile > > index 4c9eb53ba3f8..afc874272710 100644 > > --- a/drivers/dma-buf/Makefile > > +++ b/drivers/dma-buf/Makefile > > @@ -1,6 +1,6 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \ > > -dma-resv.o > > +dma-resv.o dma-buf-sysfs-caps.o > > obj-$(CONFIG_DMABUF_HEAPS)+= dma-heap.o > > obj-$(CONFIG_DMABUF_HEAPS)+= heaps/ > > obj-$(CONFIG_SYNC_FILE) += sync_file.o > > diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.c > > b/drivers/dma-buf/dma-buf-sysfs-caps.c > > new file mode 100644 > > index ..82b91eb874a9 > > --- /dev/null > > +++ b/drivers/dma-buf/dma-buf-sysfs-caps.c > > @@ -0,0 +1,51 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * DMA-BUF sysfs capabilities. > > + * > > + * Copyright (C) 2022 Simon Ser > > + */ > > + > > +#include > > +#include > > + > > +#include "dma-buf-sysfs-caps.h" > > + > > +static ssize_t sync_file_import_export_show(struct kobject *kobj, > > + struct kobj_attribute *attr, > > + char *buf) > > +{ > > + return sysfs_emit(buf, "1\n"); > > +} > > + > > +static struct kobj_attribute dma_buf_sync_file_import_export_attr = > > + __ATTR_RO(sync_file_import_export); > > + > > +static struct attribute *dma_buf_caps_attrs[] = { > > +
Re: [PATCH v3] dma-buf: Add a capabilities directory
Am 27.05.22 um 09:34 schrieb Simon Ser: To discover support for new DMA-BUF IOCTLs, user-space has no choice but to try to perform the IOCTL on an existing DMA-BUF. However, user-space may want to figure out whether or not the IOCTL is available before it has a DMA-BUF at hand, e.g. at initialization time in a Wayland compositor. Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF subsystem to advertise supported features. Add a sync_file_import_export entry which indicates that importing and exporting sync_files from/to DMA-BUFs is supported. I find a separate directory rather unusual, but can't come up with any better idea either. IIRC the security module had a mask file with names for the supported capabilities. v2: Add missing files lost in a rebase v3: - Create separate file in Documentation/ABI/testing/, add it to MAINTAINERS - Fix kernel version (Daniel) - Remove unnecessary brackets (Jason) - Fix SPDX comment style Signed-off-by: Simon Ser Reviewed-by: Jason Ekstrand Cc: Daniel Vetter Cc: Bas Nieuwenhuizen Cc: Christian König Cc: Greg KH --- .../ABI/testing/sysfs-kernel-dmabuf-caps | 13 + MAINTAINERS | 1 + drivers/dma-buf/Makefile | 2 +- drivers/dma-buf/dma-buf-sysfs-caps.c | 51 +++ drivers/dma-buf/dma-buf-sysfs-caps.h | 16 ++ drivers/dma-buf/dma-buf-sysfs-stats.c | 16 ++ drivers/dma-buf/dma-buf-sysfs-stats.h | 6 ++- drivers/dma-buf/dma-buf.c | 43 ++-- include/uapi/linux/dma-buf.h | 6 +++ 9 files changed, 134 insertions(+), 20 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-caps create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.c create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.h diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps new file mode 100644 index ..f83af422fd18 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps @@ -0,0 +1,13 @@ +What: /sys/kernel/dmabuf/caps +Date: May 2022 +KernelVersion: v5.20 +Contact: Simon Ser +Description: This directory advertises DMA-BUF capabilities supported by the + kernel. + +What: /sys/kernel/dmabuf/caps/sync_file_import_export +Date: May 2022 +KernelVersion: v5.20 +Contact: Simon Ser +Description: This file is read-only and advertises support for importing and + exporting sync_files from/to DMA-BUFs. diff --git a/MAINTAINERS b/MAINTAINERS index 11da16bfa123..8966686f7231 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5871,6 +5871,7 @@ L:dri-devel@lists.freedesktop.org L:linaro-mm-...@lists.linaro.org (moderated for non-subscribers) S:Maintained T:git git://anongit.freedesktop.org/drm/drm-misc +F: Documentation/ABI/testing/sysfs-kernel-dmabuf-caps F:Documentation/driver-api/dma-buf.rst F:drivers/dma-buf/ F:include/linux/*fence.h diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 4c9eb53ba3f8..afc874272710 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \ -dma-resv.o +dma-resv.o dma-buf-sysfs-caps.o obj-$(CONFIG_DMABUF_HEAPS)+= dma-heap.o obj-$(CONFIG_DMABUF_HEAPS)+= heaps/ obj-$(CONFIG_SYNC_FILE) += sync_file.o diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.c b/drivers/dma-buf/dma-buf-sysfs-caps.c new file mode 100644 index ..82b91eb874a9 --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-caps.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * DMA-BUF sysfs capabilities. + * + * Copyright (C) 2022 Simon Ser + */ + +#include +#include + +#include "dma-buf-sysfs-caps.h" + +static ssize_t sync_file_import_export_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "1\n"); +} + +static struct kobj_attribute dma_buf_sync_file_import_export_attr = + __ATTR_RO(sync_file_import_export); + +static struct attribute *dma_buf_caps_attrs[] = { + _buf_sync_file_import_export_attr.attr, + NULL, +}; + +static const struct attribute_group dma_buf_caps_attr_group = { + .attrs = dma_buf_caps_attrs, +}; Didn't we had macros for those? I think I have seen something for that. Apart from those two random thoughts looks good to me as well. Regards, Christian. + +static struct kobject *dma_buf_caps_kobj; + +int dma_buf_init_sysfs_capabilities(struct kset *kset) +{ + int ret; + + dma_buf_caps_kobj = kobject_create_and_add("caps", >kobj); + if (!dma_buf_caps_kobj) +
[PATCH v3] dma-buf: Add a capabilities directory
To discover support for new DMA-BUF IOCTLs, user-space has no choice but to try to perform the IOCTL on an existing DMA-BUF. However, user-space may want to figure out whether or not the IOCTL is available before it has a DMA-BUF at hand, e.g. at initialization time in a Wayland compositor. Add a /sys/kernel/dmabuf/caps directory which allows the DMA-BUF subsystem to advertise supported features. Add a sync_file_import_export entry which indicates that importing and exporting sync_files from/to DMA-BUFs is supported. v2: Add missing files lost in a rebase v3: - Create separate file in Documentation/ABI/testing/, add it to MAINTAINERS - Fix kernel version (Daniel) - Remove unnecessary brackets (Jason) - Fix SPDX comment style Signed-off-by: Simon Ser Reviewed-by: Jason Ekstrand Cc: Daniel Vetter Cc: Bas Nieuwenhuizen Cc: Christian König Cc: Greg KH --- .../ABI/testing/sysfs-kernel-dmabuf-caps | 13 + MAINTAINERS | 1 + drivers/dma-buf/Makefile | 2 +- drivers/dma-buf/dma-buf-sysfs-caps.c | 51 +++ drivers/dma-buf/dma-buf-sysfs-caps.h | 16 ++ drivers/dma-buf/dma-buf-sysfs-stats.c | 16 ++ drivers/dma-buf/dma-buf-sysfs-stats.h | 6 ++- drivers/dma-buf/dma-buf.c | 43 ++-- include/uapi/linux/dma-buf.h | 6 +++ 9 files changed, 134 insertions(+), 20 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-caps create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.c create mode 100644 drivers/dma-buf/dma-buf-sysfs-caps.h diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps new file mode 100644 index ..f83af422fd18 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-caps @@ -0,0 +1,13 @@ +What: /sys/kernel/dmabuf/caps +Date: May 2022 +KernelVersion: v5.20 +Contact: Simon Ser +Description: This directory advertises DMA-BUF capabilities supported by the + kernel. + +What: /sys/kernel/dmabuf/caps/sync_file_import_export +Date: May 2022 +KernelVersion: v5.20 +Contact: Simon Ser +Description: This file is read-only and advertises support for importing and + exporting sync_files from/to DMA-BUFs. diff --git a/MAINTAINERS b/MAINTAINERS index 11da16bfa123..8966686f7231 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5871,6 +5871,7 @@ L:dri-devel@lists.freedesktop.org L: linaro-mm-...@lists.linaro.org (moderated for non-subscribers) S: Maintained T: git git://anongit.freedesktop.org/drm/drm-misc +F: Documentation/ABI/testing/sysfs-kernel-dmabuf-caps F: Documentation/driver-api/dma-buf.rst F: drivers/dma-buf/ F: include/linux/*fence.h diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 4c9eb53ba3f8..afc874272710 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \ -dma-resv.o +dma-resv.o dma-buf-sysfs-caps.o obj-$(CONFIG_DMABUF_HEAPS) += dma-heap.o obj-$(CONFIG_DMABUF_HEAPS) += heaps/ obj-$(CONFIG_SYNC_FILE)+= sync_file.o diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.c b/drivers/dma-buf/dma-buf-sysfs-caps.c new file mode 100644 index ..82b91eb874a9 --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-caps.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * DMA-BUF sysfs capabilities. + * + * Copyright (C) 2022 Simon Ser + */ + +#include +#include + +#include "dma-buf-sysfs-caps.h" + +static ssize_t sync_file_import_export_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "1\n"); +} + +static struct kobj_attribute dma_buf_sync_file_import_export_attr = + __ATTR_RO(sync_file_import_export); + +static struct attribute *dma_buf_caps_attrs[] = { + _buf_sync_file_import_export_attr.attr, + NULL, +}; + +static const struct attribute_group dma_buf_caps_attr_group = { + .attrs = dma_buf_caps_attrs, +}; + +static struct kobject *dma_buf_caps_kobj; + +int dma_buf_init_sysfs_capabilities(struct kset *kset) +{ + int ret; + + dma_buf_caps_kobj = kobject_create_and_add("caps", >kobj); + if (!dma_buf_caps_kobj) + return -ENOMEM; + + ret = sysfs_create_group(dma_buf_caps_kobj, _buf_caps_attr_group); + if (ret) + kobject_put(dma_buf_caps_kobj); + return ret; +} + +void dma_buf_uninit_sysfs_capabilities(void) +{ + kobject_put(dma_buf_caps_kobj); +} diff --git a/drivers/dma-buf/dma-buf-sysfs-caps.h b/drivers/dma-buf/dma-buf-sysfs-caps.h new file mode 100644