| Message ID | 20231006094911.3305152-1-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Rust abstractions for network PHY drivers | expand |
On Fri, Oct 06, 2023 at 06:49:08PM +0900, FUJITA Tomonori wrote: > This patchset adds Rust abstractions for network PHY drivers. It > doesn't fully cover the C APIs for PHY drivers yet but I think that > it's already useful. I implement two PHY drivers (Asix AX88772A PHYs > and Realtek Generic FE-GE). Seems they work well with real hardware. One of the conventions for submitting patches for netdev is to include the tree in the Subject. [PATCH net-next v2 1/3] rust: core abstractions for network PHY drivers This is described here, along with other useful hits for working with netdev. https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html This tag helps patchworks decide which tree to apply your patches to and then run build tests on it: https://patchwork.kernel.org/project/netdevbpf/patch/20231006094911.3305152-4-fujita.tomonori@gmail.com/ I don't know if it made the wrong decision based on the missing tag, or it simply does not know what to do with Rust yet. There is also the question of how we merge this. Does it all come through netdev? Do we split the patches, the abstraction merged via rust and the rest via netdev? Is the Kconfig sufficient that if a tree only contains patches 2 and 3 it does not allow the driver to be enabled? Andrew
On Fri, 6 Oct 2023 14:54:43 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Fri, Oct 06, 2023 at 06:49:08PM +0900, FUJITA Tomonori wrote: >> This patchset adds Rust abstractions for network PHY drivers. It >> doesn't fully cover the C APIs for PHY drivers yet but I think that >> it's already useful. I implement two PHY drivers (Asix AX88772A PHYs >> and Realtek Generic FE-GE). Seems they work well with real hardware. > > One of the conventions for submitting patches for netdev is to include > the tree in the Subject. > > [PATCH net-next v2 1/3] rust: core abstractions for network PHY drivers > > This is described here, along with other useful hits for working with > netdev. > > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html > > This tag helps patchworks decide which tree to apply your patches to > and then run build tests on it: > > https://patchwork.kernel.org/project/netdevbpf/patch/20231006094911.3305152-4-fujita.tomonori@gmail.com/ > > I don't know if it made the wrong decision based on the missing tag, > or it simply does not know what to do with Rust yet. Thanks, I didn't know how tags and patchworks works. > There is also the question of how we merge this. Does it all come > through netdev? Do we split the patches, the abstraction merged via > rust and the rest via netdev? Is the Kconfig sufficient that if a tree > only contains patches 2 and 3 it does not allow the driver to be > enabled? A tree only that contains patches 2 and 3 allow the driver to be enabled, I think. The driver depends on CONFIG_RUST, which might doesn't have PHY bindings support (the first patch). So I think that merging the patchset through a single tree is easier; netdev or rust. Miguel, how do you prefer to merge the patchset?
> A tree only that contains patches 2 and 3 allow the driver to be > enabled, I think. The driver depends on CONFIG_RUST, which might > doesn't have PHY bindings support (the first patch). This is part of why i said there should be a Kconfig symbol CONFIG_RUST_PHYLIB_BINDING or similar. With only patches 2 and 3, that would not exists, and so you cannot enable the driver. Once all the patches meet up in linux-next, you have both parts, and you can enable it. > So I think that merging the patchset through a single tree is easier; > netdev or rust. > > Miguel, how do you prefer to merge the patchset? What are the merge conflicts looking like? What has happened in the past? Or is this the first driver to actually get this far towards being merged? Andrew
On Fri, Oct 6, 2023 at 10:47 AM Andrew Lunn <andrew@lunn.ch> wrote: > > So I think that merging the patchset through a single tree is easier; > > netdev or rust. > > > > Miguel, how do you prefer to merge the patchset? > > What are the merge conflicts looking like? What has happened in the > past? [...] Miguel has said before that if subsystems are comfortable bringing rust through their trees then they are welcome to do so, which helps get a better idea of how everything works together. If you prefer not to, it can come through rust-next with no problem. There are no serious conflicts on the rust side since there is no net module yet. I think that most new things will need to touch lib.rs and the binding helper just to register themselves, but those are trivial (e.g. same for wq updates coming [1]). > Or is this the first driver to actually get this far towards > being merged? > > Andrew I think that answer is yes :) at least for an actual leaf driver. Hence some of the build system rough edges. [1]: https://lore.kernel.org/rust-for-linux/20230828104807.1581592-1-aliceryhl@google.com/
Replying here to a missed followup on rfc v3 [1] On Mon, Oct 2, 2023 at 3:08 PM Andrew Lunn <andrew@lunn.ch> wrote: > The kernel is documented using kerneldoc. It would seem odd to me to > have a second parallel set of Documentation for Rust. Just like Rust > is integrated into the kernel tree, is configured using Kconfig, built > using make at the top level, i would also expect it to integrate into > kerneldoc somehow. I see the Rust API for PHY drivers next to the C > API for PHY drivers. Its just another API in the kernel, nothing > special. I just use 'make htmldocs' at the top level and out come the > HTML documentation in Documentation/output/ > > But kerneldoc is not my subsystem. MAINTAINERS say: > > DOCUMENTATION > M: Jonathan Corbet <corbet@lwn.net> > L: linux-doc@vger.kernel.org > S: Maintained > > So this discussion should really have Jonathon Corbet involved, if it > has not already been done. > > Andrew Having the documentation in the same place and able to easily crosslink is a goal, but it's still a work in progress. It won't look the same of course but I think that the rustdoc output will be under kerneldoc, with some sort of automated crosslinking between related modules. The docs team is in the loop, see [2] which was merged. (I suppose it must not be getting published still) - Trevor [1]: https://lore.kernel.org/rust-for-linux/78da96fc-cf66-4645-a98f-80e404800d3e@lunn.ch/ [2]: https://lore.kernel.org/rust-for-linux/20230718151534.4067460-1-carlos.bilbao@amd.com/
On Fri, 6 Oct 2023 19:37:15 -0400 Trevor Gross <tmgross@umich.edu> wrote: > On Fri, Oct 6, 2023 at 10:47 AM Andrew Lunn <andrew@lunn.ch> wrote: >> > So I think that merging the patchset through a single tree is easier; >> > netdev or rust. >> > >> > Miguel, how do you prefer to merge the patchset? >> >> What are the merge conflicts looking like? What has happened in the >> past? [...] > > Miguel has said before that if subsystems are comfortable bringing > rust through their trees then they are welcome to do so, which helps > get a better idea of how everything works together. If you prefer not > to, it can come through rust-next with no problem. It makes sense because these bindings are maintained by subsystems. I'll send patches with 'net-next' tag in the next round.
On Fri, Oct 6, 2023 at 2:54 PM Andrew Lunn <andrew@lunn.ch> wrote: > > This is described here, along with other useful hits for working with > netdev. > > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html Off-topic suggestion: the `.rst` file could be linked as the `P:` field in `MAINTAINERS`, perhaps with some tweaks. > I don't know if it made the wrong decision based on the missing tag, > or it simply does not know what to do with Rust yet. How does it usually determine the tree otherwise (if the tree is not in the subject)? > There is also the question of how we merge this. Does it all come > through netdev? Do we split the patches, the abstraction merged via > rust and the rest via netdev? Is the Kconfig sufficient that if a tree > only contains patches 2 and 3 it does not allow the driver to be > enabled? Ideally, everything goes through the subsystem's tree whenever they feel ready to do so. The idea is that maintainers get involved and handle their Rust code as any other code. Trees like Kbuild, KUnit and Workqueue have started taking things, for instance, which is great (and we appreciate it). Having said that, I would recommend caution -- I would wait until a few more people from the Rust subsystem give their `Reviewed-by`. In particular, I would wait until Wedson has given it another look at least, since he has had the most experience developing networking abstractions. I mention this because what we are trying to achieve with the Rust abstractions is not just functional equivalence to the C side, but also to make them "sound". In Rust, "sound" means the safe APIs cannot possibly introduce UB on their own. Moreover, as I said elsewhere, I do not agree with the `--rustified-enum` change in the series, given the UB risk (see the previous paragraph). If we really want to go with that, then we should be very explicit about the fact that we are placing an assumption on the C side here. Cheers, Miguel