From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 58AE7261B71; Thu, 11 Sep 2025 20:34:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757622863; cv=none; b=I1RAYNDyyQ+TgW2IpVsUcS2k7OinL4H6xN+blLzslu4meQIzprBQDxaI21ETCFjepuPhCUrb9mLLhV5zE8/SUCxgN7KR8snWvhQqflQnO/7JKqhuz3M4PMY3ej3RWMhDdr2fAppzuRvg6wexTksmVJzHjZIaRUshakjrBY3gplk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757622863; c=relaxed/simple; bh=PwiTUdsdKjtwPrO614rq/+6R+VG37tYxbhPVowLUnyc=; h=Mime-Version:Content-Type:Date:Message-Id:From:Subject:Cc:To: References:In-Reply-To; b=JSgHJszvNE6gb4qc/2jqiOybK2CVdeUZjTy2JEk7kada18CCXZB8EyincBuXU6VveQ86BneYhaL6BEsGEP7eDKBmwReyueMgusVa/KzxZetZKXq5np4EPIATvVumxHyAw07nDA2hkp+/X477OgcAiXC82APlLII9EfEaoYvA5VQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=CvjyN5vx; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="CvjyN5vx" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5C539C4CEF0; Thu, 11 Sep 2025 20:34:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1757622863; bh=PwiTUdsdKjtwPrO614rq/+6R+VG37tYxbhPVowLUnyc=; h=Date:From:Subject:Cc:To:References:In-Reply-To:From; b=CvjyN5vxeViqPaDQv2IiF+KNe/or8yTABIe5zBmvFsUcfgvmgKIOvO9OEFZeNMYJV zjR5fnn7mt2dIvbseba9zV0igdXIUlfBKufrhnDpmvB7WXulMdmyBW1CnimrVwODC9 PLZp0g+5NRv6a+xelxbdRu7jYKf93woBjrQ4mcnaBbWS+/kwbvy4lw4vijkUZuNZBe 1oOHnluxF47wWpZV5jjWi2yuWEt9HU+R0uOo5PspzuV6YlInnQawCcJcv8pOAvQRBN FMHB312KtzQJw8yVhYHpCUJEbAGkaCpqyHw+QfBlRPBk0ogVFm/RBzgXXYhoSzFreH SYPKLm8tK350g== Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 11 Sep 2025 22:34:17 +0200 Message-Id: From: "Danilo Krummrich" Subject: Re: [PATCH v5 2/3] rust: i2c: add manual I2C device creation abstractions Cc: "Miguel Ojeda" , "Alex Gaynor" , "Wolfram Sang" , "Boqun Feng" , "Gary Guo" , =?utf-8?q?Bj=C3=B6rn_Roy_Baron?= , "Benno Lossin" , "Andreas Hindborg" , "Alice Ryhl" , "Trevor Gross" , "Greg Kroah-Hartman" , "Viresh Kumar" , "Asahi Lina" , "Wedson Almeida Filho" , "Alex Hung" , "Tamir Duberstein" , "Xiangfei Ding" , , , To: "Igor Korotin" References: <20250911154717.96637-1-igor.korotin.linux@gmail.com> <20250911155015.97250-1-igor.korotin.linux@gmail.com> In-Reply-To: <20250911155015.97250-1-igor.korotin.linux@gmail.com> On Thu Sep 11, 2025 at 5:50 PM CEST, Igor Korotin wrote: > +impl I2cAdapter { > + /// Gets pointer to an `i2c_adapter` by index. > + pub fn get(index: i32) -> Result> { Where do we get this index usually from? OF, ACPI, etc. I assume? I feel li= ke it could make sense to wrap it into a new type. Even though it is not safety relevant it eliminates a source for mistakes. > + // SAFETY: `index` must refer to a valid I2C adapter; the kernel > + // guarantees that `i2c_get_adapter(index)` returns either a val= id > + // pointer or NULL. `NonNull::new` guarantees the correct check. > + let adapter =3D NonNull::new(unsafe { bindings::i2c_get_adapter(= index) }).ok_or(ENODEV)?; > + > + // SAFETY: `adapter` is non-null and points to a live `i2c_adapt= er`. > + // `I2cAdapter` is #[repr(transparent)], so this cast is valid. > + Ok(unsafe { (&*adapter.as_ptr().cast::>()).into() }) > + } > +} > + > +impl Drop for I2cAdapter { > + fn drop(&mut self) { > + // SAFETY: This `I2cAdapter` was obtained from `i2c_get_adapter`= , > + // and calling `i2c_put_adapter` exactly once will correctly rel= ease > + // the reference count in the I2C core. It is safe to call from = any context > + unsafe { bindings::i2c_put_adapter(self.as_raw()) } > + } > +} The Drop implementation is not needed, you only ever give out an ARef, but never a "raw" I2cAdapter, which is the correct thing = to do. > + > +// SAFETY: `I2cAdapter` is a transparent wrapper of a type that doesn't = depend on `I2cAdapter`'s generic > +// argument. > +kernel::impl_device_context_deref!(unsafe { I2cAdapter }); > +kernel::impl_device_context_into_aref!(I2cAdapter); > + > +// SAFETY: Instances of `I2cAdapter` are always reference-counted. > +unsafe impl crate::types::AlwaysRefCounted for I2cAdapter { > + fn inc_ref(&self) { > + // SAFETY: The existence of a shared reference guarantees that t= he refcount is non-zero. > + unsafe { bindings::i2c_get_adapter((*self.as_raw()).nr) }; Please make accessing the nr field a separate inline function, or at least = put it in a separate unsafe block. > + } > + > + unsafe fn dec_ref(obj: NonNull) { > + // SAFETY: The safety requirements guarantee that the refcount i= s non-zero. > + unsafe { bindings::i2c_put_adapter(&raw mut (*obj.as_ref().as_ra= w())) } Same here, separate unsafe blocks please. > + } > +} > + > +impl AsRef> for I2cAdapter { > + fn as_ref(&self) -> &I2cAdapter { > + &self > + } > +} This AsRef implementation doesn't seem to do anything?