From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f74.google.com (mail-pj1-f74.google.com [209.85.216.74]) (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 0CFEE3451CE for ; Thu, 26 Mar 2026 07:31:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.74 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774510319; cv=none; b=Wl+u3mBKOKR/4vavhVUZ9sbQQduyBig0fN1IcuR3j8EbkR28AUl5qphFaAcivYcoLJh8cNPw45c9g4fM/jIvqQkifFOowQSpuINSZZlY2Qh0KitxvJw5i7Ko3I0F48W38RxwBXdtzzWZYtBJAaYluCVBYpT4dduyWFLvmLua5SE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774510319; c=relaxed/simple; bh=jAG9Jc3W9yLV3CLKQacVGlyRZ1+hqgL+fx9S/k1uBtc=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=MlqpB3CZ37O0R/yn4X2LES51ml6SXyuHM6D/LWiNiwJ4jwSXzYhghQtudYPn/Dk1HlxyaVgdHWPz343SW4pMcAsqVkG5lsrBT8IBnrsvDGdyh9kYaNlPQlmvAXCgir+sWDj1BV/2Jm+pwXRadtvBHGRp7epZyLfNth6iX+FbVlc= 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=VyIjCdt1; arc=none smtp.client-ip=209.85.216.74 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="VyIjCdt1" Received: by mail-pj1-f74.google.com with SMTP id 98e67ed59e1d1-3595485abbbso1134009a91.2 for ; Thu, 26 Mar 2026 00:31:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1774510317; x=1775115117; 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=pF5UUWsg7/Xtcgow5w74ti0Q9r69XYt5gW27Z89ubs8=; b=VyIjCdt15wE7/L80qtvPdH+Iz/Yd98h5V8xR8L5TLSLu6i/Hx7PZRhc7m+5ZB3OE6K 5wQUKvaic+jEVGGbAv97Ls4jsgH64NF6j3gzrB8yZ94SZdrBBu63tOOxm3Kjbvon9gxD wH3GCTH90jyV1k9DZd3A1v8rCr2bNJTQSWYNvvmruaZrXCfrRfzaQj9ni+g4fEmuXolN HYZB+xpeDoUKL1QxSFp02vy8Ke8+iEa9ISPYDHK1y2nU6tdzDtIKi1ypxw2fWcnLRXX/ aPBbxVnbNXSJ853UPV+nBuOUXyPJil6suvoLZWUW4jwwVGrVgjnZnqG3R4iZHKUv7Fjk 8CbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774510317; x=1775115117; 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=pF5UUWsg7/Xtcgow5w74ti0Q9r69XYt5gW27Z89ubs8=; b=dS/IxQevFoXaLc+Q4g+VdicZ0JYonKD9GwasjU21FalOGyMT44wbcMBOpesl/vwNFA hqEEac4zVWLUnCSg1cnayUkAg1XdsdmhdUF2ZPQc0k6CbUn16F/M0YYRxUnPe66Gl19L 0xqa5Di+QfW9W2LHRjDkR44Xmn/3zxBPv+4EtN14KZGdHJbfKKBxroTpG5148V+MFQeT rfmtPEOJyQHPoj90Qmd+m1VFGW59EkRUsZ0Js14vMroBu27uYJYGSkp7fn9eYIz+D4tK NCojlBxtsLcIATdvNyHsvOw2vg2o6ReDiOZLqLhaicQ8mxADYFyNJocI8IO6xvOgmc6s fc0w== X-Forwarded-Encrypted: i=1; AJvYcCWPVnlLzR59HPOfymjZ1Y5l3W/S3PKl9L3CIk3DV+EpC2KMP/VkW/R9NSv5Idyo23adX7mLGz2ivomj8Lo=@vger.kernel.org X-Gm-Message-State: AOJu0Yzf9nchCOeqcBk8uBpq9bhB6hzv4hNV5N7BetkM4MiwgcputdQX 6ORK7UHDhcCvHkBCVxE4FEl3BFdIxCab9IgASwMq1c7cenQQgGFDgowF5hIH1DxREVjnr5IIMvN n/TAL+nzgzBNpouPDGmWsnJzKAw== X-Received: from pgbem14.prod.google.com ([2002:a05:6a02:468e:b0:c73:9dbd:c981]) (user=joonwonkang job=prod-delivery.src-stubby-dispatcher) by 2002:a05:6a21:3290:b0:39b:c3a0:9f31 with SMTP id adf61e73a8af0-39c4ab4e4f6mr7282873637.21.1774510317125; Thu, 26 Mar 2026 00:31:57 -0700 (PDT) Date: Thu, 26 Mar 2026 07:31:53 +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.1018.g2bb0e51243-goog Message-ID: <20260326073155.2360612-1-joonwonkang@google.com> Subject: Re: [PATCH] RFC: mailbox: Fix NULL message support in mbox_send_message() From: Joonwon Kang To: jassisinghbrar@gmail.com Cc: arnd@arndb.de, dianders@chromium.org, joonwonkang@google.com, linux-kernel@vger.kernel.org, akpm@linux-foundation.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > On Fri, Mar 20, 2026 at 4:03=E2=80=AFPM Doug Anderson wrote: > > > > Hi, > > > > On Mon, Mar 16, 2026 at 10:03=E2=80=AFPM Joonwon Kang wrote: > > > > > > > On Fri, Mar 13, 2026 at 5:12=E2=80=AFAM Joonwon Kang wrote: > > > > > > > > > > > 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 following drivers are currently using ->active_req which now = could be > > > > > assigned MBOX_NO_MSG. > > > > > - drivers/mailbox/tegra-hsp.c > > > > > - drivers/mailbox/mtk-vcp-mailbox.c > > > > > > > > > Good catch, Thanks. > > > > mtk-vcp-mailbox.c is fine as is - it assumes only valid requests. T= his > > > > patch will not introduce any change for it. > > > > Yes, tegra-hsp.c should now track MBOX_NO_MSG instead of NULL. Sing= le > > > > line change. > > > > > > > A more important aspect than single line change would be that you are > > > creating a new API contract that the controller drivers who are to us= e > > > ->active_req should now have new knowledge that the pointer value cou= ld > > > be unconventional value such as -1. Without this additional knowledge= , > > > they may easily fail checking if the channel is empty. > > > > > > > > One of them is using ->active_req to wait until the channel is em= pty. In > > > > > this case, strictly speaking, that controller driver should be aw= are of > > > > > the sentinel value MBOX_NO_MSG, which means the sentinel value sh= ould be > > > > > exposed to the controller. Or, if a future controller driver to c= ome is to > > > > > use ->active_req for the same purpose for doorbell or non-doorbel= l, it > > > > > should also be aware of the sentinel value anyway. > > > > > > > > > > However, I believe that it is not intuitive to the controller dev= elopers > > > > > that a pointer value could be other value than a real memory addr= ess, > > > > > NULL or error encoded value, which is -1(=3D=3D MBOX_NO_MSG). For= this reason, > > > > > I think it will be better to change the type of ->active_req to g= ive a > > > > > better indication to the controller developers, e.g. to integer a= s in the > > > > > original patch > > > > > https://lore.kernel.org/all/20251126045926.2413532-1-joonwonkang@= google.com/. > > > > > > > > > > Or, we could change those drivers not to use ->active_req, hide > > > > > ->active_req entirely in the mailbox core and keep this patch. > > > > > > > > > Yes, ideally controller drivers should not track active_req. But t= his > > > > patch will cause least churn it seems. > > > > > > If we will take those two drivers as an exceptional and not recommend= ed > > > case that uses ->active_req directly, it will be fine not to change t= hem > > > except for tegra-hsp.c to check MBOX_NO_MSG. > > > > > > If that is the case and you are to keep this patch, please keep in mi= nd to > > > leave the original patch link in the commit message for better tracka= bility > > > of the discussion and solutions and for credit for the contribution o= f > > > finding and analyzing this issue and proposing the first solution, wh= ich > > > I believe is also important for future contributions to come. > > > > So what's the next steps here, then? I don't think I adequately > > explained exactly the needs of my mbox client, but I also don't think > > it's a big deal. I'm convinced it should be fine even if NULL messages > > get queued. > > > I think it will be simpler than you imagine, but yes I too don't think > it's a big deal. We can iron out details later. >=20 > > Jassi: are you going to send a new version of your patch? ...or are > > you expecting Joonwon to post a new version? ...or do you want me to > > post some variant of these patches? > > > Honestly I only care about minimal churn to code and API. I think > simply using a different sentinel value is simpler than tracking the > number of submissions... which is neither strictly required nor > reduces the changes we have to make. > So I plan to submit my RFC as a patch, the trivial change to > tegra-hsp.c and resubmit the qcom fix with your ack. >=20 > Regards, > Jassi I have reviewed the new patch. Regarding the issue I brought up on how this alternative patch had been uploaded without letting the original patch author know and without any tag= for better tracking or credit, I hope next time you could use "Reported-by:" or "Link:" tag at least to encourage contributors to keep spending effort find= ing and analyzing issues in the mailbox framework. I believe it could be better= for community and quality of code as also guided in the submitting-patches doc. Sincerely, Joonwon Kang