From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id F392BCDB465 for ; Thu, 19 Oct 2023 12:45:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From:CC: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=ZAPo8LOwkw/70TJ3BZwgWXDq73yER5sbSdxjv+zW10g=; b=HENim6bcT0mQcLYWW51Oc95IJB ziCq0IPytPg61PcP1VVMPcY+ovzXx42me07DOAC4xkylnTjwLvWwMM14AjSLi+9fZ97UWzqwkCvnQ +JhmvvLffj2DPwld/785e13ool5nq2fuXwFnzTs/wP3bmgtk7kfWJIkaMtDNHdHIaKz7EJH9Gld9h 37m/srTHxkdYfpt3i4G7DatzYJsj1qeSG6jGYRhPQbWJA4LOT64Pz4fwQsjh9Sq9BJK92u4gfBLl7 LSBTf4qQULilP7TKtTFCichJSP6cgbE/qRoVD76GNp/fxn8iGXcKCR6yhRwfo8ZRNEuyyOqZB4baN m7o3bvSA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qtSP0-00HME4-15; Thu, 19 Oct 2023 12:45:26 +0000 Received: from szxga08-in.huawei.com ([45.249.212.255]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qtSOw-00HMCk-1g for linux-arm-kernel@lists.infradead.org; Thu, 19 Oct 2023 12:45:24 +0000 Received: from dggpeml500002.china.huawei.com (unknown [172.30.72.54]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4SB6mq4Xs6z15NVP; Thu, 19 Oct 2023 20:42:19 +0800 (CST) Received: from [10.67.120.218] (10.67.120.218) by dggpeml500002.china.huawei.com (7.185.36.158) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Thu, 19 Oct 2023 20:45:03 +0800 Subject: Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb To: Yicong Yang References: <20231012094706.21565-1-hejunhao3@huawei.com> <20231012094706.21565-2-hejunhao3@huawei.com> CC: , , , , , , , , From: hejunhao Message-ID: Date: Thu, 19 Oct 2023 20:45:03 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.67.120.218] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggpeml500002.china.huawei.com (7.185.36.158) X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231019_054523_007724_50F6B0A4 X-CRM114-Status: GOOD ( 21.55 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > --- > 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C8E78CDB465 for ; Thu, 19 Oct 2023 12:45:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345665AbjJSMpM (ORCPT ); Thu, 19 Oct 2023 08:45:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47848 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345442AbjJSMpK (ORCPT ); Thu, 19 Oct 2023 08:45:10 -0400 Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 046AFA4 for ; Thu, 19 Oct 2023 05:45:07 -0700 (PDT) Received: from dggpeml500002.china.huawei.com (unknown [172.30.72.54]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4SB6mq4Xs6z15NVP; Thu, 19 Oct 2023 20:42:19 +0800 (CST) Received: from [10.67.120.218] (10.67.120.218) by dggpeml500002.china.huawei.com (7.185.36.158) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Thu, 19 Oct 2023 20:45:03 +0800 Subject: Re: [PATCH 1/3] coresight: ultrasoc-smb: fix sleep while close preempt in enable_smb To: Yicong Yang References: <20231012094706.21565-1-hejunhao3@huawei.com> <20231012094706.21565-2-hejunhao3@huawei.com> CC: , , , , , , , , From: hejunhao Message-ID: Date: Thu, 19 Oct 2023 20:45:03 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.120.218] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggpeml500002.china.huawei.com (7.185.36.158) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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.