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 9DD03C433EF for ; Thu, 30 Jun 2022 16:20:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235136AbiF3QUx (ORCPT ); Thu, 30 Jun 2022 12:20:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57616 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236179AbiF3QUr (ORCPT ); Thu, 30 Jun 2022 12:20:47 -0400 Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [185.132.182.106]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BAA353C49E; Thu, 30 Jun 2022 09:20:44 -0700 (PDT) Received: from pps.filterd (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 25UDhRqt001696; Thu, 30 Jun 2022 18:20:26 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=8OPsY3NYVtvi/Y0bIeB4MSPnX33YqwmgeqJgxgT3N7c=; b=wHKDT4tzjux6li799C0XwsCWKitWHrEXKuWFA6uVszftSwA5cAdBihnOqZ4o4pw5Oqzr nAXHgCuHDe+u/wLDh6IJhi8aZec1Oomf081y//dIacJ67yzG+QiZd5Ddw3c7+rJUNB0W DQLXMBqdHFHKitirZCO8KM5FHrhMSU/HBonqiHIyf4pt7DOfEpv39c3fWhyG+5ptdQzV JTZJjS3wr38V2/eiLQD9IEMUAz91VdHcxANBBRq1H4jmvwQInJmPYAxxiP/X9VzOOECr CSk4drqswKUNQu417uIynnZPnIjyAC/EY98KreFqwUp3hIcpXyJS+VEe1+Q7hzTb3n2Q Kg== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3h1cy9h7he-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 30 Jun 2022 18:20:26 +0200 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id E6BB110002A; Thu, 30 Jun 2022 18:20:23 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node2.st.com [10.75.129.70]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id AE10A2248DD; Thu, 30 Jun 2022 18:20:23 +0200 (CEST) Received: from [10.252.24.34] (10.75.127.44) by SHFDAG1NODE2.st.com (10.75.129.70) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2308.20; Thu, 30 Jun 2022 18:20:20 +0200 Message-ID: Date: Thu, 30 Jun 2022 18:20:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe() Content-Language: en-US To: Mathieu Poirier , Anup Patel CC: Bjorn Andersson , Atish Patra , Alistair Francis , Anup Patel , , , References: <20220608171334.730739-1-apatel@ventanamicro.com> <20220629174318.GB2018382@p14s> From: Arnaud POULIQUEN In-Reply-To: <20220629174318.GB2018382@p14s> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.75.127.44] X-ClientProxiedBy: SFHDAG2NODE3.st.com (10.75.127.6) To SHFDAG1NODE2.st.com (10.75.129.70) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1 definitions=2022-06-30_11,2022-06-28_01,2022-06-22_01 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 6/29/22 19:43, Mathieu Poirier wrote: > Hi Anup, > > On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote: >> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf() >> fails due to both virtqueues (Rx and Tx) marked as broken by the >> __vring_new_virtqueue() function. To solve this, virtio_device_ready() >> (which unbreaks queues) should be called before virtqueue_add_inbuf(). >> >> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ") >> Signed-off-by: Anup Patel >> --- >> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c >> index 905ac7910c98..71a64d2c7644 100644 >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c >> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev) >> /* and half is dedicated for TX */ >> vrp->sbufs = bufs_va + total_buf_space / 2; >> >> + /* From this point on, we can notify and get callbacks. */ >> + virtio_device_ready(vdev); >> + > > Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can > potentially be called (by way of rpmsg_recv_done()), which will race with > virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then > rpmsg_recv_done() will fail, potentially breaking remote processors' state > machines that don't expect their initial name service to fail when the "device" > has been marked as ready. > > What does make me curious though is that nobody on the remoteproc mailing list > has complained about commit 8b4ec69d7e09 breaking their environment... By now, > i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on > their rig? I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4) I confirm the issue! Concerning the solution, I share Mathieu's concern. This could break legacy. I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence. I this case the patch would be: + /* + * Unbreak the virtqueues to allow to add buffers before setting the vdev status + * to ready + */ + __virtio_unbreak_device(vdev); + /* set up the receive buffers */ for (i = 0; i < vrp->num_bufs / 2; i++) { struct scatterlist sg; void *cpu_addr = vrp->rbufs + i * vrp->buf_size; Regards, Arnaud > > Thanks, > Mathieu > >> /* set up the receive buffers */ >> for (i = 0; i < vrp->num_bufs / 2; i++) { >> struct scatterlist sg; >> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev) >> */ >> notify = virtqueue_kick_prepare(vrp->rvq); >> >> - /* From this point on, we can notify and get callbacks. */ >> - virtio_device_ready(vdev); >> - >> /* tell the remote processor it can start sending messages */ >> /* >> * this might be concurrent with callbacks, but we are only >> -- >> 2.34.1 >>