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 8A48D3FF883; Tue, 24 Mar 2026 14:37:45 +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=1774363065; cv=none; b=R5CcRZLLt1ePZi9dcJSgA70R/wttfQH7lBQadSQd4IwW5ZRIB2QJloYTdFTAd+T0Fd+qs1pwlDTVQDnixBNUt/fp6wqiRU8rW95OSUzjT9mXlkiAa8cu3dRR+3JeKijDk38N2ANvSGag5pNDmo3YpC110jUTaeY3cvxZHAqmEzk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774363065; c=relaxed/simple; bh=3M1pZU/Rt+tvSzSNAmvvsplHgsc63Xp92Dg1sukL488=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=r1wAPqawZ8gbKXJwEGwwYVOExklBWq5ZP+MG962RukB8eseoacKgIsx1pie3ndRag21j8KFBS7KPHEvfbZ/4NocuYkPlAVEH+t0IgdHFdEYZf7crgFeS9Be7li+/3kP6iv7tED1hS+WlPzI8etIwGQyXujaUQvwEnrspoEyVy7I= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=PqgKbgJe; 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="PqgKbgJe" Received: by smtp.kernel.org (Postfix) with ESMTPSA id D50C4C2BC87; Tue, 24 Mar 2026 14:37:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1774363065; bh=3M1pZU/Rt+tvSzSNAmvvsplHgsc63Xp92Dg1sukL488=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=PqgKbgJeA0TFHNoShupCs/8BjLfjfpRdFRc1+QL9ZroTHAZf625/7hlReKSHx5E/X ujj1Czq7HLgl5sgc86N/UZqJmKXs5sXug11u7ckU6kFsKZeFu8y2ivJczOV62YjaJO +HCznSqBIDkIwgkH35og13sT0N5yAszfd7Sosv/UfL/ncpPo13dT7INSEP9bD8Qifv x9OR4odB+qoi2Zp/6YzvsORLj3eUER7PaXs2RiLdKK505wnePKh/fIFJPEKvFjH3jb 99ew1U06WRjPOp1dXTb0oM3PLvncpi9kyk82CSA6c9e07DUqyz2PSGKu2acRbKnOpo RTSgXB53nQebw== From: Andreas Hindborg To: Gary Guo , Danilo Krummrich , aliceryhl@google.com, acourbot@nvidia.com, ojeda@kernel.org, boqun@kernel.org, gary@garyguo.net, bjorn3_gh@protonmail.com, lossin@kernel.org, tmgross@umich.edu, abdiel.janulgue@gmail.com, daniel.almeida@collabora.com, robin.murphy@arm.com Cc: driver-core@lists.linux.dev, nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/8] rust: dma: add generalized container for types other than slices In-Reply-To: References: <20260320194626.36263-1-dakr@kernel.org> <8WwJUXIwwCnCTXlAJw77hgSFS-jjhGgfJQoyIaVPdDf99QqTFd1uHhEj56PCo9Ihv-koAQP7826Vpx8HZkKE5A==@protonmail.internalid> <20260320194626.36263-3-dakr@kernel.org> <877br1qr6v.fsf@kernel.org> Date: Tue, 24 Mar 2026 15:37:35 +0100 Message-ID: <87341pz428.fsf@t14s.mail-host-address-is-not-set> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain "Gary Guo" writes: > On Tue Mar 24, 2026 at 1:42 PM GMT, Andreas Hindborg wrote: >> "Danilo Krummrich" writes: >> >>> + /// Returns a reference to the data in the region. >>> /// >>> - /// # Examples >>> + /// # Safety >>> /// >>> - /// ``` >>> - /// # use kernel::device::{Bound, Device}; >>> - /// use kernel::dma::{attrs::*, CoherentAllocation}; >>> + /// * Callers must ensure that the device does not read/write to/from memory while the returned >>> + /// slice is live. >>> + /// * Callers must ensure that this call does not race with a write to the same region while >>> + /// the returned slice is live. >>> + #[inline] >>> + pub unsafe fn as_ref(&self) -> &T { >>> + // SAFETY: per safety requirement. >> >> Is this enough? Don't you need to assert a valid bit pattern? I assume >> you get this from `FromBytes`, but that bound is not on `T` in this impl >> block. > > Concetually `Coherent` is a container of `T` so it should always hold a valid > bit pattern for the type. Perhaps this should be added to the type invariant. > > FromBytes is only needed at the constructor level as that's the only place where > you're asserting that initial bit pattern is valid. Makes sense to add the invariant and assert at construction time. > > >> >>> + unsafe { &*self.as_ptr() } >>> + } >>> + >>> + /// Returns a mutable reference to the data in the region. >>> /// >>> - /// # fn test(dev: &Device) -> Result { >>> - /// let c: CoherentAllocation = >>> - /// CoherentAllocation::alloc_attrs(dev, 4, GFP_KERNEL, DMA_ATTR_NO_WARN)?; >>> - /// # Ok::<(), Error>(()) } >>> - /// ``` >>> - pub fn alloc_attrs( >>> + /// # Safety >>> + /// >>> + /// * Callers must ensure that the device does not read/write to/from memory while the returned >>> + /// slice is live. >>> + /// * Callers must ensure that this call does not race with a read or write to the same region >>> + /// while the returned slice is live. >>> + #[expect(clippy::mut_from_ref, reason = "unsafe to use API")] >>> + #[inline] >>> + pub unsafe fn as_mut(&self) -> &mut T { >>> + // SAFETY: per safety requirement. >> >> Same. >> >>> + unsafe { &mut *self.as_mut_ptr() } >>> + } >>> + >>> + /// Reads the value of `field` and ensures that its type is [`FromBytes`]. >>> + /// >>> + /// # Safety >>> + /// >>> + /// This must be called from the [`dma_read`] macro which ensures that the `field` pointer is >>> + /// validated beforehand. >>> + /// >>> [...] >>> + >>> +impl Coherent { >>> + /// Allocates a region of `T` of coherent memory. >>> + #[expect(unused)] >>> + fn alloc_with_attrs( >>> dev: &device::Device, >>> - count: usize, >>> gfp_flags: kernel::alloc::Flags, >>> dma_attrs: Attrs, >>> - ) -> Result> { >>> - build_assert!( >>> - core::mem::size_of::() > 0, >>> - "It doesn't make sense for the allocated type to be a ZST" >>> - ); >>> + ) -> Result { >>> + const { >>> + assert!( >>> + core::mem::size_of::() > 0, >>> + "It doesn't make sense for the allocated type to be a ZST" >>> + ); >>> + } >>> >>> - let size = count >>> - .checked_mul(core::mem::size_of::()) >>> - .ok_or(EOVERFLOW)?; >>> let mut dma_handle = 0; >>> // SAFETY: Device pointer is guaranteed as valid by the type invariant on `Device`. >>> let addr = unsafe { >>> bindings::dma_alloc_attrs( >>> dev.as_raw(), >>> - size, >>> + core::mem::size_of::(), >>> &mut dma_handle, >>> gfp_flags.as_raw(), >>> dma_attrs.as_raw(), >>> ) >>> }; >>> - let addr = NonNull::new(addr).ok_or(ENOMEM)?; >>> + let cpu_addr = NonNull::new(addr.cast()).ok_or(ENOMEM)?; >>> // INVARIANT: >>> - // - We just successfully allocated a coherent region which is accessible for >>> - // `count` elements, hence the cpu address is valid. We also hold a refcounted reference >>> - // to the device. >>> - // - The allocated `size` is equal to `size_of:: * count`. >>> - // - The allocated `size` fits into a `usize`. >>> + // - We just successfully allocated a coherent region which is adequately sized for `T`, >>> + // hence the cpu address is valid. >> >> The invariant says "valid for the lifetime", do you need to argue for >> the duration of the validity? > > It says "For the lifetime of an instance of ..." which is basically equal to > saying nothing. I think that part should just be removed. Sounds good. > >> >>> + // - We also hold a refcounted reference to the device. >> >> This does not relate to the second invariant of `Self`. > > The second invariant is about size, which is addressed in the first bullet > point. This is about the third invariant, which the doc-comemnt just says "TODO" > currently. So it is not an invariant, and it should not be covered here. I would suggest that if you use bullet list both in definition and justification, please make sure the justification bullets match the definition bullets. If you need to, you can make sub-lists by indenting. Best regards, Andreas Hindborg