Thanks Christian/TJ for all your inputs!!
On 11/24/2022 6:25 PM, Christian König wrote:
>>> I was already wondering why the order is this way.
>>>
>>> Why is dma_buf_stats_setup() needing the file in the first place?
>>
>> dmabuf->file will be used in dma_buf_stats_setup(), the
>> dma_buf_stats_setup() as follows:
>>
>>> 171 int dma_buf_stats_setup(struct dma_buf *dmabuf)
>>> 172 {
>>> 173 struct dma_buf_sysfs_entry *sysfs_entry;
>>> 174 int ret;
>>> 175
>>> 176 if (!dmabuf || !dmabuf->file)
>>> 177 return -EINVAL;
>>> 178
>>> 179 if (!dmabuf->exp_name) {
>>> 180 pr_err("exporter name must not be empty if stats
>>> needed\n");
>>> 181 return -EINVAL;
>>> 182 }
>>> 183
>>> 184 sysfs_entry = kzalloc(sizeof(struct dma_buf_sysfs_entry),
>>> GFP_KERNEL);
>>> 185 if (!sysfs_entry)
>>> 186 return -ENOMEM;
>>> 187
>>> 188 sysfs_entry->kobj.kset = dma_buf_per_buffer_stats_kset;
>>> 189 sysfs_entry->dmabuf = dmabuf;
>>> 190
>>> 191 dmabuf->sysfs_entry = sysfs_entry;
>>> 192
>>> 193 /* create the directory for buffer stats */
>>> 194 ret = kobject_init_and_add(&sysfs_entry->kobj,
>>> &dma_buf_ktype, NULL,
>>> 195 "%lu",
>>> file_inode(dmabuf->file)->i_ino);
>
> Ah, so it uses the i_ino of the file for the sysfs unique name.
>
> I'm going to take another look how to properly clean this up.
>
How about deleting the dmabuf from the db_list directly in the error
path (which is usually done by the fput()) and then continue with the
normal fput() here.
Just compile tested the below code and If the logic make sense for you,
will send the final tested patch.
----------------------><---------------------------------------------
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index e6f36c0..10a1727 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -87,19 +87,28 @@ static void dma_buf_release(struct dentry *dentry)
kfree(dmabuf);
}
-static int dma_buf_file_release(struct inode *inode, struct file *file)
+static void dma_buf_db_list_remove(struct file *file)
{
struct dma_buf *dmabuf;
- if (!is_dma_buf_file(file))
- return -EINVAL;
-
dmabuf = file->private_data;
+ if (!dmabuf)
+ return;
mutex_lock(&db_list.lock);
list_del(&dmabuf->list_node);
mutex_unlock(&db_list.lock);
+ file->private_data = NULL;
+}
+
+static int dma_buf_file_release(struct inode *inode, struct file *file)
+{
+ if (!is_dma_buf_file(file))
+ return -EINVAL;
+
+ dma_buf_db_list_remove(file);
+
return 0;
}
@@ -688,6 +697,8 @@ struct dma_buf *dma_buf_export(const struct
dma_buf_export_info *exp_info)
* early before calling the release() dma_buf op.
*/
file->f_path.dentry->d_fsdata = NULL;
+
+ dma_buf_db_list_remove(file);
fput(file);
err_dmabuf:
kfree(dmabuf);
--------------------><-----------------------------
> Thanks for pointing this out,
> Christian.
