From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f202.google.com (mail-pf1-f202.google.com [209.85.210.202]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id ED46F37B00B for ; Thu, 12 Mar 2026 10:19:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.202 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773310762; cv=none; b=ptRmEaMGpvHNo+1tdFrjWC2nS08XdJC/EuCgZVTyzu6zzB1KqWdfOSMmvW5I1BbeHf27STkblfu0ZgtfphagG/5bZx17Jx9zwFnMvbHFAHL9HKAJg5Wu7M11qC+XxosLNIG+Wqr+UjmP1hJBNgHoKvUs2raAvlDG2Dev80kLYDk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773310762; c=relaxed/simple; bh=RIJYOu2xGRzWL05vM6t8HJiTX2Lq0OSe8rkmIMH9lV0=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=PW1RHwLPYGww+f9iwNhYVbhooVS3nZBaQNdMZ99wWPRcsyaOASRbonWFuL+h44bTuaTWG8H44LoZpQVjZ54Pmr/faWT7nVHCl8tFO/TgAgQqTUwA5ll032ZRxgRwoMWaQXYNB219bTSpJ6Jxo+UpWAXcUxf/vEtKNTLFljMWuVA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--joonwonkang.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=oIfNuB9K; arc=none smtp.client-ip=209.85.210.202 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--joonwonkang.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="oIfNuB9K" Received: by mail-pf1-f202.google.com with SMTP id d2e1a72fcca58-8299499d582so3232473b3a.2 for ; Thu, 12 Mar 2026 03:19:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1773310760; x=1773915560; darn=vger.kernel.org; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:from:to:cc:subject:date:message-id :reply-to; bh=GX0OxN1xdS7I2NhNh4+gaZcdM+4ej64t/XXZ7tDQThA=; b=oIfNuB9KDA3evvda05pVFxtMUGs5cJgidJ/ZjMz9jCJ1lYoJE/kGT5LJXfROMXp4s2 AmVsB3+c0J1F9Shvk2wa/ZbZLX0QsZMj+qWTgLBsNJcAdg6FV8MX7StK2GDrQclI9pm8 riMR9CRc2pJliyPpmpGn2oDnKVNnjaefE/FylPZa413TzQIY/wrIgtc9nI3XubD9Mg0F Gqj09UzGQ3JYHLqnxgd6Gt/pPkQ0wmhGIBra6u2ORjRwBqzExmjPvpDdlaSobh8mWPaz m7L0LmuaZ6YPfIDm4+KJfJsJ2q9yd5LqkgoWxhGDTSDjAGIOXHY3S27jgg5sAjtFDRul iLAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1773310760; x=1773915560; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=GX0OxN1xdS7I2NhNh4+gaZcdM+4ej64t/XXZ7tDQThA=; b=eZlpOl89v2eiwH6IiS5AizmH3MSyT8I5XLx3K0ecWXZXNjfWCU7iPg4V37cgpqMnrP Kv7s5uKN9K99GqVKSlZht30K7aWDGlZ6+CM/Ufv1SRtkg7RH8dyiOGAX3YJIgaX09Xxa rBF0y3/JG75C201UY5lH/C8/kpnfW6YYKjlPvurpTliy2U8rJ07/iII8MdLnspCNUB4S 2EatGxKurkbYmNdYYQVtGwK/o8p1jy/wElpiUtkIKz/ib2Hgkfy9IHSOtDCCfXoFYENE /1DYt4mwRlfozflxLMffmjypQLP+q0UdwePPx2xL+Xyj/1Lu0oihxkM2/aHHIgKceXtF YtRA== X-Forwarded-Encrypted: i=1; AJvYcCXfqwU5begej4CTd7fXCJBkupofZnRF8sow1PWCXF0w/SSVxgfpOt5iIBSkw4XiYyIgJYz1AxmTwhuiTLs=@vger.kernel.org X-Gm-Message-State: AOJu0YzOiMmTrhirWhg7UDnzFGB4Cr3F4/l7cg23yYvbW3L6HlHDwcz4 /Q1faUALwYZavREl9QbiJhkq7EMKMh64xAZ4SCruv5AKqRykbR8hN5IrkFvWWmG10I0B90Z9l/U fUa2bv5RuHwzhIDMUbETQ20loIw== X-Received: from pfbbk8.prod.google.com ([2002:aa7:8308:0:b0:829:a0ce:bddc]) (user=joonwonkang job=prod-delivery.src-stubby-dispatcher) by 2002:a05:6a00:a21a:b0:829:8942:2c85 with SMTP id d2e1a72fcca58-829f6f27318mr5683442b3a.17.1773310759993; Thu, 12 Mar 2026 03:19:19 -0700 (PDT) Date: Thu, 12 Mar 2026 10:19:08 +0000 In-Reply-To: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: X-Mailer: git-send-email 2.53.0.473.g4a7958ca14-goog Message-ID: <20260312101918.2484082-1-joonwonkang@google.com> Subject: Re: [PATCH] RFC: mailbox: Fix NULL message support in From: Joonwon Kang To: dianders@chromium.org Cc: andersson@kernel.org, arnd@arndb.de, jassisinghbrar@gmail.com, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > Hi, >=20 > On Tue, Mar 10, 2026 at 5:46=E2=80=AFPM Jassi Brar wrote: > > > > On Tue, Mar 10, 2026 at 7:15=E2=80=AFPM Doug Anderson wrote: > > > > > > Hi, > > > > > > On Tue, Mar 10, 2026 at 4:59=E2=80=AFPM Jassi Brar wrote: > > > > > > > > On Tue, Mar 10, 2026 at 6:52=E2=80=AFPM Doug Anderson wrote: > > > > > > > > > > Hi, > > > > > > > > > > On Tue, Mar 10, 2026 at 4:46=E2=80=AFPM wrote: > > > > > > > > > > > > From: Jassi Brar > > > > > > > > > > > > The active_req field serves double duty as both the "is a TX in > > > > > > flight" flag (NULL means idle) and the storage for the in-fligh= t > > > > > > message pointer. When a client sends NULL via mbox_send_message= (), > > > > > > active_req is set to NULL, which the framework misinterprets as > > > > > > "no active request." This breaks the TX state machine by: > > > > > > > > > > > > - tx_tick() short-circuits on (!mssg), skipping the tx_done > > > > > > callback and the tx_complete completion > > > > > > - txdone_hrtimer() skips the channel entirely since active_req > > > > > > is NULL, so poll-based TX-done detection never fires. > > > > > > > > > > > > Fix this by introducing a MBOX_NO_MSG sentinel value that means > > > > > > "no active request," freeing NULL to be valid message data. The > > > > > > sentinel is internal to the mailbox core and is never exposed t= o > > > > > > controller drivers or clients. > > > > > > > > > > > > The only tradeoff is that 'MBOX_NO_MSG' can not be used as a me= ssage > > > > > > by clients. > > > > > > > > > > > > Signed-off-by: Jassi Brar > > > > > > --- > > > > > > drivers/mailbox/mailbox.c | 15 +++++++++------ > > > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > While I can certainly be corrected, I suspect this patch will bre= ak a > > > > > lot of users. > > > > > > > > > > My analysis of people that were passing NULL mbox messages is tha= t > > > > > they _wanted_ the current behavior. They didn't want NULL message= s to > > > > > be queued up as would happen with this patch. Instead, they just > > > > > wanted to immediately ring the doorbell again to signal an interr= upt > > > > > to the other side. > > > > > > > > > They won't be queued up because they call mbox_client_txdone() > > > > immediately after mbox_send_message() > > > > From my analysis of the 14 clients, there was just one > > > > (irq-qcom-mpm.c) that was not doing it by oversight, and I submitte= d a > > > > patch. > > > > > > > > Can you please point me to the driver you are concerned about? Perh= aps > > > > I am overlooking something. > > > > > > Ah, interesting! I hadn't thought about it that way... > > > > > > I think there are at least a few others that would need to change, > > > maybe? It looks like these ones: > > > > > > drivers/soc/xilinx/zynqmp_power.c > > > drivers/remoteproc/xlnx_r5_remoteproc.c > > The underlying driver for both is drivers/mailbox/zynqmp-ipi-mailbox.c > > which sets > > txdone_poll =3D true and implements the .last_tx_done() callback. > > The client (zynqmp_power.c) should not call mbox_client_txdone() > > > > > drivers/firmware/imx/imx-dsp.c > > The underlying driver is drivers/mailbox/imx-mailbox.c which sets > > 'txdone_irq =3D true' > > and ticks the state machine with mbox_chan_txdone() > > Again the client need not call mbox_client_txdone() > > > > All three clients above are currently failed by the core which > > misinterprets NULL messages. > > This patch will fix such clients too, besides avoiding a new api. >=20 > ...but doesn't that mean that the behavior of these clients will > change? Previously all calls to mbox_send_message() immediately called > through to the mailbox controller. Now, if the previous message hasn't > finished, the "NULL" messages will queue up. >=20 > For instance, let's say that our client calls mbox_send_message() 2 > times in quick succession (10 us apart). Let's say that the remote > processor takes 1 ms to react. >=20 > Previously, the 2 calls to mbox_send_message() would probably be > coalesced. Each would call through to the mailbox controller, which > would make sure the doorbell was asserted. After 1 ms, the remote > processor would confirm the single doorbell it saw. >=20 > Now, the first call will ring the doorbell. The second one will queue > up. After 1 ms, the remote processor will confirm the doorbell, which > will cause the second doorbell to be sent. >=20 >=20 > Looking at the 3 drivers in question, I _guess_ maybe that situation > never comes up for them? So maybe they're all fine? I don't have tons > of confidence that I understand enough about these clients to say for > sure that this doesn't happen... >=20 If multiple threads are ringing the doorbell through the same channel many times within a very short period of time, I think the issue you are concerned about could occur in theory with this patch even if the doorbell ack could be ignored. If that is the case, I believe it will be clients to handle the send failure, e.g. by retrying. >=20 > > > In my case, I have a mailbox driver that's currently downstream > > > (though I hope to change that). My mailbox controller has an interrup= t > > > for txdone, so mailbox clients _shouldn't_ call mbox_client_txdone(). > > > Some clients of this mailbox client want the txdone interrupt, but > > > some clients of it just care about sending doorbells. > > > > > So imx-dsp.c like? Please let me know what is lacking in the core to > > fully support that. Happy to look into it. >=20 > I know that with my downstream mailbox client, if I let NULL messages > queue up I end up with a queue of a dozen or so NULL messages. The > downstream client is really taking advantage (AKA abusing) the core's > current NULL behavior. It truly does want the "mbox_ring_doorbell" > concept of just making sure the doorbell is asserted and returning > immediately. If the core changes to start queuing NULL messages, we'll > have to figure out some sort of workaround... According to the Jassi's comment on https://lore.kernel.org/all/CABb+yY18OE= vfc8DDUiZqVeQtkmwcOFCSTMT7KoXb1LVA3RuxdA@mail.gmail.com/ "A controller driver is supposed to either expect data or not, but not both= ", client drivers should send to a controller either non-NULL or NULL data, not both in a mixed fashion. With this setup, if the doorbell acknowledgement from the remote could be ignored by the controller for your case, I believe you could separate a controller for doorbell from the one for non-doorbell and set the controller as follows. ``` mbox_controller->txdone_poll =3D true; mbox_controller->txpoll_period =3D 0; mbox_controller->ops->last_tx_done =3D ... just returns true ... ``` Then it will dequeue the NULL messages as fast as the hrtimer's resolution interval. Or, you could just use mbox_send_message() and mbox_client_txdone() pair. On the other hand, if the doorbell acknowledgement is to be respected by the controller(this is my case), the controller should wait until the ongoing doorbell tx is successfully done or not, and this patch could do it whereas mbox_ring_doorbell() couldn't.