From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f181.google.com (mail-pl1-f181.google.com [209.85.214.181]) (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 BF96338E8CC for ; Fri, 10 Jul 2026 19:47:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.181 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783712834; cv=none; b=IOyG7G+D+GCaX9HXMTGdwUxlcIg2Ukel94zK1970PFLOuRWHBQ2vXV2a7r9oxWqvCNlEdapnzgYAyWMjIxP4IPH2F4fs7SkopIQhyjXk+u6cqcQZVPLYVoqT8Q6lyuFlHpJ28VVtgFB73YWT8Bo5xFWePuiB77yI/+Q/zsa7FPQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783712834; c=relaxed/simple; bh=/6ZFlyF2u8YZXQDVe+9p51Y+eSDbo/i4HiDhXUU738g=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=E9gGGI3lXMmpmEtzbnlVklvMKdYO7xFJy7QmITR5EzpIoxFu65NhlvY9hC01/eS2RIQgl5/SJPGoBSPNVT8Y+wxPGBfuutJbgLeiNzIFUP9HYIQNNIIymserKLwKQVeQzJeliozWJpO1vZBe7zTKGyliW3ayV2Qfp17Amv9Qrbk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=RW2oKJEk; arc=none smtp.client-ip=209.85.214.181 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=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="RW2oKJEk" Received: by mail-pl1-f181.google.com with SMTP id d9443c01a7336-2ccdf36f63dso23185ad.0 for ; Fri, 10 Jul 2026 12:47:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1783712832; x=1784317632; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=CGpDTWyhH4xrOTpV3gYMpAtM3fyjq8FwoLgexTgA66c=; b=RW2oKJEk/qfM+M2+GNfv3orzLPznSjfKKGZR7oaWw4Eak+4YgIJ69TYSCsGursbQSM HKAmGRssGQlCa8DAwdJWMGJndKZZAZ74THhXn99VbZOxfXuiqjJ38U1ZV5Pky6xvDBHc g3Yhy/8P+fEucPUx5bQd6sNHSeet6nK7RR+e1+YxPzUeNX1V8WufInRLSFGdltp/aQ7l By5jJpkXKvvjbeiouWy4Xv9lwGji5OkWS/pWDoDvZFkwIjawDNnfT4qixTBP+fVWgwg7 3uDQhSKwWZdeKQMugxmyWcO8SahOGhmzoaUKw9M3hzWOjrk3HjAklJcACWMTXzpitww9 3zxg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783712832; x=1784317632; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=CGpDTWyhH4xrOTpV3gYMpAtM3fyjq8FwoLgexTgA66c=; b=A0m6Z0+pscLwsce2pmSsRxAQVzxbSXbho55bQl8QxDuNSpwAX61y4BdRS7c5cQToXP Z5FXpeLDQYHsYPdOvNyt4yj3MNjsFw01M93S8FH6wpepsdbcgTZTckPrKjOA64zc7KW9 p7bYA0wWxVQq+GHcZ7JFv8yHCYv6lFvVwwhMCkq6VHIR0XoOISt9o33pBjcdHPSW9HIp FhT5pS0TXixsvyM701iGM6wfxorLQgD04hfJ/TOllY9LqPMReAn6h9ZRXcBE7v9yftB9 PmtmOV1RJShYChOMzv69YtmPUyYqA07zaEN1tc1Ud1pSHjHL6Giq6B9bIbigz5EF8Z6F 9IzQ== X-Forwarded-Encrypted: i=1; AHgh+RqzB/UsOMlZrNPmcIxppGt0GAPiBemOi+HmDMeIAivBA1+YEVfalTrLNp/9GKB9/SIzBo8/HU8lRxEvx7g=@vger.kernel.org X-Gm-Message-State: AOJu0YwfUx4J/r3UdxSYKeirNFGHPv2uKw+GQQNpiAh/gneufeR2S6ah diBK9hUqOFCIyqueI8/sZ4URV6wtinKjGzK4c7SVP/WvPSF3qIYzUfLn6N3vUeqcyw== X-Gm-Gg: AfdE7cklIChcHbI6XJGESkOBzTsSzZyBunno7CSkcUYyRrdsp3PKaMskFpYBYKRlrpm WoHWxkqka/VWjZFdc0DWGD5EGcqvN63As1sh0c8Cj29GW7mKRQH08Eco4zQ3Yhnkq4lfmzHnNDX VPT8nbNJftszi4t6ZLPLRRXLOENebtAhDcfZanrPKUhBJpGbbGBalfueiknikpQcexauuI+BBHG 31NQs/u440VvioWnx+Fc6gzxhova7On58F/poGoq35eX2DDGEmvwYGD8VaaA2klmJhGUo1/QEST YTM8mPbKM7O6iVp7+seTo/1uOOt+XOWsRS3ExrK521UO+mWCWYBXCAEX5XeWrDcfiiEgikxRuEs +56JlQ7zSL4SniNSQ9TYYLPFKadOhKaVA3l/UMNXHbBqxauDFLONuZQzU0IC56P3MFbD8yUgHfh IGiXayAI5fkprvPBqVP2i4/6Lch/SxKC5xm5VlEUtj1lxsdNbkCHncmA93o7HyE3PEeAO7uKIO8 MjG/g8lxMtNYI3V1w0eG7J+KKps/eoOSIVm X-Received: by 2002:a17:902:e88b:b0:2c7:9e6a:1a8d with SMTP id d9443c01a7336-2ce9eaeca48mr695745ad.12.1783712831478; Fri, 10 Jul 2026 12:47:11 -0700 (PDT) Received: from google.com (112.174.16.34.bc.googleusercontent.com. [34.16.174.112]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-38d11280409sm924921a91.0.2026.07.10.12.47.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Jul 2026 12:47:10 -0700 (PDT) Date: Fri, 10 Jul 2026 19:47:05 +0000 From: Carlos Llamas To: jahnavimn@google.com Cc: Greg Kroah-Hartman , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Todd Kjos , Christian Brauner , Alice Ryhl , Miguel Ojeda , Boqun Feng , Gary Guo , =?iso-8859-1?Q?Bj=F6rn?= Roy Baron , Benno Lossin , Andreas Hindborg , Trevor Gross , Danilo Krummrich , Daniel Almeida , Tamir Duberstein , Alexandre Courbot , Onur =?iso-8859-1?Q?=D6zkan?= , linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org Subject: Re: [PATCH v2 1/7] rust_binder: Add dynamic debug logging mask Message-ID: References: <20260710-rust_binder_debug_mask-v2-0-2846410e3ae6@google.com> <20260710-rust_binder_debug_mask-v2-1-2846410e3ae6@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260710-rust_binder_debug_mask-v2-1-2846410e3ae6@google.com> On Fri, Jul 10, 2026 at 02:32:52PM +0000, Jahnavi MN via B4 Relay wrote: > From: Jahnavi MN > > Implement a dynamic debug logging mask (`debug_mask`) for the > `rust_binder` module to allow dynamic runtime configuration of log > levels. This enables parity with the legacy C driver's debug mask. > > Since the Rust `module!` macro in the current kernel build does not yet > support declaring module parameters directly in Rust, we define the > `debug_mask` variable in a C companion file to expose it to the kernel > runtime and import it using FFI with volatile reads. Yeah, declaring it under rust_binderfs.c is not great but I guess we don't have any other alternative for now. So this is fine. > > To verify the setup, instrument process lifecycle events (open, flush, > and release) in `process.rs` under the new `BINDER_DEBUG_OPEN_CLOSE` > logging mask. These entry-point events are chosen for initial validation > because they represent the start of the Binder lifecycle and occur > at low frequency, allowing simple runtime verification of the dynamic > toggle without log noise. > > Signed-off-by: Jahnavi MN > --- > drivers/android/binder/debug.rs | 80 ++++++++++++++++++++++++++++++ > drivers/android/binder/process.rs | 7 ++- > drivers/android/binder/rust_binder_main.rs | 2 + > drivers/android/binder/rust_binderfs.c | 3 ++ > rust/kernel/task.rs | 7 +++ > 5 files changed, 98 insertions(+), 1 deletion(-) > > diff --git a/drivers/android/binder/debug.rs b/drivers/android/binder/debug.rs > new file mode 100644 > index 000000000000..da48bce3df54 > --- /dev/null > +++ b/drivers/android/binder/debug.rs > @@ -0,0 +1,80 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2026 Google LLC. > + > +//! Binder debugging helpers. > + > +#![allow(dead_code)] > + > +kernel::impl_flags!( > + /// Represents multiple debug mask flags. > + #[derive(Debug, Clone, Default, Copy, PartialEq, Eq)] > + pub struct DebugMasks(u32); Where is this being used? I can't find it on subsequent patches. Maybe leftover from previous implementation? > + > + /// Represents a single debug mask category. > + #[derive(Debug, Clone, Copy, PartialEq, Eq)] > + pub enum DebugMask { > + UserError = 1 << 0, > + FailedTransaction = 1 << 1, > + DeadTransaction = 1 << 2, > + OpenClose = 1 << 3, > + DeadBinder = 1 << 4, > + DeathNotification = 1 << 5, > + ReadWrite = 1 << 6, > + UserRefs = 1 << 7, > + Threads = 1 << 8, > + Transaction = 1 << 9, > + TransactionComplete = 1 << 10, > + FreeBuffer = 1 << 11, > + InternalRefs = 1 << 12, > + PriorityCap = 1 << 13, > + Spinlocks = 1 << 14, > + } > +); > + > +extern "C" { > + // Declared as static mut because the value is mutated dynamically by C sysfs. > + static mut rust_binder_debug_mask: u32; > +} > + > +/// Checks if the given debug logging category is enabled in the mask. > +pub(crate) fn debug_mask_enabled(mask: DebugMask) -> bool { > + // SAFETY: `rust_binder_debug_mask` is defined in C and can be mutated at any time. > + // We read it via volatile to ensure we fetch the current value from memory. > + let current_mask = unsafe { core::ptr::read_volatile(&raw const rust_binder_debug_mask) }; > + (current_mask & (mask as u32)) != 0 > +} > + > +/// Prints a debug log if the specified mask category is enabled. > +#[macro_export] > +macro_rules! binder_debug { > + // 1. Internal helper rule to do the raw print. > + (raw, $($arg:tt)*) => { > + kernel::pr_info!($($arg)*); > + }; > + > + // 2. Rule to explicitly specify a PID (used in kworkers). > + (pid=$pid:expr, $mask:ident, $($arg:tt)*) => { > + if $crate::debug::debug_mask_enabled($crate::debug::DebugMask::$mask) { > + $crate::binder_debug!( > + raw, > + "{}: {}\n", > + $pid, > + kernel::prelude::fmt!($($arg)*) > + ); > + } > + }; > + > + // 3. Default rule (automatically prepends "PID:TID" of the current calling thread). Adding the pid:tid implicitly is a good idea. Nice! > + ($mask:ident, $($arg:tt)*) => { > + if $crate::debug::debug_mask_enabled($crate::debug::DebugMask::$mask) { > + let thread = kernel::current!(); > + $crate::binder_debug!( > + raw, > + "{}:{} {}\n", > + thread.tgid(), > + thread.pid(), > + kernel::prelude::fmt!($($arg)*) > + ); > + } > + }; > +} > diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs > index f855d8d9818c..836a0db92354 100644 > --- a/drivers/android/binder/process.rs > +++ b/drivers/android/binder/process.rs > @@ -1349,6 +1349,7 @@ pub(crate) fn lock_with_nodes(&self) -> WithNodes<'_> { > } > > fn deferred_flush(&self) { > + binder_debug!(pid=self.task.pid(), OpenClose, "flushing process"); > let inner = self.inner.lock(); > for thread in inner.threads.values() { > thread.exit_looper(); > @@ -1356,6 +1357,8 @@ fn deferred_flush(&self) { > } > > fn deferred_release(self: Arc) { > + binder_debug!(pid=self.task.pid(), OpenClose, "releasing process"); > + > let is_manager = { > let mut inner = self.inner.lock(); > inner.is_dead = true; > @@ -1658,7 +1661,9 @@ fn ioctl_write_read( > /// The file operations supported by `Process`. > impl Process { > pub(crate) fn open(ctx: ArcBorrow<'_, Context>, file: &File) -> Result> { > - Self::new(ctx.into(), ARef::from(file.cred())) > + let proc = Self::new(ctx.into(), ARef::from(file.cred()))?; > + binder_debug!(OpenClose, "opened process"); > + Ok(proc) > } > > pub(crate) fn release(this: Arc, _file: &File) { > diff --git a/drivers/android/binder/rust_binder_main.rs b/drivers/android/binder/rust_binder_main.rs > index df47aba05133..995e6131bb77 100644 > --- a/drivers/android/binder/rust_binder_main.rs > +++ b/drivers/android/binder/rust_binder_main.rs > @@ -31,6 +31,8 @@ > mod context; > mod deferred_close; > mod defs; > +#[macro_use] > +mod debug; > mod error; > mod netlink; > mod node; > diff --git a/drivers/android/binder/rust_binderfs.c b/drivers/android/binder/rust_binderfs.c > index ade1c4d92499..fa8b38465550 100644 > --- a/drivers/android/binder/rust_binderfs.c > +++ b/drivers/android/binder/rust_binderfs.c > @@ -51,6 +51,9 @@ DEFINE_SHOW_ATTRIBUTE(rust_binder_proc); > char *rust_binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES; > module_param_named(rust_devices, rust_binder_devices_param, charp, 0444); > > +u32 rust_binder_debug_mask = 7; hmm, I'm not sure about this magic number. Do you think we can initialize on the rust end at init()? Or at least a comment about what flags we are setting as default here? > +module_param_named(debug_mask, rust_binder_debug_mask, uint, 0644); > + > static dev_t binderfs_dev; > static DEFINE_MUTEX(binderfs_minors_mutex); > static DEFINE_IDA(binderfs_minors); > diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs > index 38273f4eedb5..1b290c61714d 100644 > --- a/rust/kernel/task.rs > +++ b/rust/kernel/task.rs > @@ -210,6 +210,13 @@ pub fn pid(&self) -> Pid { > unsafe { *ptr::addr_of!((*self.as_ptr()).pid) } > } > > + /// Returns the TGID (Thread Group ID / Process ID) of the given task. > + pub fn tgid(&self) -> Pid { > + // SAFETY: The tgid of a task never changes after initialization, so reading this field is > + // not a data race. > + unsafe { *ptr::addr_of!((*self.as_ptr()).tgid) } > + } > + > /// Returns the UID of the given task. > #[inline] > pub fn uid(&self) -> Kuid { > > -- Only a few optional suggestions so... Reviewed-by: Carlos Llamas