Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 93AB8C678D5 for ; Fri, 24 Feb 2023 14:19:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229918AbjBXOTm (ORCPT ); Fri, 24 Feb 2023 09:19:42 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40286 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229478AbjBXOTk (ORCPT ); Fri, 24 Feb 2023 09:19:40 -0500 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E47E915140; Fri, 24 Feb 2023 06:19:39 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 741FC618EF; Fri, 24 Feb 2023 14:19:39 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 59918C433D2; Fri, 24 Feb 2023 14:19:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1677248378; bh=5Dip4T8i/Ae6EeytUGwMelRYWHubcfbMstd13xVi0Ow=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=z+F73CiWjvJ2iS0cNIYbdsOpTO+NEbcSKCCnL7Sb30dNuDHVDS4mVXDcHw2PYVEh5 k1FHw50uaIbs5kvP+/8oZI+hDI2oyur0+GnuJtb/QeiesAT0Q+x55HU/MKouE8v8Y0 k6Y8dY34wJ4UWsOePj7T6L7Tp2Urb2eO7zHONlxA= Date: Fri, 24 Feb 2023 15:19:36 +0100 From: Greg Kroah-Hartman To: Asahi Lina Cc: Miguel Ojeda , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?iso-8859-1?Q?Bj=F6rn?= Roy Baron , Will Deacon , Robin Murphy , Joerg Roedel , Hector Martin , Sven Peter , Arnd Bergmann , "Rafael J. Wysocki" , Alyssa Rosenzweig , Neal Gompa , rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, asahi@lists.linux.dev Subject: Re: [PATCH 2/5] rust: device: Add a minimal RawDevice trait Message-ID: References: <20230224-rust-iopt-rtkit-v1-0-49ced3391295@asahilina.net> <20230224-rust-iopt-rtkit-v1-2-49ced3391295@asahilina.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 24, 2023 at 03:11:05PM +0100, Greg Kroah-Hartman wrote: > Thanks for the detailed rust explainations, I'd like to just highlight > one thing: > > On Fri, Feb 24, 2023 at 10:15:12PM +0900, Asahi Lina wrote: > > On 24/02/2023 20.23, Greg Kroah-Hartman wrote: > > > And again, why are bindings needed for a "raw" struct device at all? > > > Shouldn't the bus-specific wrappings work better? > > > > Because lots of kernel subsystems need to be able to accept "any" device > > and don't care about the bus! That's what this is for. > > That's great, but: > > > All the bus > > wrappers would implement this so they can be used as an argument for all > > those subsystems (plus a generic one when you just need to pass around > > an actual owned generic reference and no longer need bus-specific > > operations - you can materialize that out of a RawDevice impl, which is > > when get_device() would be called). That's why I'm introducing this now, > > because both io_pgtable and rtkit need to take `struct device` pointers > > on the C side so we need some "generic struct device" view on the Rust side. > > In looking at both ftkit and io_pgtable, those seem to be good examples > of how "not to use a struct device", so trying to make safe bindings > from Rust to these frameworks is very ironic :) > > rtkit takes a struct device pointer and then never increments it, > despite saving it off, which is unsafe. It then only uses it to print > out messages if things go wrong (or right in some cases), which is odd. > So it can get away from using a device pointer entirely, except for the > devm_apple_rtkit_init() call, which I doubt you want to call from rust > code, right? > > for io_pgtable, that's a bit messier, you want to pass in a device that > io_pgtable treats as a "device" but again, it is NEVER properly > reference counted, AND, it is only needed to try to figure out the bus > operations that dma memory should be allocated from for this device. So > what would be better to save off there would be a pointer to the bus, > which is constant and soon will be read-only so there are no lifetime > rules needed at all (see the major struct bus_type changes going into > 6.3-rc1 that will enable that to happen). > > So the two subsystems you want to call from rust code don't properly > handle the reference count of the object you are going to pass into it, > and only need it for debugging and iommu stuff, which is really only the > bus that the device is on, not good examples to start out with :) > > Yeah, this is yack-shaving, sorry, but it's how we clean up core > subsystems for apis and implementations that are not really correct and > were not noticed at the time. > > Can we see some users of this code posted so I can see how struct device > is going to work in a rust driver? That's the thing I worry most about > the rust/C interaction here as we have two different ways of thinking > about reference counts from the two worlds and putting them together is > going to be "interesting", as can be seen here already. Also, where are you getting your 'struct device' from in the first place? What bus is createing it and giving it to your rust driver? thanks, greg k-h