Message ID | 1699341365-47737-4-git-send-email-quic_qianyu@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:aa0b:0:b0:403:3b70:6f57 with SMTP id k11csp65126vqo; Mon, 6 Nov 2023 23:17:14 -0800 (PST) X-Google-Smtp-Source: AGHT+IHYqgZ6xphci5j4gBZWxEfydsJ0TUNNZ83s9Mj7qneK+mLMKPwcRxTrlp0iYvPrAU9S7ABh X-Received: by 2002:a17:90b:100b:b0:27c:f48e:e245 with SMTP id gm11-20020a17090b100b00b0027cf48ee245mr27553425pjb.24.1699341434400; Mon, 06 Nov 2023 23:17:14 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1699341434; cv=none; d=google.com; s=arc-20160816; b=cRUHJ5ONNTNMyRlUrEz7CG2Q46DLx5bL/OA78FPHD42V5wuLQo7iGZ1qjkBvme13Cs kx/CJnQxprc/J5o17P6PnmaTpC+OWC/u8QR4oEb0SIgphRa6W9CXJ7PjvNZ6FYSil6co VjsO1B8In7dJVj6SdOuIsHr2N1RhIyL2D40aUcmGHS+uWZJ9YsfNcUHBiWlfbaD1YTAk Y3UJeQE+bXLV9yeBZVMdosXoRcwQjljjPWZOTdNlO6CC0fu1/ZPSEQPj5jwx/o5yWQEt wMpA3HrIUO0ci7aKbQQ7MwwxAv4qTC9rbakcATSFrNqqwJq2X8gq9KisMNHF/y//baga LCug== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:references:in-reply-to:message-id:date:subject :cc:to:from:dkim-signature; bh=0wF1vmqvNntDEJT9NQ3UDGYk2u4j/IoHwLjboLwGHNk=; fh=f4KQnOf4DL3FZ4rINpxCCB681RH9plJXTbAzdkN1cKE=; b=bqzs87/lWstEtR7Dt8kiuitvE1Z7k2Ms9r+t7BfUx+wXwS/9PKDxMSSdg/t/gp/enj M2LfJfeedcoMQSu9M30gduH1UjoNtf/smTi7rDEf6UUnKzY41zxbyyQGtKJyPU9KVaIQ QKg77tnrhkRHB9GgjH+CaknCL6zP8MYIIf3SjGF7z1ZSHg64Pjq7x7338G+lCQm0CcVU 5eOdyEHXrL82yZ4mEMjSgKc1mL0X1v0adQ4TFw21S/G5cnhdIWlXweSdfSiRKwoP6ZsK bonwg0pR7paodLZ9D0c4+XV7phxsGpQQ7I8bcJZgVbvN2/HQniDFRffx/g9IrV4IN+Fj N6og== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b="eRgU/lHp"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from lipwig.vger.email (lipwig.vger.email. [2620:137:e000::3:3]) by mx.google.com with ESMTPS id gq22-20020a17090b105600b0026b502223cbsi9763630pjb.10.2023.11.06.23.17.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Nov 2023 23:17:14 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b="eRgU/lHp"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id 1C34A807BECB; Mon, 6 Nov 2023 23:17:11 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233559AbjKGHQn (ORCPT <rfc822;heyuhang3455@gmail.com> + 33 others); Tue, 7 Nov 2023 02:16:43 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35282 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233029AbjKGHQm (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 7 Nov 2023 02:16:42 -0500 Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D0519FC; Mon, 6 Nov 2023 23:16:39 -0800 (PST) Received: from pps.filterd (m0279873.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3A77DkHB015693; Tue, 7 Nov 2023 07:16:27 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=from : to : cc : subject : date : message-id : in-reply-to : references; s=qcppdkim1; bh=0wF1vmqvNntDEJT9NQ3UDGYk2u4j/IoHwLjboLwGHNk=; b=eRgU/lHpRtkppnsGMhNRGmAjCWOLVzxEDA4lYCzO46xJjJOfW5bvmahownUtHZkUkDPh TXP0fIWJdNh06sO7mPyWMabmQbQWHpZeYRtSZkwucB2thq4ny3nTt5aI6GMZ33Jr7yYb rR2H/oAPuK4nAu0GPT9+BIpJEu7N6vnFC5G07gW6AhWXx7ee+qnPSGyBbTrgygYrGqxo S9bAqRFl8OqlNRzuXDZqWLxuelsdO311y2sFj6ZHn7pDXUhkIcdJ5QnWCI4uqnBkGADa VpfBt85wxrkd6alRpxJ3wKvF8sDs0OfFq4s0luak/+DkiGbUqdlARUxB/D6zenitJ5em /A== Received: from aptaippmta01.qualcomm.com (tpe-colo-wan-fw-bordernet.qualcomm.com [103.229.16.4]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3u72avswwj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 07 Nov 2023 07:16:27 +0000 Received: from pps.filterd (APTAIPPMTA01.qualcomm.com [127.0.0.1]) by APTAIPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTP id 3A77GAvN028711; Tue, 7 Nov 2023 07:16:10 GMT Received: from pps.reinject (localhost [127.0.0.1]) by APTAIPPMTA01.qualcomm.com (PPS) with ESMTP id 3u5f1m663m-1; Tue, 07 Nov 2023 07:16:10 +0000 Received: from APTAIPPMTA01.qualcomm.com (APTAIPPMTA01.qualcomm.com [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 3A77G9E4028706; Tue, 7 Nov 2023 07:16:10 GMT Received: from cbsp-sh-gv.qualcomm.com (CBSP-SH-gv.ap.qualcomm.com [10.231.249.68]) by APTAIPPMTA01.qualcomm.com (PPS) with ESMTP id 3A77G9D5028705; Tue, 07 Nov 2023 07:16:09 +0000 Received: by cbsp-sh-gv.qualcomm.com (Postfix, from userid 4098150) id 1A8705441; Tue, 7 Nov 2023 15:16:09 +0800 (CST) From: Qiang Yu <quic_qianyu@quicinc.com> To: mani@kernel.org, quic_jhugo@quicinc.com Cc: mhi@lists.linux.dev, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, quic_cang@quicinc.com, quic_mrana@quicinc.com, Qiang Yu <quic_qianyu@quicinc.com> Subject: [PATCH v3 3/4] bus: mhi: host: Avoid processing buffer and event of a disable channel Date: Tue, 7 Nov 2023 15:16:04 +0800 Message-Id: <1699341365-47737-4-git-send-email-quic_qianyu@quicinc.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1699341365-47737-1-git-send-email-quic_qianyu@quicinc.com> References: <1699341365-47737-1-git-send-email-quic_qianyu@quicinc.com> X-QCInternal: smtphost X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-ORIG-GUID: NGTZLya88HmvASpMEyG_Kfp3twAkXhHn X-Proofpoint-GUID: NGTZLya88HmvASpMEyG_Kfp3twAkXhHn X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-11-06_15,2023-11-02_03,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 malwarescore=0 mlxlogscore=708 impostorscore=0 phishscore=0 bulkscore=0 adultscore=0 priorityscore=1501 clxscore=1015 spamscore=0 suspectscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310240000 definitions=main-2311070059 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Mon, 06 Nov 2023 23:17:11 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781888644045607867 X-GMAIL-MSGID: 1781888644045607867 |
Series |
bus: mhi: host: Add lock to avoid race when ringing channel DB
|
|
Commit Message
Qiang Yu
Nov. 7, 2023, 7:16 a.m. UTC
Ckeck mhi channel state after getting chan->lock to ensure that we only
queue buffer to an enabled channel and process event of an enabled channel.
Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
---
drivers/bus/mhi/host/main.c | 4 ++++
1 file changed, 4 insertions(+)
Comments
Hi Qiang, kernel test robot noticed the following build warnings: https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Qiang-Yu/bus-mhi-host-Add-spinlock-to-protect-WP-access-when-queueing-TREs/20231107-151918 base: https://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git mhi-next patch link: https://lore.kernel.org/r/1699341365-47737-4-git-send-email-quic_qianyu%40quicinc.com patch subject: [PATCH v3 3/4] bus: mhi: host: Avoid processing buffer and event of a disable channel config: x86_64-randconfig-161-20231107 (https://download.01.org/0day-ci/archive/20231108/202311080155.MiyREgoQ-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce: (https://download.01.org/0day-ci/archive/20231108/202311080155.MiyREgoQ-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> | Closes: https://lore.kernel.org/r/202311080155.MiyREgoQ-lkp@intel.com/ smatch warnings: drivers/bus/mhi/host/main.c:1257 mhi_gen_tre() warn: inconsistent returns '&mhi_chan->lock'. vim +1257 drivers/bus/mhi/host/main.c 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1204 int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan, cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1205 struct mhi_buf_info *info, enum mhi_flags flags) 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1206 { 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1207 struct mhi_ring *buf_ring, *tre_ring; 84f5f31f110e5e drivers/bus/mhi/host/main.c Manivannan Sadhasivam 2022-03-01 1208 struct mhi_ring_element *mhi_tre; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1209 struct mhi_buf_info *buf_info; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1210 int eot, eob, chain, bei; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1211 int ret; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1212 da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1213 /* Protect accesses for reading and incrementing WP */ da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1214 write_lock_bh(&mhi_chan->lock); 6025cde6ecb13c drivers/bus/mhi/host/main.c Qiang Yu 2023-11-07 1215 if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) 6025cde6ecb13c drivers/bus/mhi/host/main.c Qiang Yu 2023-11-07 1216 return -EINVAL; This looks like two people submitted patches on the same day? We should unlock before returning? da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1217 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1218 buf_ring = &mhi_chan->buf_ring; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1219 tre_ring = &mhi_chan->tre_ring; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1220 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1221 buf_info = buf_ring->wp; cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1222 WARN_ON(buf_info->used); cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1223 buf_info->pre_mapped = info->pre_mapped; cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1224 if (info->pre_mapped) cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1225 buf_info->p_addr = info->p_addr; cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1226 else cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1227 buf_info->v_addr = info->v_addr; cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1228 buf_info->cb_buf = info->cb_buf; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1229 buf_info->wp = tre_ring->wp; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1230 buf_info->dir = mhi_chan->dir; cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1231 buf_info->len = info->len; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1232 cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1233 if (!info->pre_mapped) { 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1234 ret = mhi_cntrl->map_single(mhi_cntrl, buf_info); da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1235 if (ret) { da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1236 write_unlock_bh(&mhi_chan->lock); 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1237 return ret; cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1238 } da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1239 } 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1240 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1241 eob = !!(flags & MHI_EOB); 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1242 eot = !!(flags & MHI_EOT); 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1243 chain = !!(flags & MHI_CHAIN); 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1244 bei = !!(mhi_chan->intmod); 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1245 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1246 mhi_tre = tre_ring->wp; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1247 mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr); cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1248 mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(info->len); 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1249 mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, eot, eob, chain); 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1250 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1251 /* increment WP */ 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1252 mhi_add_ring_element(mhi_cntrl, tre_ring); 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1253 mhi_add_ring_element(mhi_cntrl, buf_ring); 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1254 da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1255 write_unlock_bh(&mhi_chan->lock); da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1256 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 @1257 return 0; 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1258 }
On Tue, Nov 07, 2023 at 03:16:04PM +0800, Qiang Yu wrote: > Ckeck mhi channel state after getting chan->lock to ensure that we only > queue buffer to an enabled channel and process event of an enabled channel. > This commit message doesn't give proper explanation on how the channel can go to disabled state in between parse_xfer_event() and mhi_gen_tre(). - Mani > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> > --- > drivers/bus/mhi/host/main.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c > index a236dc2..b137d54 100644 > --- a/drivers/bus/mhi/host/main.c > +++ b/drivers/bus/mhi/host/main.c > @@ -672,6 +672,8 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl, > } > > read_lock_bh(&mhi_chan->lock); > + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) > + goto end_process_tx_event; > } > break; > } /* CC_EOT */ > @@ -1211,6 +1213,8 @@ int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan, > > /* Protect accesses for reading and incrementing WP */ > write_lock_bh(&mhi_chan->lock); > + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) > + return -EINVAL; > > buf_ring = &mhi_chan->buf_ring; > tre_ring = &mhi_chan->tre_ring; > -- > 2.7.4 > >
On 11/10/2023 12:32 AM, Manivannan Sadhasivam wrote: > On Tue, Nov 07, 2023 at 03:16:04PM +0800, Qiang Yu wrote: >> Ckeck mhi channel state after getting chan->lock to ensure that we only >> queue buffer to an enabled channel and process event of an enabled channel. >> > This commit message doesn't give proper explanation on how the channel can go to > disabled state in between parse_xfer_event() and mhi_gen_tre(). > > - Mani Hi Mani. How about following commit message MHI channel state is protected by mhi_chan->lock. Hence, after core drops mhi_chan->lock during processing xfer event, it can not prevent channel state being changed if client closes channel or driver is removed at this time. So let's check mhi channel state after getting chan->lock again to avoid queuing buffer to a disabled channel in xfer callback and stop processing event of the disabled channel. >> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> >> --- >> drivers/bus/mhi/host/main.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c >> index a236dc2..b137d54 100644 >> --- a/drivers/bus/mhi/host/main.c >> +++ b/drivers/bus/mhi/host/main.c >> @@ -672,6 +672,8 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl, >> } >> >> read_lock_bh(&mhi_chan->lock); >> + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) >> + goto end_process_tx_event; >> } >> break; >> } /* CC_EOT */ >> @@ -1211,6 +1213,8 @@ int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan, >> >> /* Protect accesses for reading and incrementing WP */ >> write_lock_bh(&mhi_chan->lock); >> + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) >> + return -EINVAL; >> >> buf_ring = &mhi_chan->buf_ring; >> tre_ring = &mhi_chan->tre_ring; >> -- >> 2.7.4 >> >>
On 11/8/2023 2:48 PM, Dan Carpenter wrote: > Hi Qiang, > > kernel test robot noticed the following build warnings: > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Qiang-Yu/bus-mhi-host-Add-spinlock-to-protect-WP-access-when-queueing-TREs/20231107-151918 > base: https://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git mhi-next > patch link: https://lore.kernel.org/r/1699341365-47737-4-git-send-email-quic_qianyu%40quicinc.com > patch subject: [PATCH v3 3/4] bus: mhi: host: Avoid processing buffer and event of a disable channel > config: x86_64-randconfig-161-20231107 (https://download.01.org/0day-ci/archive/20231108/202311080155.MiyREgoQ-lkp@intel.com/config) > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 > reproduce: (https://download.01.org/0day-ci/archive/20231108/202311080155.MiyREgoQ-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > | Closes: https://lore.kernel.org/r/202311080155.MiyREgoQ-lkp@intel.com/ > > smatch warnings: > drivers/bus/mhi/host/main.c:1257 mhi_gen_tre() warn: inconsistent returns '&mhi_chan->lock'. > > vim +1257 drivers/bus/mhi/host/main.c > > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1204 int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan, > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1205 struct mhi_buf_info *info, enum mhi_flags flags) > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1206 { > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1207 struct mhi_ring *buf_ring, *tre_ring; > 84f5f31f110e5e drivers/bus/mhi/host/main.c Manivannan Sadhasivam 2022-03-01 1208 struct mhi_ring_element *mhi_tre; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1209 struct mhi_buf_info *buf_info; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1210 int eot, eob, chain, bei; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1211 int ret; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1212 > da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1213 /* Protect accesses for reading and incrementing WP */ > da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1214 write_lock_bh(&mhi_chan->lock); > 6025cde6ecb13c drivers/bus/mhi/host/main.c Qiang Yu 2023-11-07 1215 if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) > 6025cde6ecb13c drivers/bus/mhi/host/main.c Qiang Yu 2023-11-07 1216 return -EINVAL; > > This looks like two people submitted patches on the same day? > We should unlock before returning? Yes, we should unlock before return, will change in next version patch. Thank you. > > da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1217 > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1218 buf_ring = &mhi_chan->buf_ring; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1219 tre_ring = &mhi_chan->tre_ring; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1220 > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1221 buf_info = buf_ring->wp; > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1222 WARN_ON(buf_info->used); > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1223 buf_info->pre_mapped = info->pre_mapped; > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1224 if (info->pre_mapped) > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1225 buf_info->p_addr = info->p_addr; > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1226 else > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1227 buf_info->v_addr = info->v_addr; > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1228 buf_info->cb_buf = info->cb_buf; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1229 buf_info->wp = tre_ring->wp; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1230 buf_info->dir = mhi_chan->dir; > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1231 buf_info->len = info->len; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1232 > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1233 if (!info->pre_mapped) { > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1234 ret = mhi_cntrl->map_single(mhi_cntrl, buf_info); > da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1235 if (ret) { > da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1236 write_unlock_bh(&mhi_chan->lock); > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1237 return ret; > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1238 } > da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1239 } > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1240 > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1241 eob = !!(flags & MHI_EOB); > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1242 eot = !!(flags & MHI_EOT); > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1243 chain = !!(flags & MHI_CHAIN); > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1244 bei = !!(mhi_chan->intmod); > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1245 > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1246 mhi_tre = tre_ring->wp; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1247 mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr); > cd116318803f5e drivers/bus/mhi/core/main.c Hemant Kumar 2020-05-21 1248 mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(info->len); > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1249 mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, eot, eob, chain); > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1250 > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1251 /* increment WP */ > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1252 mhi_add_ring_element(mhi_cntrl, tre_ring); > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1253 mhi_add_ring_element(mhi_cntrl, buf_ring); > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1254 > da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1255 write_unlock_bh(&mhi_chan->lock); > da05b5d43f483c drivers/bus/mhi/host/main.c Bhaumik Bhatt 2023-11-07 1256 > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 @1257 return 0; > 189ff97cca53e3 drivers/bus/mhi/core/main.c Manivannan Sadhasivam 2020-02-20 1258 } >
On 11/10/2023 11:38 AM, Qiang Yu wrote: > > On 11/10/2023 12:32 AM, Manivannan Sadhasivam wrote: >> On Tue, Nov 07, 2023 at 03:16:04PM +0800, Qiang Yu wrote: >>> Ckeck mhi channel state after getting chan->lock to ensure that we only >>> queue buffer to an enabled channel and process event of an enabled >>> channel. >>> >> This commit message doesn't give proper explanation on how the >> channel can go to >> disabled state in between parse_xfer_event() and mhi_gen_tre(). >> >> - Mani > > Hi Mani. How about following commit message > > MHI channel state is protected by mhi_chan->lock. Hence, after core drops > mhi_chan->lock during processing xfer event, it can not prevent channel > state being changed if client closes channel or driver is removed at this > time. So let's check mhi channel state after getting chan->lock again > to avoid > queuing buffer to a disabled channel in xfer callback and stop processing > event of the disabled channel. Hi Mani, Have updated this commit message in V4 patch, hope get your comments. > >>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> >>> --- >>> drivers/bus/mhi/host/main.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c >>> index a236dc2..b137d54 100644 >>> --- a/drivers/bus/mhi/host/main.c >>> +++ b/drivers/bus/mhi/host/main.c >>> @@ -672,6 +672,8 @@ static int parse_xfer_event(struct >>> mhi_controller *mhi_cntrl, >>> } >>> read_lock_bh(&mhi_chan->lock); >>> + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) >>> + goto end_process_tx_event; >>> } >>> break; >>> } /* CC_EOT */ >>> @@ -1211,6 +1213,8 @@ int mhi_gen_tre(struct mhi_controller >>> *mhi_cntrl, struct mhi_chan *mhi_chan, >>> /* Protect accesses for reading and incrementing WP */ >>> write_lock_bh(&mhi_chan->lock); >>> + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) >>> + return -EINVAL; >>> buf_ring = &mhi_chan->buf_ring; >>> tre_ring = &mhi_chan->tre_ring; >>> -- >>> 2.7.4 >>> >>> >
On 11/10/2023 11:38 AM, Qiang Yu wrote: > > On 11/10/2023 12:32 AM, Manivannan Sadhasivam wrote: >> On Tue, Nov 07, 2023 at 03:16:04PM +0800, Qiang Yu wrote: >>> Ckeck mhi channel state after getting chan->lock to ensure that we only >>> queue buffer to an enabled channel and process event of an enabled >>> channel. >>> >> This commit message doesn't give proper explanation on how the >> channel can go to >> disabled state in between parse_xfer_event() and mhi_gen_tre(). >> >> - Mani > > Hi Mani. How about following commit message > > MHI channel state is protected by mhi_chan->lock. Hence, after core drops > mhi_chan->lock during processing xfer event, it can not prevent channel > state being changed if client closes channel or driver is removed at this > time. So let's check mhi channel state after getting chan->lock again > to avoid > queuing buffer to a disabled channel in xfer callback and stop processing > event of the disabled channel. Hi Mani, Have updated this commit message in V4 patch, hope get your comments. > >>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> >>> --- >>> drivers/bus/mhi/host/main.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c >>> index a236dc2..b137d54 100644 >>> --- a/drivers/bus/mhi/host/main.c >>> +++ b/drivers/bus/mhi/host/main.c >>> @@ -672,6 +672,8 @@ static int parse_xfer_event(struct >>> mhi_controller *mhi_cntrl, >>> } >>> read_lock_bh(&mhi_chan->lock); >>> + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) >>> + goto end_process_tx_event; >>> } >>> break; >>> } /* CC_EOT */ >>> @@ -1211,6 +1213,8 @@ int mhi_gen_tre(struct mhi_controller >>> *mhi_cntrl, struct mhi_chan *mhi_chan, >>> /* Protect accesses for reading and incrementing WP */ >>> write_lock_bh(&mhi_chan->lock); >>> + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) >>> + return -EINVAL; >>> buf_ring = &mhi_chan->buf_ring; >>> tre_ring = &mhi_chan->tre_ring; >>> -- >>> 2.7.4 >>> >>> >
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c index a236dc2..b137d54 100644 --- a/drivers/bus/mhi/host/main.c +++ b/drivers/bus/mhi/host/main.c @@ -672,6 +672,8 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl, } read_lock_bh(&mhi_chan->lock); + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) + goto end_process_tx_event; } break; } /* CC_EOT */ @@ -1211,6 +1213,8 @@ int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan, /* Protect accesses for reading and incrementing WP */ write_lock_bh(&mhi_chan->lock); + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) + return -EINVAL; buf_ring = &mhi_chan->buf_ring; tre_ring = &mhi_chan->tre_ring;