From patchwork Mon Oct 14 16:07:17 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Chen X-Patchwork-Id: 13835305 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 EBD53D1812C for ; Mon, 14 Oct 2024 16:10:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: MIME-Version:Message-Id:Date:Subject:Cc:To:From:Reply-To:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References:List-Owner; bh=olM901OjHI4fUJb9hALlmkL2W414ca2nuDxeB9DT6Go=; b=sss2JXhZ8moI/m8Szfy/Ae+u2I udXtZettOSLbls/JtbtKeaGzTjYgNzo7/mgS5xfmqP6Qa8JctH+4RJoLzVJrTnDlzlzpksOwYC4v0 VcFRqvvWSLAhdrceu+VNaYX8ZZ6qQuftEyNxm4duah0s6HPox5zGB4sy4qe01gpuoB3UBZlX8dw72 fqbjwhGdLP0WXbLCgiNSKsp6RJnuYWYuX0D2gYHZZWF9fs9BDviLmUfogu5BnbBYz+aD6HI1huVH/ RA4Y3weRLs+zhQROoS/bEwYtoM9S8zV5agUSnXOKRVFk3xRzoC0JUFhK2EJ2AYeVTHjp0mZKuqa+m bCMjE3pA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t0Ndz-00000005oPr-2KRB; Mon, 14 Oct 2024 16:10:03 +0000 Received: from mail-pl1-x62a.google.com ([2607:f8b0:4864:20::62a]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1t0NbS-00000005nx8-2XPu for linux-arm-kernel@lists.infradead.org; Mon, 14 Oct 2024 16:07:28 +0000 Received: by mail-pl1-x62a.google.com with SMTP id d9443c01a7336-20ceb8bd22fso8782875ad.3 for ; Mon, 14 Oct 2024 09:07:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; t=1728922044; x=1729526844; darn=lists.infradead.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=olM901OjHI4fUJb9hALlmkL2W414ca2nuDxeB9DT6Go=; b=SBSi6FNpH9+Ge4hPhJslyqRz6QlRq4fz06lOI28m26inSMOWsPQIdGQqTg5Vq7w5cV TyMl2GUGCVkvMrDS/T+oux2yhkzlm8GMYgG5hxhiWVVpIIVmyjZIBySpn71XV8x2Afv5 EXwzUTC7xK7bhpzMktHx5nH26XKzX8A7dfiwo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728922044; x=1729526844; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=olM901OjHI4fUJb9hALlmkL2W414ca2nuDxeB9DT6Go=; b=FuopXjwDmxqpXdiibz4NBBwOcmNK6Samx96MyScUrcsl7v1CZpn9j/Bn5UP9UAkix8 cJOYHjPy18WvbEo8JLVicXD2/wd5CiPzZH9wDA4ViB2FO7Yk8bzX6c+CkARIldCT215J JiUJvo9v111BmtFNjCwmLgiIC/cZE0fPXguZLy2EoB3RztYh1A+CQNy4JqoOoU0Qvw6z ehU68nCdfJGqtRkvUXI501oXjeQIp3bQHdzjfMUAVj4iOdWJMKgeZIqvH75U6zDH5kHR UISNzifxq1GBNv51VyDR9b/yMehD9jhiiyiATcUJ1HdyIo9DrYT2la8k3JoFyTCMQZ1/ p8OQ== X-Gm-Message-State: AOJu0YxeKHDutrOzeZCdS0oxkdAusfVlafyGlXqPfcWlllpKPTsJCCwT hmymD6RCF9cHQEXnIyRd10o0gw+7kZlFWcsTNlAANbUhyHRaAzt/GCm2imTm1Q4ZdZ7XCT+cCG/ b118p3bS1r2c+jlz/XVyTKD9T3H1kS8inL6YEW3tzGo3cG+24GyLVj4TbHdwkxCfDdSSLHUDXwq ChoZ9eW3zSx2y26CrD6eyVr92EUhIjjzuWfJdLqlT5/6Vccq5gabrq4klc6GXb X-Google-Smtp-Source: AGHT+IFzEMW7le/fIi2Ii4Dl68d7B+eQog99gAtjbcmkwo59kxx4eTsXsCtWs+7AzRKO6b9vkAIxcg== X-Received: by 2002:a17:902:f550:b0:20c:a175:1942 with SMTP id d9443c01a7336-20cbb1aa3a5mr108055365ad.24.1728922044239; Mon, 14 Oct 2024 09:07:24 -0700 (PDT) Received: from stbirv-lnx-1.igp.broadcom.net ([192.19.223.252]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-20c8c212d19sm67854515ad.191.2024.10.14.09.07.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 14 Oct 2024 09:07:23 -0700 (PDT) From: Justin Chen To: linux-arm-kernel@lists.infradead.org Cc: viresh.kumar@linaro.org, peng.fan@nxp.com, cristian.marussi@arm.com, sudeep.holla@arm.com, florian.fainelli@broadcom.com, bcm-kernel-feedback-list@broadcom.com, Justin Chen Subject: [PATCH v3] firmware: arm_scmi: Queue in scmi layer for mailbox implementation Date: Mon, 14 Oct 2024 09:07:17 -0700 Message-Id: <20241014160717.1678953-1-justin.chen@broadcom.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241014_090726_678939_538F3935 X-CRM114-Status: GOOD ( 18.84 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org send_message() does not block in the MBOX implementation. This is because the mailbox layer has its own queue. However, this confuses the per xfer timeouts as they all start their timeout ticks in parallel. Consider a case where the xfer timeout is 30ms and a SCMI transaction takes 25ms. 0ms: Message #0 is queued in mailbox layer and sent out, then sits at scmi_wait_for_message_response() with a timeout of 30ms 1ms: Message #1 is queued in mailbox layer but not sent out yet. Since send_message() doesn't block, it also sits at scmi_wait_for_message_response() with a timeout of 30ms ... 25ms: Message #0 is completed, txdone is called and Message #1 is sent out 31ms: Message #1 times out since the count started at 1ms. Even though it has only been inflight for 6ms. Fixes: 5c8a47a5a91d ("firmware: arm_scmi: Make scmi core independent of the transport type") Signed-off-by: Justin Chen Reviewed-by: Cristian Marussi Tested-by: Cristian Marussi --- Changes in v3: - Changed Fixes tag - Fixed mutex imbalance - Add Doxygen comment - Fixed spelling mistake - Moved mutex init Changes in v2 - Added Fixes tag - Improved commit message to better capture the issue .../firmware/arm_scmi/transports/mailbox.c | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c index 1a754dee24f7..af08fb5cc72f 100644 --- a/drivers/firmware/arm_scmi/transports/mailbox.c +++ b/drivers/firmware/arm_scmi/transports/mailbox.c @@ -25,6 +25,7 @@ * @chan_platform_receiver: Optional Platform Receiver mailbox unidirectional channel * @cinfo: SCMI channel info * @shmem: Transmit/Receive shared memory area + * @chan_lock: Lock that prevents multiple xfers from being queued */ struct scmi_mailbox { struct mbox_client cl; @@ -33,6 +34,7 @@ struct scmi_mailbox { struct mbox_chan *chan_platform_receiver; struct scmi_chan_info *cinfo; struct scmi_shared_mem __iomem *shmem; + struct mutex chan_lock; }; #define client_to_scmi_mailbox(c) container_of(c, struct scmi_mailbox, cl) @@ -238,6 +240,7 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, cinfo->transport_info = smbox; smbox->cinfo = cinfo; + mutex_init(&smbox->chan_lock); return 0; } @@ -267,13 +270,23 @@ static int mailbox_send_message(struct scmi_chan_info *cinfo, struct scmi_mailbox *smbox = cinfo->transport_info; int ret; + /* + * The mailbox layer has its own queue. However the mailbox queue confuses + * the per message SCMI timeouts since the clock starts when the message is + * submitted into the mailbox queue. So when multiple messages are queued up + * the clock starts on all messages instead of only the one inflight. + */ + mutex_lock(&smbox->chan_lock); + ret = mbox_send_message(smbox->chan, xfer); /* mbox_send_message returns non-negative value on success, so reset */ - if (ret > 0) - ret = 0; + if (ret < 0) { + mutex_unlock(&smbox->chan_lock); + return ret; + } - return ret; + return 0; } static void mailbox_mark_txdone(struct scmi_chan_info *cinfo, int ret, @@ -281,13 +294,10 @@ static void mailbox_mark_txdone(struct scmi_chan_info *cinfo, int ret, { struct scmi_mailbox *smbox = cinfo->transport_info; - /* - * NOTE: we might prefer not to need the mailbox ticker to manage the - * transfer queueing since the protocol layer queues things by itself. - * Unfortunately, we have to kick the mailbox framework after we have - * received our message. - */ mbox_client_txdone(smbox->chan, ret); + + /* Release channel */ + mutex_unlock(&smbox->chan_lock); } static void mailbox_fetch_response(struct scmi_chan_info *cinfo,