Hi Yicong,

On 2023/10/19 11:05, Yicong Yang wrote:

On 2023/10/12 17:47, Junhao He wrote:

Use spinlock replace mutex to control driver data access to one at a time. But the function copy_to_user() may sleep so spinlock do not to lock it.

Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver") Signed-off-by: Junhao He hejunhao3@huawei.com

---

drivers/hwtracing/coresight/ultrasoc-smb.c | 36 ++++++++++------------ drivers/hwtracing/coresight/ultrasoc-smb.h | 6 ++-- 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c index e9a32a97fbee..b08a619d1116 100644 --- a/drivers/hwtracing/coresight/ultrasoc-smb.c +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c @@ -99,7 +99,7 @@ static int smb_open(struct inode *inode, struct file *file) struct smb_drv_data, miscdev); int ret = 0;

• mutex_lock(&drvdata->mutex);

• spin_lock(&drvdata->spinlock);

if (drvdata->reading) { ret = -EBUSY; @@ -115,7 +115,7 @@ static int smb_open(struct inode *inode, struct file *file) drvdata->reading = true; out:

• mutex_unlock(&drvdata->mutex);

• spin_unlock(&drvdata->spinlock);

return ret; } @@ -132,10 +132,8 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len, if (!len) return 0;

• mutex_lock(&drvdata->mutex);
• if (!sdb->data_size)

• goto out;
  

• return 0;
  

to_copy = min(sdb->data_size, len); @@ -145,20 +143,18 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len, if (copy_to_user(data, sdb->buf_base + sdb->buf_rdptr, to_copy)) { dev_dbg(dev, "Failed to copy data to user\n");

• to_copy = -EFAULT;
  

• goto out;
  

• return -EFAULT;
  

  }

• spin_lock(&drvdata->spinlock); *ppos += to_copy;

• smb_update_read_ptr(drvdata, to_copy);

• dev_dbg(dev, "%zu bytes copied\n", to_copy);

-out: if (!sdb->data_size) smb_reset_buffer(drvdata);

• mutex_unlock(&drvdata->mutex);

• spin_unlock(&drvdata->spinlock);

Do we still need the lock here? If we get here, we should have the exclusive access to the file, which is protected in open(). Or any other cases?

This is something I've also considered. If someone opens an SMB device and reads it using multithreading, The SMB device will got out of sync. I've seen other coresight sink drivers such as etb do not use locks in the read function. Maybe it's not necessary here, the buffer synchronization is guaranteed by the user.

Best regards, Junhao.