From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.mail-out.lima-city.de (mx1.mail-out.lima-city.de [91.216.248.203]) (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 00E782F532F; Sun, 27 Sep 2026 22:53:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.216.248.203 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790549641; cv=none; b=JWyubjgMT9CF1DAsjCQzlPyz6XnrN0ig0e7xL/eBbR+ld/iHwqf4xoLYkqEn/j23C/GUkdfoziYvgSNu7L9zAXPVnwSXUQEJfs6LFb5f8NfyldXceBcjxV3muUCdNdUlQ8s1ANXKxT+r89jCqF7LVW7V4fABUMPPxENPINIbS7g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790549641; c=relaxed/simple; bh=xC9euzdpfWQA6clWp09WVE/7sgF5HEkIjOsWgJhhgTE=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: Content-Type:MIME-Version; b=lujh6DGJD09q1B3ff4zuXczW+Feoyu75KGzVEUsD8cyu0ZoK/XoFAeGaq4InekaARYci93+/9V0mDgmzama9RlZ/Hj1YE2wubTONM+d4SGskl7lwafbs6W2HWbZt8r/WDhS1yHTcCBhJxOya1E+v8FkymKnVhVSfcj/wWP38Bnk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=matt3o12.de; spf=pass smtp.mailfrom=matt3o12.de; dkim=pass (2048-bit key) header.d=matt3o12.de header.i=@matt3o12.de header.b=QPxsBAyX; arc=none smtp.client-ip=91.216.248.203 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=matt3o12.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=matt3o12.de Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=matt3o12.de header.i=@matt3o12.de header.b="QPxsBAyX" From: Matteo Kloiber X-Lima-ML-UUID: 7b21bed0-f515-40d2-8fad-e214b5c9faf0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=matt3o12.de; s=securedbylima-20230709; t=1790549158; bh=xC9euzdpfWQA6clWp09WVE/7sgF5HEkIjOsWgJhhgTE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=QPxsBAyXObGEqRuVtVwk+kG/MmUHlCLH7ftmtL5BWr2j8oaZf0M4V9Q3kIBZ9zV+2 uylOzjCrgcHThRhe5hs4jq2HvGiNof6YkMwPPkWx3ToNAJplgp+cp0SpLJ0eZShIhP cL9aqqpJMKK1jiCRLqPoDgiKaEH3N2yN8bY7vVrd8qp8ScHRVQw3QGZxZ9UKv/+oAR xLVSKeXX1+T/jtMI9IQloUVQvaHzI/703WAmDE/7Jdc8d2wO5bvZAMStVQq6a1vbR5 RNVGTGvhjnpL8x33L1GqLVpWnHzdQflfLmVHlzU0CgiRtDVX2EmxDPhXofOm2Jdl3V I/ygm44GFUvqA== To: Alexandre Courbot Cc: Danilo Krummrich , Gary Guo , Robin Murphy , aliceryhl@google.com, ojeda@kernel.org, airlied@gmail.com, simona@ffwll.ch, abdiel.janulgue@gmail.com, daniel.almeida@collabora.com, a.hindborg@kernel.org, nova-gpu@lists.linux.dev, dri-devel@lists.freedesktop.org, driver-core@lists.linux.dev, rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] rust: scatterlist: honor the device's maximum segment size Date: Mon, 28 Sep 2026 00:44:43 +0200 Message-ID: <20260927224446.1053037-1-kernel@matt3o12.de> X-Mailer: git-send-email 2.51.2 In-Reply-To: References: <20260831233215.287881-1-kernel@matt3o12.de> <20260831233215.287881-3-kernel@matt3o12.de> <20260913210806.125589-1-kernel@matt3o12.de> Content-Type: text/plain; charset="utf-8" Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit First of all, thanks for merging my first two patches and sorry about my late reply. What I think Alex meant and what I agree with is that the new API should focus on making semantic bugs harder, not to necessarily make it easier to write safer drivers. That being said, I think we can do both with this change. The key change here is that we introduce a structure that defines dma parameters for a device. This structure deliberately has no default implementation and new fields should be added even if they require changes for all dma drivers. ``` pub struct Params { pub streaming_mask: DmaMask, pub coherent_mask: DmaMask, pub max_seg_size: NonZeroU32, } ``` Suppose, we didn't have `max_seg_size` before and only discovered it later. We would add it and the compiler would guide us to any drivers that will need to consider an appropriate value for `max_seg_size`. If rust was bigger in the kernel, this could be quite a bit of work, requiring coordination with other maintainers, but I would call this a feature because it actually requires everyone to at least consider what the right value is instead of forgetting to set it. The same is also true for new drivers. Authors do not have to be omniscient of all dma related settings and instead just fill out the struct and already have some important ones set. If a parameter is only important for some drivers but those drivers should not forget it, we could make it an Option so that drivers that really don't need a parameter can set it to `None`, but they had to think about it at least. I think, instead of discussing different possibilities, it might be easier to discuss code directly. Taking Alex's example, I went ahead and wrote a small LLM assisted PoC. I'm happy to share it here or in another thread, but here are the main changes: 1. The PCI and platform probe callbacks receive a unique setup token: ```rust fn probe<'bound>( dev: &'bound Device>, dma: dma::Setup<'bound>, id_info: Option<&'bound Self::IdInfo>, ) -> impl PinInit, Error> + 'bound; ``` The bus driver creates this token and drivers cannot obtain another one from the device. 2. `setup.configure(params)` consumes the token, applies the parameters through the C DMA setters, and returns a `dma::Handle<'bound>` on success. This combines the setters and `finish()` from Alex's outline into one call. ``` pub fn configure(self, params: Params) -> Result> { let dev = self.dev.as_raw(); // SAFETY: `dev` is valid and its DMA parameters are initialized by the bus. The `Setup` // invariants exclude concurrent DMA operations and other parameter setters. Consuming // the unique token prevents reconfiguration after a handle has been issued. unsafe { to_result(bindings::dma_set_mask(dev, params.streaming_mask.value()))?; to_result(bindings::dma_set_coherent_mask(dev, params.coherent_mask.value()))?; bindings::dma_set_max_seg_size(dev, params.max_seg_size.get()); } Ok(Handle { dev: self.dev }) } ``` 3. The handle contains a bound device reference and is copyable, but provides no way to reconfigure DMA: ```rust #[derive(Clone, Copy)] pub struct Handle<'bound> { dev: &'bound device::Device, } ``` 4. Allocation and mapping APIs now require a Handle. For example, scatter list now takes: ``` pub fn new<'a>( dma: dma::Handle<'a>, pages: P, dir: dma::DataDirection, flags: alloc::Flags, ) -> impl PinInit + 'a { ``` The PoC uses Handle for coherent allocations, owned scatterlists, and DRM scatterlist mappings. 5. Each pci/platform driver that uses DMA configures its own `Params`. Let's look at `samples/rust/rust_dma.rs`: ```rust fn probe<'bound>( pdev: &'bound pci::Device>, setup: dma::Setup<'bound>, _info: Option<&'bound Self::IdInfo>, ) -> impl PinInit, Error> + 'bound { pin_init::pin_init_scope(move || { dev_info!(pdev, "Probe DMA test driver.\n"); let mask = DmaMask::new::<64>(); let dma = setup.configure(dma::Params { streaming_mask: mask, coherent_mask: mask, max_seg_size: const { NonZeroU32::new(u32::SZ_64K).unwrap() }, })?; let ca: Coherent<'_, [MyStruct]> = Coherent::zeroed_slice(dma, TEST_VALUES.len(), GFP_KERNEL)?; for (i, value) in TEST_VALUES.into_iter().enumerate() { io_project!(ca, [panic: i]).copy_write(MyStruct::new(value.0, value.1)); } let size = 4 * page::PAGE_SIZE; let pages = VVec::with_capacity(size, GFP_KERNEL)?; let sgt = SGTable::new(dma, pages, DataDirection::ToDevice, GFP_KERNEL); Ok(try_pin_init!(Self::Data { pdev, ca, sgt <- sgt, })) }) } ``` What I am not 100% sure about is how we should handle drivers that don't use dma. Currently I just discarded the setup token like suggested, for example `samples/rust/rust_driver_auxiliary.rs`: ```rust impl pci::Driver for ParentDriver { fn probe<'bound>( pdev: &'bound pci::Device>, _dma: kernel::dma::Setup<'bound>, _info: Option<&'bound Self::IdInfo>, ) -> impl PinInit, Error> + 'bound { ... } } ``` But maybe it would make sense to introduce a C API such as: `debug_no_dma` and have the bus call it unless configure was called first. This would then trigger a debug warning if a marked rust device calls any C APIs that use DMA regardless. But this could also very well be a worthwhile follow up.