Am 06.12.22 um 13:55 schrieb Charan Teja Kalla:
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.
No, that's not really clean either.
Give me 10 Minutes, going to come up with something.
Regards, Christian.
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);--------------------><-----------------------------
Thanks for pointing this out, Christian.