[PATCH] dma-buf: fix dma_buf_export init order
Christian König
ckoenig.leichtzumerken at gmail.com
Tue Dec 6 18:17:19 UTC 2022
Am 06.12.22 um 17:20 schrieb Ruhl, Michael J:
>> -----Original Message-----
>> From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of
>> Christian König
>> Sent: Tuesday, December 6, 2022 10:12 AM
>> To: quic_charante at quicinc.com; cuigaosheng1 at huawei.com;
>> tjmercier at google.com; sumit.semwal at linaro.org
>> Cc: linaro-mm-sig at lists.linaro.org; dri-devel at lists.freedesktop.org; linux-
>> media at vger.kernel.org
>> Subject: [PATCH] dma-buf: fix dma_buf_export init order
>>
>> The init order and resulting error handling in dma_buf_export
>> was pretty messy.
>>
>> Subordinate objects like the file and the sysfs kernel objects
>> were initializing and wiring itself up with the object in the
>> wrong order resulting not only in complicating and partially
>> incorrect error handling, but also in publishing only halve
>> initialized DMA-buf objects.
>>
>> Clean this up thoughtfully by allocating the file independent
>> of the DMA-buf object. Then allocate and initialize the DMA-buf
>> object itself, before publishing it through sysfs. If everything
>> works as expected the file is then connected with the DMA-buf
>> object and publish it through debugfs.
>>
>> Also adds the missing dma_resv_fini() into the error handling.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>> drivers/dma-buf/dma-buf-sysfs-stats.c | 7 +--
>> drivers/dma-buf/dma-buf-sysfs-stats.h | 4 +-
>> drivers/dma-buf/dma-buf.c | 65 +++++++++++++--------------
>> 3 files changed, 34 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-
>> buf-sysfs-stats.c
>> index 2bba0babcb62..4b680e10c15a 100644
>> --- a/drivers/dma-buf/dma-buf-sysfs-stats.c
>> +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
>> @@ -168,14 +168,11 @@ void dma_buf_uninit_sysfs_statistics(void)
>> kset_unregister(dma_buf_stats_kset);
>> }
>>
>> -int dma_buf_stats_setup(struct dma_buf *dmabuf)
>> +int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file)
>> {
>> struct dma_buf_sysfs_entry *sysfs_entry;
>> int ret;
>>
>> - if (!dmabuf || !dmabuf->file)
>> - return -EINVAL;
>> -
>> if (!dmabuf->exp_name) {
>> pr_err("exporter name must not be empty if stats
>> needed\n");
>> return -EINVAL;
>> @@ -192,7 +189,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf)
>>
>> /* create the directory for buffer stats */
>> ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype,
>> NULL,
>> - "%lu", file_inode(dmabuf->file)->i_ino);
>> + "%lu", file_inode(file)->i_ino);
>> if (ret)
>> goto err_sysfs_dmabuf;
>>
>> diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-
>> buf-sysfs-stats.h
>> index a49c6e2650cc..7a8a995b75ba 100644
>> --- a/drivers/dma-buf/dma-buf-sysfs-stats.h
>> +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h
>> @@ -13,7 +13,7 @@
>> int dma_buf_init_sysfs_statistics(void);
>> void dma_buf_uninit_sysfs_statistics(void);
>>
>> -int dma_buf_stats_setup(struct dma_buf *dmabuf);
>> +int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file);
>>
>> void dma_buf_stats_teardown(struct dma_buf *dmabuf);
>> #else
>> @@ -25,7 +25,7 @@ static inline int dma_buf_init_sysfs_statistics(void)
>>
>> static inline void dma_buf_uninit_sysfs_statistics(void) {}
>>
>> -static inline int dma_buf_stats_setup(struct dma_buf *dmabuf)
>> +static inline int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file
>> *file)
>> {
>> return 0;
>> }
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index e6f36c014c4c..ea08049b70ae 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -614,19 +614,11 @@ struct dma_buf *dma_buf_export(const struct
>> dma_buf_export_info *exp_info)
>> size_t alloc_size = sizeof(struct dma_buf);
>> int ret;
>>
>> - if (!exp_info->resv)
>> - alloc_size += sizeof(struct dma_resv);
>> - else
>> - /* prevent &dma_buf[1] == dma_buf->resv */
>> - alloc_size += 1;
>> -
>> - if (WARN_ON(!exp_info->priv
>> - || !exp_info->ops
>> - || !exp_info->ops->map_dma_buf
>> - || !exp_info->ops->unmap_dma_buf
>> - || !exp_info->ops->release)) {
>> + if (WARN_ON(!exp_info->priv || !exp_info->ops
>> + || !exp_info->ops->map_dma_buf
>> + || !exp_info->ops->unmap_dma_buf
>> + || !exp_info->ops->release))
>> return ERR_PTR(-EINVAL);
>> - }
>>
>> if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
>> (exp_info->ops->pin || exp_info->ops->unpin)))
>> @@ -638,10 +630,21 @@ struct dma_buf *dma_buf_export(const struct
>> dma_buf_export_info *exp_info)
>> if (!try_module_get(exp_info->owner))
>> return ERR_PTR(-ENOENT);
>>
>> + file = dma_buf_getfile(exp_info->size, exp_info->flags);
> Hi Christian,
>
> dma_buf_getfile takes a dmabuf, here you have a size?
>
> Did you change this function somewhere?
Ups forgot to add that change to the patch. I shouldn't code when I'm in
a hurry.
Addressed this and Charans comment in v2.
Thanks,
Christian.
>
> with that addressed....
>
> This cleanup makes sense to me.
>
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
>
> M
>
>> + if (IS_ERR(file)) {
>> + ret = PTR_ERR(file);
>> + goto err_module;
>> + }
>> +
>> + if (!exp_info->resv)
>> + alloc_size += sizeof(struct dma_resv);
>> + else
>> + /* prevent &dma_buf[1] == dma_buf->resv */
>> + alloc_size += 1;
>> dmabuf = kzalloc(alloc_size, GFP_KERNEL);
>> if (!dmabuf) {
>> ret = -ENOMEM;
>> - goto err_module;
>> + goto err_file;
>> }
>>
>> dmabuf->priv = exp_info->priv;
>> @@ -653,44 +656,36 @@ struct dma_buf *dma_buf_export(const struct
>> dma_buf_export_info *exp_info)
>> init_waitqueue_head(&dmabuf->poll);
>> dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
>> dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
>> + mutex_init(&dmabuf->lock);
>> + INIT_LIST_HEAD(&dmabuf->attachments);
>>
>> if (!resv) {
>> - resv = (struct dma_resv *)&dmabuf[1];
>> - dma_resv_init(resv);
>> + dmabuf->resv = (struct dma_resv *)&dmabuf[1];
>> + dma_resv_init(dmabuf->resv);
>> + } else {
>> + dmabuf->resv = resv;
>> }
>> - dmabuf->resv = resv;
>>
>> - file = dma_buf_getfile(dmabuf, exp_info->flags);
>> - if (IS_ERR(file)) {
>> - ret = PTR_ERR(file);
>> + ret = dma_buf_stats_setup(dmabuf, file);
>> + if (ret)
>> goto err_dmabuf;
>> - }
>>
>> + file->private_data = dmabuf;
>> + file->f_path.dentry->d_fsdata = dmabuf;
>> dmabuf->file = file;
>>
>> - mutex_init(&dmabuf->lock);
>> - INIT_LIST_HEAD(&dmabuf->attachments);
>> -
>> mutex_lock(&db_list.lock);
>> list_add(&dmabuf->list_node, &db_list.head);
>> mutex_unlock(&db_list.lock);
>>
>> - ret = dma_buf_stats_setup(dmabuf);
>> - if (ret)
>> - goto err_sysfs;
>> -
>> return dmabuf;
>>
>> -err_sysfs:
>> - /*
>> - * Set file->f_path.dentry->d_fsdata to NULL so that when
>> - * dma_buf_release() gets invoked by dentry_ops, it exits
>> - * early before calling the release() dma_buf op.
>> - */
>> - file->f_path.dentry->d_fsdata = NULL;
>> - fput(file);
>> err_dmabuf:
>> + if (!resv)
>> + dma_resv_fini(dmabuf->resv);
>> kfree(dmabuf);
>> +err_file:
>> + fput(file);
>> err_module:
>> module_put(exp_info->owner);
>> return ERR_PTR(ret);
>> --
>> 2.34.1
More information about the dri-devel
mailing list