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 73AF2D0EE24 for ; Fri, 11 Oct 2024 19:16:49 +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: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=LjFrsfEdmIwzQUjak3mQDZtLMhU12hmQ+DxJ5h/HQ10=; b=azzDd7sULMsVk8z59QeV4oA3EE XtnHoQUQ/DHFA6GbvP1U3SCSy7ABWa3JQ9YDGwUyV5iVyy3xiZY99b+XsVsXOMo5f4ZfY4J/sC+jl W30F7rgnjmcYsAgJeq6Ui3S+ua5qTo+98tiF/76HynloW70QHvLyWWtz9x0TUfUY9gxWWwNzvYm41 /g/YtGCxTbQjDJQs33d2TJp4JNXpIkdoKXRmnLrLxyMudqH1+mJnE524Rp6v2jtvuc4eTC+nL6FSd 4S5ADpLV157iE6kSxVfTK4/SyL3SzdinKc91TNWkTyjjrAq2138r+6uSyMgZa0/xxGXv98MS/ConM iRWzpdcg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1szL7u-0000000HOXk-3hRv; Fri, 11 Oct 2024 19:16:38 +0000 Received: from mail-qt1-x82b.google.com ([2607:f8b0:4864:20::82b]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1szL6X-0000000HOQ1-0CJx for linux-arm-kernel@lists.infradead.org; Fri, 11 Oct 2024 19:15:15 +0000 Received: by mail-qt1-x82b.google.com with SMTP id d75a77b69052e-45f091bf433so18805901cf.3 for ; Fri, 11 Oct 2024 12:15:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; t=1728674110; x=1729278910; darn=lists.infradead.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=LjFrsfEdmIwzQUjak3mQDZtLMhU12hmQ+DxJ5h/HQ10=; b=G8U51aYgfIVJOdMiAngTHXk9HqQ53QZ2dn7nYhz4f9mvzz5eJYg1OLpr06WpKU6FEL x/K+dfSqDGOxgh64KH9M+Vl3yxGzfpjhC5NgCZGc1fe0f/gw8ZfRa9jyI0LNn3sikRHH cMwMvNYdQTLNkiT2soBEgBkVIfWGGxWZgT3AY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728674110; x=1729278910; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=LjFrsfEdmIwzQUjak3mQDZtLMhU12hmQ+DxJ5h/HQ10=; b=AlZyTuybRRs0AM6hUlkBVAN+neoquCFUKeWkOxIXqqus53dUN1//Nq/1so3J8w13pi iBfpZOyXzADgSy9hLRZIqIWlcfaBWNh3cdEJTiWLhJjz/yHoiSG5g7rqZ8nAIMz0ipNq mPBk+yMSIxtKBeifd1QZGqq8QpPQdQikL4p7qh0FWi6abSNGHqs77N4bbBhktNTT/WXx ipHRxKzIOgHWAeL/wbFj8y+yAP5naDT7nwb7I/U0dT6pYN+j/4xczaSBLACtGJzaq7kN 0M8CMemX0ZFdtnqxp/N89y9uxMoDNMC6bJE/ddkNw//fLiyW94OWgvLzYRMRcQMhxa/L 3Zhg== X-Forwarded-Encrypted: i=1; AJvYcCWuxE0Tcie3zusjulQ8izthJt7WzB+Jn0hGV0/2lIh4b/W/pPKUCGoIRo+ejvz+cRZbEhWcvLR2rF1mLKnpSG2l@lists.infradead.org X-Gm-Message-State: AOJu0Yy00jHkBxt9qO+OCVamwaQscMzH4c+5MYwHjQ27fEyAIVclqBtr QxzpgUnyAQk8b5LJqipJLoFAQVfyfFFYgTO0gct2w+vBJ8GbRh46055mjStCaQ== X-Google-Smtp-Source: AGHT+IHaD9B2lB3GvcRk/gnB22OjcUbBKHprjtQdmpjL9vfXeqWSM/4Z1IGg8qCLe70ABMu1NSQBNQ== X-Received: by 2002:a05:6214:328b:b0:6cb:e9eb:2f51 with SMTP id 6a1803df08f44-6cbefef4ca7mr42219966d6.0.1728674109633; Fri, 11 Oct 2024 12:15:09 -0700 (PDT) Received: from [10.69.69.40] ([192.19.223.252]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-6cbe8608d7esm17869196d6.100.2024.10.11.12.15.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 11 Oct 2024 12:15:09 -0700 (PDT) Message-ID: <50f4d2d6-dead-4053-834f-134d2df0d6bd@broadcom.com> Date: Fri, 11 Oct 2024 12:15:07 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] firmware: arm_scmi: Queue in scmi layer for mailbox implementation To: Cristian Marussi Cc: arm-scmi@vger.kernel.org, sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org, peng.fan@nxp.com, bcm-kernel-feedback-list@broadcom.com, florian.fainelli@broadcom.com References: <20241009192637.1090238-1-justin.chen@broadcom.com> Content-Language: en-US From: Justin Chen In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241011_121513_247029_E1F06DD3 X-CRM114-Status: GOOD ( 33.98 ) 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 On 10/11/24 6:43 AM, Cristian Marussi wrote: > On Wed, Oct 09, 2024 at 12:26:37PM -0700, Justin Chen wrote: >> 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: b53515fa177c ("firmware: arm_scmi: Make MBOX transport a standalone driver") >> Signed-off-by: Justin Chen >> --- >> >> Changes in v2: > > Hi Justin, > > thanks. > > A few nitpicks and one remark down below. > >> >> - Added Fixes tag >> - Improved commit message to better capture the issue >> >> .../firmware/arm_scmi/transports/mailbox.c | 21 +++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c >> index 1a754dee24f7..30bc2865582f 100644 >> --- a/drivers/firmware/arm_scmi/transports/mailbox.c >> +++ b/drivers/firmware/arm_scmi/transports/mailbox.c >> @@ -33,6 +33,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; > > Missing Doxygen comment.... > > arm_scmi/transports/mailbox.c:39: warning: Function parameter or struct member 'chan_lock' not described in 'scmi_mailbox > >> }; >> >> #define client_to_scmi_mailbox(c) container_of(c, struct scmi_mailbox, cl) >> @@ -205,6 +206,7 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, >> cl->rx_callback = rx_callback; >> cl->tx_block = false; >> cl->knows_txdone = tx; >> + mutex_init(&smbox->chan_lock); > > This could be move at the end of this function after the channels are > requested and it is no more possible to fail and bail out....messages > wont flow and lock wont be used anyway until this chan_setup completes... > ...BUT I have NOT string opinion about this....you can leave it here > too...up to you >> >> smbox->chan = mbox_request_channel(cl, tx ? 0 : p2a_chan); >> if (IS_ERR(smbox->chan)) { >> @@ -267,11 +269,21 @@ static int mailbox_send_message(struct scmi_chan_info *cinfo, >> struct scmi_mailbox *smbox = cinfo->transport_info; >> int ret; >> >> + /* >> + * The mailbox layer has it's own queue. However the mailbox queue confuses > its own queue > >> + * 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; >> + else >> + mutex_unlock(&smbox->chan_lock); > > I think this should be > > else if (ret < 0) > mutex_unlock(&smbox->chan_lock); > > ...since looking at mbox_send_message() and its implementation it returns > NON-Negative integers on Success...so 0 from mbox_send_mmessage() also means > SUCCESS and we should not release the mutex (I think the 'ret' returned > here is the idx from add_to_rbuf...so it will become zero peridiocally > on normal successfull operation) > Yes, I see the implementation. Looks like it returns the position in the ring buffer. I also confirmed with CONFIG_DEBUG_MUTEXES which triggers a warning. What about this? if (ret >= 0) ret = 0 else mutex_unlock(&smbox->chan_lock); A bit easier to read IMO. Thanks, Justin >> >> return ret; >> } >> @@ -281,13 +293,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, >> -- >> 2.34.1 >> > > I gave it a go on a couple of JUNO, without any issues. > > Other than the above, LGTM. > > Reviewed-by: Cristian Marussi > Tested-by: Cristian Marussi > > Thanks, > Cristian >