Message ID | 20231210234924.1453917-2-fujita.tomonori@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | Rust abstractions for network PHY drivers | expand |
On Mon, Dec 11, 2023 at 08:49:21AM +0900, FUJITA Tomonori wrote: > This patch adds abstractions to implement network PHY drivers; the > driver registration and bindings for some of callback functions in > struct phy_driver and many genphy_ functions. > > This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y. > > This patch enables unstable const_maybe_uninit_zeroed feature for > kernel crate to enable unsafe code to handle a constant value with > uninitialized data. With the feature, the abstractions can initialize > a phy_driver structure with zero easily; instead of initializing all > the members by hand. It's supposed to be stable in the not so distant > future. > > Link: https://github.com/rust-lang/rust/pull/116218 > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 12/11/23 00:49, FUJITA Tomonori wrote: > This patch adds abstractions to implement network PHY drivers; the > driver registration and bindings for some of callback functions in > struct phy_driver and many genphy_ functions. > > This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y. > > This patch enables unstable const_maybe_uninit_zeroed feature for > kernel crate to enable unsafe code to handle a constant value with > uninitialized data. With the feature, the abstractions can initialize > a phy_driver structure with zero easily; instead of initializing all > the members by hand. It's supposed to be stable in the not so distant > future. > > Link: https://github.com/rust-lang/rust/pull/116218 > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Overall looks fine to me. Just a few comments below that confuse me: > + /// Gets the state of PHY state machine states. > + pub fn state(&self) -> DeviceState { > + let phydev = self.0.get(); > + // SAFETY: The struct invariant ensures that we may access > + // this field without additional synchronization. > + let state = unsafe { (*phydev).state }; > + // TODO: this conversion code will be replaced with automatically generated code by bindgen > + // when it becomes possible. > + // better to call WARN_ONCE() when the state is out-of-range. Did you mix up two comments here? This doesn't parse in my brain. > + /// Reads a given C22 PHY register. > + // This function reads a hardware register and updates the stats so takes `&mut self`. > + pub fn read(&mut self, regnum: u16) -> Result<u16> { > + let phydev = self.0.get(); > + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > + // So an FFI call with a valid pointer. This sentence also doesn't parse in my brain. Perhaps "So it's just an FFI call" or similar? Alice
On Mon, 11 Dec 2023 22:46:01 +0100 Alice Ryhl <alice@ryhl.io> wrote: > On 12/11/23 00:49, FUJITA Tomonori wrote: >> This patch adds abstractions to implement network PHY drivers; the >> driver registration and bindings for some of callback functions in >> struct phy_driver and many genphy_ functions. >> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y. >> This patch enables unstable const_maybe_uninit_zeroed feature for >> kernel crate to enable unsafe code to handle a constant value with >> uninitialized data. With the feature, the abstractions can initialize >> a phy_driver structure with zero easily; instead of initializing all >> the members by hand. It's supposed to be stable in the not so distant >> future. >> Link: https://github.com/rust-lang/rust/pull/116218 >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > Overall looks fine to me. Just a few comments below that confuse me: Thanks. >> + /// Gets the state of PHY state machine states. >> + pub fn state(&self) -> DeviceState { >> + let phydev = self.0.get(); >> + // SAFETY: The struct invariant ensures that we may access >> + // this field without additional synchronization. >> + let state = unsafe { (*phydev).state }; >> + // TODO: this conversion code will be replaced with automatically >> generated code by bindgen >> + // when it becomes possible. >> + // better to call WARN_ONCE() when the state is out-of-range. > > Did you mix up two comments here? This doesn't parse in my brain. I'll remove the second comment because all we have to do here is using bindgen. >> + /// Reads a given C22 PHY register. >> + // This function reads a hardware register and updates the stats so >> takes `&mut self`. >> + pub fn read(&mut self, regnum: u16) -> Result<u16> { >> + let phydev = self.0.get(); >> + // SAFETY: `phydev` is pointing to a valid object by the type >> invariant of `Self`. >> + // So an FFI call with a valid pointer. > > This sentence also doesn't parse in my brain. Perhaps "So it's just an > FFI call" or similar? "So it's just an FFI call" looks good. I'll fix all the places that use the same comment.
On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote: [...] > >> + /// Reads a given C22 PHY register. > >> + // This function reads a hardware register and updates the stats so > >> takes `&mut self`. > >> + pub fn read(&mut self, regnum: u16) -> Result<u16> { > >> + let phydev = self.0.get(); > >> + // SAFETY: `phydev` is pointing to a valid object by the type > >> invariant of `Self`. > >> + // So an FFI call with a valid pointer. > > > > This sentence also doesn't parse in my brain. Perhaps "So it's just an > > FFI call" or similar? > > "So it's just an FFI call" looks good. I'll fix all the places that > use the same comment. I would also mention that `(*phydev).mdio.addr` is smaller than PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise mdiobus_read() would cause out-of-bound accesses at ->stats. The safety comments are supposed to describe why calling the C function won't cause memory safety issues.. Regards, Boqun
On Mon, 11 Dec 2023 15:40:33 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote: > [...] >> >> + /// Reads a given C22 PHY register. >> >> + // This function reads a hardware register and updates the stats so >> >> takes `&mut self`. >> >> + pub fn read(&mut self, regnum: u16) -> Result<u16> { >> >> + let phydev = self.0.get(); >> >> + // SAFETY: `phydev` is pointing to a valid object by the type >> >> invariant of `Self`. >> >> + // So an FFI call with a valid pointer. >> > >> > This sentence also doesn't parse in my brain. Perhaps "So it's just an >> > FFI call" or similar? >> >> "So it's just an FFI call" looks good. I'll fix all the places that >> use the same comment. > > I would also mention that `(*phydev).mdio.addr` is smaller than > PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise > mdiobus_read() would cause out-of-bound accesses at ->stats. The safety > comments are supposed to describe why calling the C function won't cause > memory safety issues.. (*phydev).mdio.addr is managed in the C side and Rust code doesn't touch it (doesn't need to know anything about it). What safety comment should be written here?
On Tue, Dec 12, 2023 at 08:47:53AM +0900, FUJITA Tomonori wrote: > On Mon, 11 Dec 2023 15:40:33 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > > > On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote: > > [...] > >> >> + /// Reads a given C22 PHY register. > >> >> + // This function reads a hardware register and updates the stats so > >> >> takes `&mut self`. > >> >> + pub fn read(&mut self, regnum: u16) -> Result<u16> { > >> >> + let phydev = self.0.get(); > >> >> + // SAFETY: `phydev` is pointing to a valid object by the type > >> >> invariant of `Self`. > >> >> + // So an FFI call with a valid pointer. > >> > > >> > This sentence also doesn't parse in my brain. Perhaps "So it's just an > >> > FFI call" or similar? > >> > >> "So it's just an FFI call" looks good. I'll fix all the places that > >> use the same comment. > > > > I would also mention that `(*phydev).mdio.addr` is smaller than > > PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise > > mdiobus_read() would cause out-of-bound accesses at ->stats. The safety > > comments are supposed to describe why calling the C function won't cause > > memory safety issues.. > > (*phydev).mdio.addr is managed in the C side and Rust code doesn't It's OK to rely on C side to give a correct addr value. > touch it (doesn't need to know anything about it). What safety comment > should be written here? Basically, here Rust just does the same as C does in phy_read(), right? So why phy_read() is implemented correctly, because C side maintains the `(*phydev).mdio.addr` in that way. We ususally don't call it out in C code, since it's obvious(TM), and there is no safe/unsafe boundary in C side. But in Rust code, that matters. Yes, Rust doesn't control the value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other words, as long as C side holds the invariants, calling mdiobus_read() is safe here. How about // SAFETY: `phydev` points to valid object per the type invariant of // `Self`, also `(*phydev).mdio` is totally maintained by C in a way // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call // `mdiobus_read`. ? Regards, Boqun
On Mon, 11 Dec 2023 16:49:39 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: >> touch it (doesn't need to know anything about it). What safety comment >> should be written here? > > Basically, here Rust just does the same as C does in phy_read(), right? > So why phy_read() is implemented correctly, because C side maintains the > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C > code, since it's obvious(TM), and there is no safe/unsafe boundary in C > side. But in Rust code, that matters. Yes, Rust doesn't control the > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other > words, as long as C side holds the invariants, calling mdiobus_read() is > safe here. How about > > // SAFETY: `phydev` points to valid object per the type invariant of > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call > // `mdiobus_read`. I thought that "`phydev` is pointing to a valid object by the type invariant of `Self`." comment implies that "C side holds the invariants" Do we need a comment about the C implementation details like PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C side because the C code is changed any time.
On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote: > On Mon, 11 Dec 2023 16:49:39 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > > >> touch it (doesn't need to know anything about it). What safety comment > >> should be written here? > > > > Basically, here Rust just does the same as C does in phy_read(), right? > > So why phy_read() is implemented correctly, because C side maintains the > > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C > > code, since it's obvious(TM), and there is no safe/unsafe boundary in C > > side. But in Rust code, that matters. Yes, Rust doesn't control the > > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other > > words, as long as C side holds the invariants, calling mdiobus_read() is > > safe here. How about > > > > // SAFETY: `phydev` points to valid object per the type invariant of > > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way > > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and > > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call > > // `mdiobus_read`. > > I thought that "`phydev` is pointing to a valid object by the type > invariant of `Self`." comment implies that "C side holds the invariants" > By the type invariant of `Self`, you mean: /// # Invariants /// /// Referencing a `phy_device` using this struct asserts that you are in /// a context where all methods defined on this struct are safe to call. my read on that only tells me the context is guaranteed to be in a driver callback, nothing has been said about all other invariants C side should hold. > Do we need a comment about the C implementation details like > PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C > side because the C code is changed any time. Well, exactly, "the C code is changed any time", I thought having more information in Rust helps people who is going to change the C side to see whether they may break Rust side. Plus it's the safety comment, you need to prove that it's safe to call the function, the function is unsafe for a reason: there are inputs that may cause issues, and writing the safety comment is a process to think and double check. Maybe we can simplify this a little bit, since IIUC, you just want to call phy_read() here, but due to that Rust cannot call inline C functions directly, hence the open-code. How about: // SAFETY: `phydev` points to valid object per the type invariant of // `Self`, also the following just minics what `phy_read()` does in C // side, which should be safe as long as `phydev` is valid. ? Regards, Boqun
On Mon, 11 Dec 2023 18:30:36 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote: >> On Mon, 11 Dec 2023 16:49:39 -0800 >> Boqun Feng <boqun.feng@gmail.com> wrote: >> >> >> touch it (doesn't need to know anything about it). What safety comment >> >> should be written here? >> > >> > Basically, here Rust just does the same as C does in phy_read(), right? >> > So why phy_read() is implemented correctly, because C side maintains the >> > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C >> > code, since it's obvious(TM), and there is no safe/unsafe boundary in C >> > side. But in Rust code, that matters. Yes, Rust doesn't control the >> > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other >> > words, as long as C side holds the invariants, calling mdiobus_read() is >> > safe here. How about >> > >> > // SAFETY: `phydev` points to valid object per the type invariant of >> > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way >> > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and >> > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call >> > // `mdiobus_read`. >> >> I thought that "`phydev` is pointing to a valid object by the type >> invariant of `Self`." comment implies that "C side holds the invariants" >> > > By the type invariant of `Self`, you mean: > > /// # Invariants > /// > /// Referencing a `phy_device` using this struct asserts that you are in > /// a context where all methods defined on this struct are safe to call. > > my read on that only tells me the context is guaranteed to be in a > driver callback, nothing has been said about all other invariants C side > should hold. I meant that phydev points to a valid object, thus mdio and mdio.addr do too. But after reading you phy_read() comment, I suspect that you aren't talking about if mdio.addr points to valid memory or not. Your point is about the validity of calling mdiobus_read() with mdio.addr? >> Do we need a comment about the C implementation details like >> PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C >> side because the C code is changed any time. > > Well, exactly, "the C code is changed any time", I thought having more > information in Rust helps people who is going to change the C side to > see whether they may break Rust side. Plus it's the safety comment, you The C side people read the Rust code before changing the C code? Let's see. > need to prove that it's safe to call the function, the function is > unsafe for a reason: there are inputs that may cause issues, and writing > the safety comment is a process to think and double check. > > Maybe we can simplify this a little bit, since IIUC, you just want to > call phy_read() here, but due to that Rust cannot call inline C > functions directly, hence the open-code. How about: Yeah, I hope that the discussion about inline C functions would end with a solution. > // SAFETY: `phydev` points to valid object per the type invariant of > // `Self`, also the following just minics what `phy_read()` does in C > // side, which should be safe as long as `phydev` is valid. > > ? Looks ok to me but after a quick look at in-tree Rust code, I can't find a comment like X is valid for the first argument in this C function. What I found are comments like X points to valid memory.
On Tue, Dec 12, 2023 at 01:04:10PM +0900, FUJITA Tomonori wrote: > On Mon, 11 Dec 2023 18:30:36 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > > > On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote: > >> On Mon, 11 Dec 2023 16:49:39 -0800 > >> Boqun Feng <boqun.feng@gmail.com> wrote: > >> > >> >> touch it (doesn't need to know anything about it). What safety comment > >> >> should be written here? > >> > > >> > Basically, here Rust just does the same as C does in phy_read(), right? > >> > So why phy_read() is implemented correctly, because C side maintains the > >> > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C > >> > code, since it's obvious(TM), and there is no safe/unsafe boundary in C > >> > side. But in Rust code, that matters. Yes, Rust doesn't control the > >> > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other > >> > words, as long as C side holds the invariants, calling mdiobus_read() is > >> > safe here. How about > >> > > >> > // SAFETY: `phydev` points to valid object per the type invariant of > >> > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way > >> > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and > >> > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call > >> > // `mdiobus_read`. > >> > >> I thought that "`phydev` is pointing to a valid object by the type > >> invariant of `Self`." comment implies that "C side holds the invariants" > >> > > > > By the type invariant of `Self`, you mean: > > > > /// # Invariants > > /// > > /// Referencing a `phy_device` using this struct asserts that you are in > > /// a context where all methods defined on this struct are safe to call. > > > > my read on that only tells me the context is guaranteed to be in a > > driver callback, nothing has been said about all other invariants C side > > should hold. > > I meant that phydev points to a valid object, thus mdio and mdio.addr > do too. But after reading you phy_read() comment, I suspect that you > aren't talking about if mdio.addr points to valid memory or not. Your > point is about the validity of calling mdiobus_read() with mdio.addr? > Yes, I was talking about the safety comment for calling mdiobus_read(). > > >> Do we need a comment about the C implementation details like > >> PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C > >> side because the C code is changed any time. > > > > Well, exactly, "the C code is changed any time", I thought having more > > information in Rust helps people who is going to change the C side to > > see whether they may break Rust side. Plus it's the safety comment, you > > The C side people read the Rust code before changing the C code? Let's > see. > Hmm... I usually won't call someone "C side people". I mean, the project has C part and Rust part, but the community is one. In case of myself, I write both C and Rust, if I'm going to change some C side function, I may want to see the usage at Rust side, especially whether my changes could break the safety, and safety comments may be important. > > > need to prove that it's safe to call the function, the function is > > unsafe for a reason: there are inputs that may cause issues, and writing > > the safety comment is a process to think and double check. > > > > Maybe we can simplify this a little bit, since IIUC, you just want to > > call phy_read() here, but due to that Rust cannot call inline C > > functions directly, hence the open-code. How about: > > Yeah, I hope that the discussion about inline C functions would end > with a solution. > > > > // SAFETY: `phydev` points to valid object per the type invariant of > > // `Self`, also the following just minics what `phy_read()` does in C > > // side, which should be safe as long as `phydev` is valid. > > > > ? > > Looks ok to me but after a quick look at in-tree Rust code, I can't > find a comment like X is valid for the first argument in this C > function. What I found are comments like X points to valid memory. Hmm.. maybe "is valid" could be a confusing term, so the point is: if `phydev` is pointing to a properly maintained struct phy_device, then an open code of phy_read() should be safe. Maybe "..., which should be safe as long as `phydev` points to a valid struct phy_device" ? Regards, Boqun
On Tue, Dec 12, 2023 at 12:15 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Mon, 11 Dec 2023 22:46:01 +0100 > Alice Ryhl <alice@ryhl.io> wrote: > >> + /// Gets the state of PHY state machine states. > >> + pub fn state(&self) -> DeviceState { > >> + let phydev = self.0.get(); > >> + // SAFETY: The struct invariant ensures that we may access > >> + // this field without additional synchronization. > >> + let state = unsafe { (*phydev).state }; > >> + // TODO: this conversion code will be replaced with automatically > >> generated code by bindgen > >> + // when it becomes possible. > >> + // better to call WARN_ONCE() when the state is out-of-range. > > > > Did you mix up two comments here? This doesn't parse in my brain. > > I'll remove the second comment because all we have to do here is using > bindgen. > > > >> + /// Reads a given C22 PHY register. > >> + // This function reads a hardware register and updates the stats so > >> takes `&mut self`. > >> + pub fn read(&mut self, regnum: u16) -> Result<u16> { > >> + let phydev = self.0.get(); > >> + // SAFETY: `phydev` is pointing to a valid object by the type > >> invariant of `Self`. > >> + // So an FFI call with a valid pointer. > > > > This sentence also doesn't parse in my brain. Perhaps "So it's just an > > FFI call" or similar? > > "So it's just an FFI call" looks good. I'll fix all the places that > use the same comment. If you make those two comment changes, then you can add Reviewed-by: Alice Ryhl <aliceryhl@google.com>
On Tue, 12 Dec 2023 10:23:30 +0100 Alice Ryhl <aliceryhl@google.com> wrote: > On Tue, Dec 12, 2023 at 12:15 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> On Mon, 11 Dec 2023 22:46:01 +0100 >> Alice Ryhl <alice@ryhl.io> wrote: >> >> + /// Gets the state of PHY state machine states. >> >> + pub fn state(&self) -> DeviceState { >> >> + let phydev = self.0.get(); >> >> + // SAFETY: The struct invariant ensures that we may access >> >> + // this field without additional synchronization. >> >> + let state = unsafe { (*phydev).state }; >> >> + // TODO: this conversion code will be replaced with automatically >> >> generated code by bindgen >> >> + // when it becomes possible. >> >> + // better to call WARN_ONCE() when the state is out-of-range. >> > >> > Did you mix up two comments here? This doesn't parse in my brain. >> >> I'll remove the second comment because all we have to do here is using >> bindgen. >> >> >> >> + /// Reads a given C22 PHY register. >> >> + // This function reads a hardware register and updates the stats so >> >> takes `&mut self`. >> >> + pub fn read(&mut self, regnum: u16) -> Result<u16> { >> >> + let phydev = self.0.get(); >> >> + // SAFETY: `phydev` is pointing to a valid object by the type >> >> invariant of `Self`. >> >> + // So an FFI call with a valid pointer. >> > >> > This sentence also doesn't parse in my brain. Perhaps "So it's just an >> > FFI call" or similar? >> >> "So it's just an FFI call" looks good. I'll fix all the places that >> use the same comment. > > If you make those two comment changes, then you can add > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> I will, thanks!
On Tue, Dec 12, 2023 at 5:04 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > The C side people read the Rust code before changing the C code? Let's > see. In the beginning, it is true that developers may forget often. But it is still the case that they need to be mindful of the Rust side, just like when changing some C function you need to take care of changing its callers. That is also why we want to have maintainers of each subsystem onboard. In some cases it may be useful to annotate/mark the C side with a comment warning about it (or similar) if you think it is likely that a change on the C side will go unnoticed. Cheers, Miguel
On Mon, 11 Dec 2023 22:11:15 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: >> > // SAFETY: `phydev` points to valid object per the type invariant of >> > // `Self`, also the following just minics what `phy_read()` does in C >> > // side, which should be safe as long as `phydev` is valid. >> > >> > ? >> >> Looks ok to me but after a quick look at in-tree Rust code, I can't >> find a comment like X is valid for the first argument in this C >> function. What I found are comments like X points to valid memory. > > Hmm.. maybe "is valid" could be a confusing term, so the point is: if > `phydev` is pointing to a properly maintained struct phy_device, then an > open code of phy_read() should be safe. Maybe "..., which should be safe > as long as `phydev` points to a valid struct phy_device" ? As Alice suggested, I updated the comment. The current comment is: // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. // So it's just an FFI call. let ret = unsafe { bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into()) }; If phy_read() is called here, I assume that you are happy about the above comment. The way to call mdiobus_read() here is safe because it just an open code of phy_read(). Simply adding it works for you? // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. // So it's just an FFI call, open code of `phy_read()`.
On 12/12/23 14:02, FUJITA Tomonori wrote: > On Mon, 11 Dec 2023 22:11:15 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > >>>> // SAFETY: `phydev` points to valid object per the type invariant of >>>> // `Self`, also the following just minics what `phy_read()` does in C >>>> // side, which should be safe as long as `phydev` is valid. >>>> >>>> ? >>> >>> Looks ok to me but after a quick look at in-tree Rust code, I can't >>> find a comment like X is valid for the first argument in this C >>> function. What I found are comments like X points to valid memory. >> >> Hmm.. maybe "is valid" could be a confusing term, so the point is: if >> `phydev` is pointing to a properly maintained struct phy_device, then an >> open code of phy_read() should be safe. Maybe "..., which should be safe >> as long as `phydev` points to a valid struct phy_device" ? > > As Alice suggested, I updated the comment. The current comment is: > > // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > // So it's just an FFI call. > let ret = unsafe { > bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into()) > }; I still think you need to justify why `mdio.bus` is a pointer that you can give to `midobus_read`. After looking at the C code, it seems like that the pointer needs to point to a valid `struct mii_bus`. This *could* just be an invariant of `struct phy_device` [1], but where do we document that? We could make an exception here and treat this differently until bindgen can handle the `static inline` functions, but I am not so sure if we want to have this as a general pattern. We need to discuss this more. [1]: Technically it is a combination of the following invariants: - the `mdio` field of `struct phy_device` is a valid `struct mido_device` - the `bus` field of `struct mdio_device` is a valid pointer to a valid `struct mii_bus`. > If phy_read() is called here, I assume that you are happy about the > above comment. The way to call mdiobus_read() here is safe because it > just an open code of phy_read(). Simply adding it works for you? > > // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > // So it's just an FFI call, open code of `phy_read()`. This would be fine if we decide to go with the exception I detailed above. Although instead of "open code" I would write "see implementation of `phy_read()`".
On Tue, Dec 12, 2023 at 05:35:34PM +0000, Benno Lossin wrote: > On 12/12/23 14:02, FUJITA Tomonori wrote: > > On Mon, 11 Dec 2023 22:11:15 -0800 > > Boqun Feng <boqun.feng@gmail.com> wrote: > > > >>>> // SAFETY: `phydev` points to valid object per the type invariant of > >>>> // `Self`, also the following just minics what `phy_read()` does in C > >>>> // side, which should be safe as long as `phydev` is valid. > >>>> > >>>> ? > >>> > >>> Looks ok to me but after a quick look at in-tree Rust code, I can't > >>> find a comment like X is valid for the first argument in this C > >>> function. What I found are comments like X points to valid memory. > >> > >> Hmm.. maybe "is valid" could be a confusing term, so the point is: if > >> `phydev` is pointing to a properly maintained struct phy_device, then an > >> open code of phy_read() should be safe. Maybe "..., which should be safe > >> as long as `phydev` points to a valid struct phy_device" ? > > > > As Alice suggested, I updated the comment. The current comment is: > > > > // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > > // So it's just an FFI call. > > let ret = unsafe { > > bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into()) > > }; > > I still think you need to justify why `mdio.bus` is a pointer that you > can give to `midobus_read`. After looking at the C code, it seems like > that the pointer needs to point to a valid `struct mii_bus`. > This *could* just be an invariant of `struct phy_device` [1], but where > do we document that? Yeah, it's better if we call it out in the type invariant. > > We could make an exception here and treat this differently until bindgen > can handle the `static inline` functions, but I am not so sure if we > want to have this as a general pattern. We need to discuss this more. > Agreed, here my latest suggestion was definitely a workaround. > > [1]: Technically it is a combination of the following invariants: > - the `mdio` field of `struct phy_device` is a valid `struct mido_device` > - the `bus` field of `struct mdio_device` is a valid pointer to a valid > `struct mii_bus`. > > > If phy_read() is called here, I assume that you are happy about the > > above comment. The way to call mdiobus_read() here is safe because it > > just an open code of phy_read(). Simply adding it works for you? > > > > // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. > > // So it's just an FFI call, open code of `phy_read()`. > > This would be fine if we decide to go with the exception I detailed > above. Although instead of "open code" I would write "see implementation > of `phy_read()`". > So the rationale here is the callsite of mdiobus_read() is just a open-code version of phy_read(), so if we meet the same requirement of phy_read(), we should be safe here. Maybe: "... open code of `phy_read()` with a valid phy_device pointer `phydev`" ? Regards, Boqun > -- > Cheers, > Benno >
On 12/12/23 21:23, Boqun Feng wrote: > On Tue, Dec 12, 2023 at 05:35:34PM +0000, Benno Lossin wrote: >> [1]: Technically it is a combination of the following invariants: >> - the `mdio` field of `struct phy_device` is a valid `struct mido_device` >> - the `bus` field of `struct mdio_device` is a valid pointer to a valid >> `struct mii_bus`. >> >>> If phy_read() is called here, I assume that you are happy about the >>> above comment. The way to call mdiobus_read() here is safe because it >>> just an open code of phy_read(). Simply adding it works for you? >>> >>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >>> // So it's just an FFI call, open code of `phy_read()`. >> >> This would be fine if we decide to go with the exception I detailed >> above. Although instead of "open code" I would write "see implementation >> of `phy_read()`". >> > > So the rationale here is the callsite of mdiobus_read() is just a > open-code version of phy_read(), so if we meet the same requirement of > phy_read(), we should be safe here. Maybe: > > "... open code of `phy_read()` with a valid phy_device pointer > `phydev`" > > ? Hmm that might be OK if we add "TODO: replace this with `phy_read` once bindgen can handle static inline functions.". Actually, why can't we just use the normal `rust_helper_*` approach? So just create a `rust_helper_phy_read` that calls `phy_read`. Then call that from the rust side. Doing this means that we can just keep the invariants of `struct phy_device` opaque to the Rust side. That would probably be preferable to adding the `TODO`, since when bindgen has this feature available, we will automatically handle this and not forget it. Also we have no issue with diverging code.
On Tue, 12 Dec 2023 17:35:34 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 12/12/23 14:02, FUJITA Tomonori wrote: >> On Mon, 11 Dec 2023 22:11:15 -0800 >> Boqun Feng <boqun.feng@gmail.com> wrote: >> >>>>> // SAFETY: `phydev` points to valid object per the type invariant of >>>>> // `Self`, also the following just minics what `phy_read()` does in C >>>>> // side, which should be safe as long as `phydev` is valid. >>>>> >>>>> ? >>>> >>>> Looks ok to me but after a quick look at in-tree Rust code, I can't >>>> find a comment like X is valid for the first argument in this C >>>> function. What I found are comments like X points to valid memory. >>> >>> Hmm.. maybe "is valid" could be a confusing term, so the point is: if >>> `phydev` is pointing to a properly maintained struct phy_device, then an >>> open code of phy_read() should be safe. Maybe "..., which should be safe >>> as long as `phydev` points to a valid struct phy_device" ? >> >> As Alice suggested, I updated the comment. The current comment is: >> >> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> // So it's just an FFI call. >> let ret = unsafe { >> bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into()) >> }; > > I still think you need to justify why `mdio.bus` is a pointer that you > can give to `midobus_read`. After looking at the C code, it seems like > that the pointer needs to point to a valid `struct mii_bus`. > This *could* just be an invariant of `struct phy_device` [1], but where > do we document that? If phy_device points to a valid object, phy_device.mdio is valid. A mii_bus must exist before a phy device. A bus is scanned and then a phy device is found (so phy_device object is crated). https://elixir.bootlin.com/linux/v6.6.5/source/drivers/net/phy/phy_device.c#L634
On 12/13/23 00:01, FUJITA Tomonori wrote: > On Tue, 12 Dec 2023 17:35:34 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >> On 12/12/23 14:02, FUJITA Tomonori wrote: >>> On Mon, 11 Dec 2023 22:11:15 -0800 >>> Boqun Feng <boqun.feng@gmail.com> wrote: >>> >>>>>> // SAFETY: `phydev` points to valid object per the type invariant of >>>>>> // `Self`, also the following just minics what `phy_read()` does in C >>>>>> // side, which should be safe as long as `phydev` is valid. >>>>>> >>>>>> ? >>>>> >>>>> Looks ok to me but after a quick look at in-tree Rust code, I can't >>>>> find a comment like X is valid for the first argument in this C >>>>> function. What I found are comments like X points to valid memory. >>>> >>>> Hmm.. maybe "is valid" could be a confusing term, so the point is: if >>>> `phydev` is pointing to a properly maintained struct phy_device, then an >>>> open code of phy_read() should be safe. Maybe "..., which should be safe >>>> as long as `phydev` points to a valid struct phy_device" ? >>> >>> As Alice suggested, I updated the comment. The current comment is: >>> >>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >>> // So it's just an FFI call. >>> let ret = unsafe { >>> bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into()) >>> }; >> >> I still think you need to justify why `mdio.bus` is a pointer that you >> can give to `midobus_read`. After looking at the C code, it seems like >> that the pointer needs to point to a valid `struct mii_bus`. >> This *could* just be an invariant of `struct phy_device` [1], but where >> do we document that? > > If phy_device points to a valid object, phy_device.mdio is valid. Where is this documented? > A mii_bus must exist before a phy device. A bus is scanned and then a > phy device is found (so phy_device object is crated). > > https://elixir.bootlin.com/linux/v6.6.5/source/drivers/net/phy/phy_device.c#L634 I know that this is the status. But for this to be useful as a justification it must be written down somewhere where you can expect to find it. Not some knowledge that "everyone just knows". I would prefer to use the solution that I detailed in the other thread.
On Tue, 12 Dec 2023 22:40:01 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 12/12/23 21:23, Boqun Feng wrote: >> On Tue, Dec 12, 2023 at 05:35:34PM +0000, Benno Lossin wrote: >>> [1]: Technically it is a combination of the following invariants: >>> - the `mdio` field of `struct phy_device` is a valid `struct mido_device` >>> - the `bus` field of `struct mdio_device` is a valid pointer to a valid >>> `struct mii_bus`. >>> >>>> If phy_read() is called here, I assume that you are happy about the >>>> above comment. The way to call mdiobus_read() here is safe because it >>>> just an open code of phy_read(). Simply adding it works for you? >>>> >>>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >>>> // So it's just an FFI call, open code of `phy_read()`. >>> >>> This would be fine if we decide to go with the exception I detailed >>> above. Although instead of "open code" I would write "see implementation >>> of `phy_read()`". >>> >> >> So the rationale here is the callsite of mdiobus_read() is just a >> open-code version of phy_read(), so if we meet the same requirement of >> phy_read(), we should be safe here. Maybe: >> >> "... open code of `phy_read()` with a valid phy_device pointer >> `phydev`" >> >> ? > > Hmm that might be OK if we add "TODO: replace this with `phy_read` once > bindgen can handle static inline functions.". That's the conclusion? I suppose that a way to handle static inline functions is still under discussion. > Actually, why can't we just use the normal `rust_helper_*` approach? So > just create a `rust_helper_phy_read` that calls `phy_read`. Then call > that from the rust side. Doing this means that we can just keep the > invariants of `struct phy_device` opaque to the Rust side. > That would probably be preferable to adding the `TODO`, since when > bindgen has this feature available, we will automatically handle this > and not forget it. Also we have no issue with diverging code. I wasn't sure that `rust_helper_*` approach is the way to go.
On Tue, 12 Dec 2023 12:23:04 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > So the rationale here is the callsite of mdiobus_read() is just a > open-code version of phy_read(), so if we meet the same requirement of > phy_read(), we should be safe here. Maybe: > > "... open code of `phy_read()` with a valid phy_device pointer > `phydev`" > > ? I'll add the above comment with the similar for phy_write(), thanks!
On 13.12.23 00:31, FUJITA Tomonori wrote: > On Tue, 12 Dec 2023 12:23:04 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > >> So the rationale here is the callsite of mdiobus_read() is just a >> open-code version of phy_read(), so if we meet the same requirement of >> phy_read(), we should be safe here. Maybe: >> >> "... open code of `phy_read()` with a valid phy_device pointer >> `phydev`" >> >> ? > > I'll add the above comment with the similar for phy_write(), thanks! Why don't you use the `rust_helper` approach?
On 13.12.23 00:27, FUJITA Tomonori wrote: > On Tue, 12 Dec 2023 22:40:01 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: >> Actually, why can't we just use the normal `rust_helper_*` approach? So >> just create a `rust_helper_phy_read` that calls `phy_read`. Then call >> that from the rust side. Doing this means that we can just keep the >> invariants of `struct phy_device` opaque to the Rust side. >> That would probably be preferable to adding the `TODO`, since when >> bindgen has this feature available, we will automatically handle this >> and not forget it. Also we have no issue with diverging code. > > I wasn't sure that `rust_helper_*` approach is the way to go. It is the way to go to avoid code duplication and call those functions.
> > The C side people read the Rust code before changing the C code? Let's > > see. > > > > Hmm... I usually won't call someone "C side people". I mean, the project > has C part and Rust part, but the community is one. > > In case of myself, I write both C and Rust, if I'm going to change some > C side function, I may want to see the usage at Rust side, especially > whether my changes could break the safety, and safety comments may be > important. While i agree with your sentiment, ideally we want bilingual developers, in reality that is not going to happen for a long time. I could be wrong, but i expect developers to be either C developers, or Rust developers. They are existing kernel developers who know C, or Rust developers who are new to the kernel, and may not know much C. So we should try to keep that in mind. I personally don't think i have enough Rust knowledge to of even reached the dangerous stage. But at least the hard part with Rust seems to be the comments, not the actual code :-( Andrew
> I still think you need to justify why `mdio.bus` is a pointer that you > can give to `midobus_read`. Maybe a dumb question. Why are you limiting this to just a few members of struct phy_device? It has ~50 members, any of which can be used by the C side when Rust calls into C. Andrew
On 12/13/23 11:28, Andrew Lunn wrote: >> I still think you need to justify why `mdio.bus` is a pointer that you >> can give to `midobus_read`. > > Maybe a dumb question. Why are you limiting this to just a few members > of struct phy_device? It has ~50 members, any of which can be used by > the C side when Rust calls into C. I limited it to those few members, because the Rust side only uses those. Theoretically one could specify all invariants for all members, but that seems like a *lot* of work. In Rust everything [1] has to be initialized with a valid value. Contrast this with C where everything is potentially uninitialized. As an example, let's look at the first few fields of `PhyDevice`: struct PhyDevice { mdio: MdioDevice, drv: Box<PhyDriver>, devlink: Box<DeviceLink>, phy_id: u32, c45_ids: PhyC45DeviceIds, // ... } Note that in Rust we now do not need to write down any invariants, since they are all implicitly encoded. For example the `drv` field is of type `Box<PhyDriver>`, it *always* is a pointer to an allocation that contains a valid `PhyDriver`. So while on the C side you would have to state this, on the Rust side we get it for free. Rust function also make use of the "everything is in a valid state" rule, they know that the fields are valid and thus the Rust equivalent of `phy_read` could be safe and without any comments: impl PhyDevice { fn phy_read(&self, regnum: u32) -> i32 { self.mdio.bus.mdiobus_read(self.mdio.addr, regnum) } } All of this only applies to safe code, `unsafe` code is allowed to violate all of these things temporarily. However, when it gives a value back to safe code [2], that value needs to be valid. I think that specifying all of these implicit invariants in C will be extremely laborious. Especially since the usual way of doing things in C is not considering these invariants (at least not consciously), but rather "just do the thing". The way we do the interoperability is to just fully trust the C side to produce valid values that we can feed back to the C side. Of course there are caveats, so e.g. one needs to initialize a `struct mutex` before it can be used, but that is what we need to capture with the safety comments. [1]: There are exceptions for this, but for the purposes of this discussion, they can be ignored. If you want to know more, you can read this article in the nomicon: https://doc.rust-lang.org/nomicon/unchecked-uninit.html [2]: There are also exceptions, but I will omit them here.
On Wed, Dec 13, 2023 at 11:24:03AM +0100, Andrew Lunn wrote: > > > The C side people read the Rust code before changing the C code? Let's > > > see. > > > > > > > Hmm... I usually won't call someone "C side people". I mean, the project > > has C part and Rust part, but the community is one. > > > > In case of myself, I write both C and Rust, if I'm going to change some > > C side function, I may want to see the usage at Rust side, especially > > whether my changes could break the safety, and safety comments may be > > important. > > While i agree with your sentiment, ideally we want bilingual > developers, in reality that is not going to happen for a long time. I > could be wrong, but i expect developers to be either C developers, or > Rust developers. They are existing kernel developers who know C, or > Rust developers who are new to the kernel, and may not know much C. So Sorry, I cannot agree with you. Why do we try to divide the community in two parts? In fact, I keep telling people who want to contribute Rust-for-Linux that one way to contribute is trying to do some C code changes first to get familiar with the subsystem and kernel development. The sentence from Tomo really read like: I don't want to put this information here, since I don't think anyone would use it. Why do we want to shutdown the door for more people to collaborate, really, why? The only downside here is that Tomo needs to maintain a few more lines of comments. Also the comment is not a random comment, it's the safety comment, please see below.. > we should try to keep that in mind. > > I personally don't think i have enough Rust knowledge to of even > reached the dangerous stage. But at least the hard part with Rust > seems to be the comments, not the actual code :-( > Well, a safety comment is a basic part of Rust, which identifies the safe/unsafe boundary (i.e. where the code could go wrong in memory safety) and without that, the code will be just using Rust syntax and grammar. Honestly, if one doesn't try hard to identify the safe/unsafe boundaries, why do they try to use Rust? Unsafe Rust is harder to write than C, and safe Rust is pointless without a clear safe/unsafe boundary. Plus the syntax is not liked by anyone last time I heard ;-) Having a correct safety comment is really the bottom line. Without that, it's just bad Rust code, which I don't think netdev doesn't want either? Am I missing something here? Regards, Boqun > Andrew >
On Wed, Dec 13, 2023 at 08:43:09AM -0800, Boqun Feng wrote: > On Wed, Dec 13, 2023 at 11:24:03AM +0100, Andrew Lunn wrote: > > > > The C side people read the Rust code before changing the C code? Let's > > > > see. > > > > > > > > > > Hmm... I usually won't call someone "C side people". I mean, the project > > > has C part and Rust part, but the community is one. > > > > > > In case of myself, I write both C and Rust, if I'm going to change some > > > C side function, I may want to see the usage at Rust side, especially > > > whether my changes could break the safety, and safety comments may be > > > important. > > > > While i agree with your sentiment, ideally we want bilingual > > developers, in reality that is not going to happen for a long time. I > > could be wrong, but i expect developers to be either C developers, or > > Rust developers. They are existing kernel developers who know C, or > > Rust developers who are new to the kernel, and may not know much C. So > > Sorry, I cannot agree with you. Why do we try to divide the community in > two parts? In fact, I keep telling people who want to contribute > Rust-for-Linux that one way to contribute is trying to do some C code > changes first to get familiar with the subsystem and kernel development. > > The sentence from Tomo really read like: I don't want to put this > information here, since I don't think anyone would use it. Why do we > want to shutdown the door for more people to collaborate, really, why? > The only downside here is that Tomo needs to maintain a few more lines > of comments. Also the comment is not a random comment, it's the safety > comment, please see below.. > > > we should try to keep that in mind. > > > > I personally don't think i have enough Rust knowledge to of even > > reached the dangerous stage. But at least the hard part with Rust > > seems to be the comments, not the actual code :-( > > > > Well, a safety comment is a basic part of Rust, which identifies the > safe/unsafe boundary (i.e. where the code could go wrong in memory > safety) and without that, the code will be just using Rust syntax and > grammar. Honestly, if one doesn't try hard to identify the safe/unsafe > boundaries, why do they try to use Rust? Unsafe Rust is harder to write > than C, and safe Rust is pointless without a clear safe/unsafe boundary. > Plus the syntax is not liked by anyone last time I heard ;-) > > Having a correct safety comment is really the bottom line. Without that, > it's just bad Rust code, which I don't think netdev doesn't want either? s/doesn't// > Am I missing something here? > > Regards, > Boqun > > > Andrew > >
> Well, a safety comment is a basic part of Rust, which identifies the > safe/unsafe boundary (i.e. where the code could go wrong in memory > safety) and without that, the code will be just using Rust syntax and > grammar. Honestly, if one doesn't try hard to identify the safe/unsafe > boundaries, why do they try to use Rust? Unsafe Rust is harder to write > than C, and safe Rust is pointless without a clear safe/unsafe boundary. > Plus the syntax is not liked by anyone last time I heard ;-) Maybe comments are the wrong format for this? Maybe it should be a formal language? It could then be compiled into an executable form and tested? It won't show it is complete, but it would at least show it is correct/incorrect description of the assumptions. For normal builds it would not be included in the final binary, but maybe debug or formal verification builds it would be included? > Having a correct safety comment is really the bottom line. Without that, > it's just bad Rust code, which I don't think netdev doesn't want either? > Am I missing something here? It seems much easier to agree actual code is correct, maybe because it is a formal language, with a compiler, and a method to test it. Is that code really bad without the comments? It would be interesting to look back and see how much the actual code has changed because of these comments? I _think_ most of the review comments have resulted in changes to the comments, not the executable code itself. Does that mean it is much harder to write correct comments than correct code? Andrew
On 12/13/23 22:48, Andrew Lunn wrote: >> Well, a safety comment is a basic part of Rust, which identifies the >> safe/unsafe boundary (i.e. where the code could go wrong in memory >> safety) and without that, the code will be just using Rust syntax and >> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe >> boundaries, why do they try to use Rust? Unsafe Rust is harder to write >> than C, and safe Rust is pointless without a clear safe/unsafe boundary. >> Plus the syntax is not liked by anyone last time I heard ;-) > > Maybe comments are the wrong format for this? Maybe it should be a > formal language? It could then be compiled into an executable form and > tested? It won't show it is complete, but it would at least show it is > correct/incorrect description of the assumptions. For normal builds it > would not be included in the final binary, but maybe debug or formal > verification builds it would be included? That is an interesting suggestion, do you have any specific tools in mind? There are some Rust tools for formal verification, see https://rust-formal-methods.github.io/tools.html but I don't know if they can be used in the kernel, especially since we would need a tool that also supports C (I have no experience/knowledge of verification tools for C, so maybe you have something). Also my experience tells me that there are several issues with formal verification in practice: 1. When you want to use some formal system to prove something it is often an "all or nothing" game. So you will have to first verify everything that lies beneath you, or assume that it is correctly implemented. But assuming that everything is correctly implemented is rather dangerous, because if you base your formal system on classical logic [1], then a single contradiction allows you to prove everything. So in order for you to be _sure_ that it is correct, you need to work from the ground up. 2. There is no formal Rust memory model. So proving anything for interoperability between Rust and C is going to be challenging. 3. The burden of fully verifying a program is great. I know this, as I have some experience in this field. Now the programmer not only needs to know how to write a piece of code, but also how to write the required statements in the formal system and most importantly how to prove said statements from the axioms and theorems. When using safety comments, we avoid the problems of having to prove the statements formally (which is _very_ difficult). Of course people still need to know how to write safety comments, which is why I am working on a standard for safety comments. I hope to post an RFC in a couple weeks. It will also make the safety comments more formal by having a fixed set of phrases with exact interpretations, so there can be less room for misunderstandings. [2]: You might try to work around this by using a paraconsistent logic, but I have little to no experience with that field, so I cannot really say more than "it exists". >> Having a correct safety comment is really the bottom line. Without that, >> it's just bad Rust code, which I don't think netdev doesn't want either? >> Am I missing something here? > > It seems much easier to agree actual code is correct, maybe because it > is a formal language, with a compiler, and a method to test it. Is I disagree. You always have to consider the entire kernel when you want to determine if some piece of code is correct. This is by the nature of the formal language, anything can affect anything. For example you consider this: foo(void **ptr) { *ptr = NULL; synchronize_rcu() print(**x) } How do you know that this is correct? Well you have to look at all locations where `foo` is invoked and see if after a `synchronize_rcu` the supplied pointer is valid. If we do the safety comment stuff correctly, then we have a _local_ property, so something where you do not have to consider the entire kernel. Instead you assume that all other safety comments are correct and then only verify the boundary. If all boundaries agree, we have a reasonably correct program. > that code really bad without the comments? It would be interesting to > look back and see how much the actual code has changed because of > these comments? I _think_ most of the review comments have resulted in > changes to the comments, not the executable code itself. Does that > mean it is much harder to write correct comments than correct code? The code not having changed does not mean that it is correct. There are no obvious issues present, but can we really know that it is correct? Only time will tell (or a formal verification). The issue that we currently have with this patch series is that the people who know how the stuff works and the people who know how to write good safety comments are not the same. I hope that my safety standard will help close this gap. For example we do not know how the synchronization mechanism for `phy_suspend` and `phy_resume` work, but you mentioned in some previous thread that the knowledge is actually somewhere out there. It would help us a lot if you could give us a link or an explanation. Then we can work on a suitable safety comment.
On 12/14/23 00:40, Benno Lossin wrote: > On 12/13/23 22:48, Andrew Lunn wrote: >>> Well, a safety comment is a basic part of Rust, which identifies the >>> safe/unsafe boundary (i.e. where the code could go wrong in memory >>> safety) and without that, the code will be just using Rust syntax and >>> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe >>> boundaries, why do they try to use Rust? Unsafe Rust is harder to write >>> than C, and safe Rust is pointless without a clear safe/unsafe boundary. >>> Plus the syntax is not liked by anyone last time I heard ;-) >> >> Maybe comments are the wrong format for this? Maybe it should be a >> formal language? It could then be compiled into an executable form and >> tested? It won't show it is complete, but it would at least show it is >> correct/incorrect description of the assumptions. For normal builds it >> would not be included in the final binary, but maybe debug or formal >> verification builds it would be included? > > That is an interesting suggestion, do you have any specific tools in > mind? > There are some Rust tools for formal verification, see > https://rust-formal-methods.github.io/tools.html > but I don't know if they can be used in the kernel, especially since we > would need a tool that also supports C (I have no experience/knowledge > of verification tools for C, so maybe you have something). > Also my experience tells me that there are several issues with formal > verification in practice: Don't get me wrong, I would welcome a more formalized approach. I just have seen what that entails and I believe Rust (with safety comments) to be a good compromise that still allows programmers with no knowledge in formal systems to program and reasonable correctness. > 1. When you want to use some formal system to prove something it is > often an "all or nothing" game. So you will have to first verify > everything that lies beneath you, or assume that it is correctly > implemented. But assuming that everything is correctly implemented is > rather dangerous, because if you base your formal system on classical > logic [1], then a single contradiction allows you to prove > everything. So in order for you to be _sure_ that it is correct, you > need to work from the ground up. > > 2. There is no formal Rust memory model. So proving anything for > interoperability between Rust and C is going to be challenging. > > 3. The burden of fully verifying a program is great. I know this, as I > have some experience in this field. Now the programmer not only needs > to know how to write a piece of code, but also how to write the > required statements in the formal system and most importantly how to > prove said statements from the axioms and theorems. > > > When using safety comments, we avoid the problems of having to prove the > statements formally (which is _very_ difficult). Of course people still > need to know how to write safety comments, which is why I am working on > a standard for safety comments. I hope to post an RFC in a couple weeks. > It will also make the safety comments more formal by having a fixed > set of phrases with exact interpretations, so there can be less room for > misunderstandings. > > > [2]: You might try to work around this by using a paraconsistent logic, This should actually be [1]:. > but I have little to no experience with that field, so I cannot > really say more than "it exists".
On Wed, Dec 13, 2023 at 10:48:39PM +0100, Andrew Lunn wrote: > > Well, a safety comment is a basic part of Rust, which identifies the > > safe/unsafe boundary (i.e. where the code could go wrong in memory > > safety) and without that, the code will be just using Rust syntax and > > grammar. Honestly, if one doesn't try hard to identify the safe/unsafe > > boundaries, why do they try to use Rust? Unsafe Rust is harder to write > > than C, and safe Rust is pointless without a clear safe/unsafe boundary. > > Plus the syntax is not liked by anyone last time I heard ;-) > > Maybe comments are the wrong format for this? Maybe it should be a Maybe, but they are what we have right now. I do believe the unsafe <-> safe boundary in Rust needs better tooling: using comments may be a little bit abitrary. However, let's stick to what we have right now. Benno is actually working our Rust safety standard (feel free to correct me if I get the name wrong), hopefully that helps. > formal language? It could then be compiled into an executable form and > tested? It won't show it is complete, but it would at least show it is > correct/incorrect description of the assumptions. For normal builds it > would not be included in the final binary, but maybe debug or formal > verification builds it would be included? > Good idea, we actually want something similar when we were talking about Benno's safety standard, but as you may know a complete one would take years, or maybe impossbile. Also, kernel's safety requirement sometimes is weird and non-trivial, it's going to be a learning process ;-) A tool is certainly what we would like to look into if we have more experience in Rust abstraction and more time. > > Having a correct safety comment is really the bottom line. Without that, > > it's just bad Rust code, which I don't think netdev doesn't want either? > > Am I missing something here? > > It seems much easier to agree actual code is correct, maybe because it > is a formal language, with a compiler, and a method to test it. Is > that code really bad without the comments? It would be interesting to Note that most of the comment reviews are on *safety* comments (type invariants are related to safety comments). To me, in the review process, safety comments are similar to commit logs. In kernel patch reviews, we sometimes say "thanks for the explanation but please put that in the commit log", that is, even we know the code is correct and the patchset has been explained, we still want to see the explanation written down somewhere. Commit logs are ways to explain "why do I want to do this", and safety comments are ways to explain "why can I do the following safely". They are both ways to communicate during the review and for the future readers. So is a patchset really bad without commit logs? ;-) > look back and see how much the actual code has changed because of > these comments? I _think_ most of the review comments have resulted in I was trying to figure out the answer, but the patchset just has too many versions.. and discussions were splitted between versions... > changes to the comments, not the executable code itself. Does that > mean it is much harder to write correct comments than correct code? > If one can explain their code to others and make sure others understand the correctness of the code, then the code is very likely correct. So I think yes, writing correct comments is harder than correct code. I admit the review process took a bit long because it's a learning process for both developers and reviewers: being the first in-tree Rust driver may not be that easy, please understand the high standard expectation. Regards, Boqun > Andrew
On Wed, Dec 13, 2023 at 11:40:26PM +0000, Benno Lossin wrote: > On 12/13/23 22:48, Andrew Lunn wrote: > >> Well, a safety comment is a basic part of Rust, which identifies the > >> safe/unsafe boundary (i.e. where the code could go wrong in memory > >> safety) and without that, the code will be just using Rust syntax and > >> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe > >> boundaries, why do they try to use Rust? Unsafe Rust is harder to write > >> than C, and safe Rust is pointless without a clear safe/unsafe boundary. > >> Plus the syntax is not liked by anyone last time I heard ;-) > > > > Maybe comments are the wrong format for this? Maybe it should be a > > formal language? It could then be compiled into an executable form and > > tested? It won't show it is complete, but it would at least show it is > > correct/incorrect description of the assumptions. For normal builds it > > would not be included in the final binary, but maybe debug or formal > > verification builds it would be included? > > That is an interesting suggestion, do you have any specific tools in > mind? Sorry, no. I've no experience in this field at all. But given the discussions this patch has caused, simply a list of C or Rust expressions which evaluate to True when an assumption is correct would be a good start. We have said that we assume the phydev->lock is held. That is easy to express in code. We have said that phydev->mdio must be set, which is again easy to express. phydev->mdio.addr must be in the range 0..PHY_MAX_ADDR, etc. You probably cannot express all the safety requirements this way, but the set you can describe should be easy to understand and also unambiguous, since it is code. The rest still can be as comments. It would be easy to compile this code and insert it before the function on a verification build. Its only runtime checking, but its more functional than comments which the compiler just throws away. And maybe subsystems like this which are pretty much always slow path might even leave them enabled all the time, to act as a set of assert()s, which you sometimes see in code bases. Andrew
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 25cfc5ded1da..8d00cece5e23 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -60,6 +60,14 @@ config FIXED_PHY Currently tested with mpc866ads and mpc8349e-mitx. +config RUST_PHYLIB_ABSTRACTIONS + bool "Rust PHYLIB abstractions support" + depends on RUST + depends on PHYLIB=y + help + Adds support needed for PHY drivers written in Rust. It provides + a wrapper around the C phylib core. + config SFP tristate "SFP cage support" depends on I2C && PHYLINK diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 85f013ed4ca4..eaf01df7d97a 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -8,6 +8,9 @@ #include <kunit/test.h> #include <linux/errname.h> +#include <linux/ethtool.h> +#include <linux/mdio.h> +#include <linux/phy.h> #include <linux/slab.h> #include <linux/refcount.h> #include <linux/wait.h> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index e6aff80b521f..7ac39874aeac 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -14,6 +14,7 @@ #![no_std] #![feature(allocator_api)] #![feature(coerce_unsized)] +#![feature(const_maybe_uninit_zeroed)] #![feature(dispatch_from_dyn)] #![feature(new_uninit)] #![feature(offset_of)] @@ -38,6 +39,8 @@ pub mod ioctl; #[cfg(CONFIG_KUNIT)] pub mod kunit; +#[cfg(CONFIG_NET)] +pub mod net; pub mod prelude; pub mod print; mod static_assert; diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs new file mode 100644 index 000000000000..fe415cb369d3 --- /dev/null +++ b/rust/kernel/net.rs @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Networking. + +#[cfg(CONFIG_RUST_PHYLIB_ABSTRACTIONS)] +pub mod phy; diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs new file mode 100644 index 000000000000..5d220187eec9 --- /dev/null +++ b/rust/kernel/net/phy.rs @@ -0,0 +1,754 @@ +// SPDX-License-Identifier: GPL-2.0 + +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com> + +//! Network PHY device. +//! +//! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h). + +use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque}; + +use core::marker::PhantomData; + +/// PHY state machine states. +/// +/// Corresponds to the kernel's [`enum phy_state`]. +/// +/// Some of PHY drivers access to the state of PHY's software state machine. +/// +/// [`enum phy_state`]: ../../../../../../../include/linux/phy.h +#[derive(PartialEq, Eq)] +pub enum DeviceState { + /// PHY device and driver are not ready for anything. + Down, + /// PHY is ready to send and receive packets. + Ready, + /// PHY is up, but no polling or interrupts are done. + Halted, + /// PHY is up, but is in an error state. + Error, + /// PHY and attached device are ready to do work. + Up, + /// PHY is currently running. + Running, + /// PHY is up, but not currently plugged in. + NoLink, + /// PHY is performing a cable test. + CableTest, +} + +/// A mode of Ethernet communication. +/// +/// PHY drivers get duplex information from hardware and update the current state. +pub enum DuplexMode { + /// PHY is in full-duplex mode. + Full, + /// PHY is in half-duplex mode. + Half, + /// PHY is in unknown duplex mode. + Unknown, +} + +/// An instance of a PHY device. +/// +/// Wraps the kernel's [`struct phy_device`]. +/// +/// A [`Device`] instance is created when a callback in [`Driver`] is executed. A PHY driver +/// executes [`Driver`]'s methods during the callback. +/// +/// # Invariants +/// +/// Referencing a `phy_device` using this struct asserts that you are in +/// a context where all methods defined on this struct are safe to call. +/// +/// [`struct phy_device`]: ../../../../../../../include/linux/phy.h +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is +// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with +// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance. +// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access +// to the instance. +#[repr(transparent)] +pub struct Device(Opaque<bindings::phy_device>); + +impl Device { + /// Creates a new [`Device`] instance from a raw pointer. + /// + /// # Safety + /// + /// For the duration of 'a, the pointer must point at a valid `phy_device`, + /// and the caller must be in a context where all methods defined on this struct + /// are safe to call. + unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self { + // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`. + let ptr = ptr.cast::<Self>(); + // SAFETY: by the function requirements the pointer is valid and we have unique access for + // the duration of `'a`. + unsafe { &mut *ptr } + } + + /// Gets the id of the PHY. + pub fn phy_id(&self) -> u32 { + let phydev = self.0.get(); + // SAFETY: The struct invariant ensures that we may access + // this field without additional synchronization. + unsafe { (*phydev).phy_id } + } + + /// Gets the state of PHY state machine states. + pub fn state(&self) -> DeviceState { + let phydev = self.0.get(); + // SAFETY: The struct invariant ensures that we may access + // this field without additional synchronization. + let state = unsafe { (*phydev).state }; + // TODO: this conversion code will be replaced with automatically generated code by bindgen + // when it becomes possible. + // better to call WARN_ONCE() when the state is out-of-range. + match state { + bindings::phy_state_PHY_DOWN => DeviceState::Down, + bindings::phy_state_PHY_READY => DeviceState::Ready, + bindings::phy_state_PHY_HALTED => DeviceState::Halted, + bindings::phy_state_PHY_ERROR => DeviceState::Error, + bindings::phy_state_PHY_UP => DeviceState::Up, + bindings::phy_state_PHY_RUNNING => DeviceState::Running, + bindings::phy_state_PHY_NOLINK => DeviceState::NoLink, + bindings::phy_state_PHY_CABLETEST => DeviceState::CableTest, + _ => DeviceState::Error, + } + } + + /// Gets the current link state. + /// + /// It returns true if the link is up. + pub fn is_link_up(&self) -> bool { + const LINK_IS_UP: u64 = 1; + // TODO: the code to access to the bit field will be replaced with automatically + // generated code by bindgen when it becomes possible. + // SAFETY: The struct invariant ensures that we may access + // this field without additional synchronization. + let bit_field = unsafe { &(*self.0.get())._bitfield_1 }; + bit_field.get(14, 1) == LINK_IS_UP + } + + /// Gets the current auto-negotiation configuration. + /// + /// It returns true if auto-negotiation is enabled. + pub fn is_autoneg_enabled(&self) -> bool { + // TODO: the code to access to the bit field will be replaced with automatically + // generated code by bindgen when it becomes possible. + // SAFETY: The struct invariant ensures that we may access + // this field without additional synchronization. + let bit_field = unsafe { &(*self.0.get())._bitfield_1 }; + bit_field.get(13, 1) == bindings::AUTONEG_ENABLE as u64 + } + + /// Gets the current auto-negotiation state. + /// + /// It returns true if auto-negotiation is completed. + pub fn is_autoneg_completed(&self) -> bool { + const AUTONEG_COMPLETED: u64 = 1; + // TODO: the code to access to the bit field will be replaced with automatically + // generated code by bindgen when it becomes possible. + // SAFETY: The struct invariant ensures that we may access + // this field without additional synchronization. + let bit_field = unsafe { &(*self.0.get())._bitfield_1 }; + bit_field.get(15, 1) == AUTONEG_COMPLETED + } + + /// Sets the speed of the PHY. + pub fn set_speed(&mut self, speed: u32) { + let phydev = self.0.get(); + // SAFETY: The struct invariant ensures that we may access + // this field without additional synchronization. + unsafe { (*phydev).speed = speed as i32 }; + } + + /// Sets duplex mode. + pub fn set_duplex(&mut self, mode: DuplexMode) { + let phydev = self.0.get(); + let v = match mode { + DuplexMode::Full => bindings::DUPLEX_FULL as i32, + DuplexMode::Half => bindings::DUPLEX_HALF as i32, + DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32, + }; + // SAFETY: The struct invariant ensures that we may access + // this field without additional synchronization. + unsafe { (*phydev).duplex = v }; + } + + /// Reads a given C22 PHY register. + // This function reads a hardware register and updates the stats so takes `&mut self`. + pub fn read(&mut self, regnum: u16) -> Result<u16> { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + let ret = unsafe { + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into()) + }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Writes a given C22 PHY register. + pub fn write(&mut self, regnum: u16, val: u16) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { + bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val) + }) + } + + /// Reads a paged register. + pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Resolves the advertisements into PHY settings. + pub fn resolve_aneg_linkmode(&mut self) { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + unsafe { bindings::phy_resolve_aneg_linkmode(phydev) }; + } + + /// Executes software reset the PHY via `BMCR_RESET` bit. + pub fn genphy_soft_reset(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_soft_reset(phydev) }) + } + + /// Initializes the PHY. + pub fn init_hw(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // so an FFI call with a valid pointer. + to_result(unsafe { bindings::phy_init_hw(phydev) }) + } + + /// Starts auto-negotiation. + pub fn start_aneg(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::_phy_start_aneg(phydev) }) + } + + /// Resumes the PHY via `BMCR_PDOWN` bit. + pub fn genphy_resume(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_resume(phydev) }) + } + + /// Suspends the PHY via `BMCR_PDOWN` bit. + pub fn genphy_suspend(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_suspend(phydev) }) + } + + /// Checks the link status and updates current link state. + pub fn genphy_read_status(&mut self) -> Result<u16> { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + let ret = unsafe { bindings::genphy_read_status(phydev) }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Updates the link status. + pub fn genphy_update_link(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_update_link(phydev) }) + } + + /// Reads link partner ability. + pub fn genphy_read_lpa(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_read_lpa(phydev) }) + } + + /// Reads PHY abilities. + pub fn genphy_read_abilities(&mut self) -> Result { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + // So an FFI call with a valid pointer. + to_result(unsafe { bindings::genphy_read_abilities(phydev) }) + } +} + +/// Defines certain other features this PHY supports (like interrupts). +/// +/// These flag values are used in [`Driver::FLAGS`]. +pub mod flags { + /// PHY is internal. + pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL; + /// PHY needs to be reset after the refclk is enabled. + pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN; + /// Polling is used to detect PHY status changes. + pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST; + /// Don't suspend. + pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND; +} + +/// An adapter for the registration of a PHY driver. +struct Adapter<T: Driver> { + _p: PhantomData<T>, +} + +impl<T: Driver> Adapter<T> { + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn soft_reset_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + from_result(|| { + // SAFETY: This callback is called only in contexts + // where we hold `phy_device->lock`, so the accessors on + // `Device` are okay to call. + let dev = unsafe { Device::from_raw(phydev) }; + T::soft_reset(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn get_features_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + from_result(|| { + // SAFETY: This callback is called only in contexts + // where we hold `phy_device->lock`, so the accessors on + // `Device` are okay to call. + let dev = unsafe { Device::from_raw(phydev) }; + T::get_features(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { + from_result(|| { + // SAFETY: The C core code ensures that the accessors on + // `Device` are okay to call even though `phy_device->lock` + // might not be held. + let dev = unsafe { Device::from_raw(phydev) }; + T::suspend(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { + from_result(|| { + // SAFETY: The C core code ensures that the accessors on + // `Device` are okay to call even though `phy_device->lock` + // might not be held. + let dev = unsafe { Device::from_raw(phydev) }; + T::resume(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn config_aneg_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + from_result(|| { + // SAFETY: This callback is called only in contexts + // where we hold `phy_device->lock`, so the accessors on + // `Device` are okay to call. + let dev = unsafe { Device::from_raw(phydev) }; + T::config_aneg(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn read_status_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + from_result(|| { + // SAFETY: This callback is called only in contexts + // where we hold `phy_device->lock`, so the accessors on + // `Device` are okay to call. + let dev = unsafe { Device::from_raw(phydev) }; + T::read_status(dev)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn match_phy_device_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + // SAFETY: This callback is called only in contexts + // where we hold `phy_device->lock`, so the accessors on + // `Device` are okay to call. + let dev = unsafe { Device::from_raw(phydev) }; + T::match_phy_device(dev) as i32 + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn read_mmd_callback( + phydev: *mut bindings::phy_device, + devnum: i32, + regnum: u16, + ) -> i32 { + from_result(|| { + // SAFETY: This callback is called only in contexts + // where we hold `phy_device->lock`, so the accessors on + // `Device` are okay to call. + let dev = unsafe { Device::from_raw(phydev) }; + // CAST: the C side verifies devnum < 32. + let ret = T::read_mmd(dev, devnum as u8, regnum)?; + Ok(ret.into()) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn write_mmd_callback( + phydev: *mut bindings::phy_device, + devnum: i32, + regnum: u16, + val: u16, + ) -> i32 { + from_result(|| { + // SAFETY: This callback is called only in contexts + // where we hold `phy_device->lock`, so the accessors on + // `Device` are okay to call. + let dev = unsafe { Device::from_raw(phydev) }; + T::write_mmd(dev, devnum as u8, regnum, val)?; + Ok(0) + }) + } + + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) { + // SAFETY: This callback is called only in contexts + // where we hold `phy_device->lock`, so the accessors on + // `Device` are okay to call. + let dev = unsafe { Device::from_raw(phydev) }; + T::link_change_notify(dev); + } +} + +/// Driver structure for a particular PHY type. +/// +/// Wraps the kernel's [`struct phy_driver`]. +/// This is used to register a driver for a particular PHY type with the kernel. +/// +/// # Invariants +/// +/// `self.0` is always in a valid state. +/// +/// [`struct phy_driver`]: ../../../../../../../include/linux/phy.h +#[repr(transparent)] +pub struct DriverVTable(Opaque<bindings::phy_driver>); + +// SAFETY: `DriverVTable` has no &self methods, so immutable references to it +// are useless. +unsafe impl Sync for DriverVTable {} + +/// Creates a [`DriverVTable`] instance from [`Driver`]. +/// +/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`. +/// +/// [`module_phy_driver`]: crate::module_phy_driver +pub const fn create_phy_driver<T: Driver>() -> DriverVTable { + // INVARIANT: All the fields of `struct phy_driver` are initialized properly. + DriverVTable(Opaque::new(bindings::phy_driver { + name: T::NAME.as_char_ptr().cast_mut(), + flags: T::FLAGS, + phy_id: T::PHY_DEVICE_ID.id, + phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(), + soft_reset: if T::HAS_SOFT_RESET { + Some(Adapter::<T>::soft_reset_callback) + } else { + None + }, + get_features: if T::HAS_GET_FEATURES { + Some(Adapter::<T>::get_features_callback) + } else { + None + }, + match_phy_device: if T::HAS_MATCH_PHY_DEVICE { + Some(Adapter::<T>::match_phy_device_callback) + } else { + None + }, + suspend: if T::HAS_SUSPEND { + Some(Adapter::<T>::suspend_callback) + } else { + None + }, + resume: if T::HAS_RESUME { + Some(Adapter::<T>::resume_callback) + } else { + None + }, + config_aneg: if T::HAS_CONFIG_ANEG { + Some(Adapter::<T>::config_aneg_callback) + } else { + None + }, + read_status: if T::HAS_READ_STATUS { + Some(Adapter::<T>::read_status_callback) + } else { + None + }, + read_mmd: if T::HAS_READ_MMD { + Some(Adapter::<T>::read_mmd_callback) + } else { + None + }, + write_mmd: if T::HAS_WRITE_MMD { + Some(Adapter::<T>::write_mmd_callback) + } else { + None + }, + link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY { + Some(Adapter::<T>::link_change_notify_callback) + } else { + None + }, + // SAFETY: The rest is zeroed out to initialize `struct phy_driver`, + // sets `Option<&F>` to be `None`. + ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() } + })) +} + +/// Driver implementation for a particular PHY type. +/// +/// This trait is used to create a [`DriverVTable`]. +#[vtable] +pub trait Driver { + /// Defines certain other features this PHY supports. + /// It is a combination of the flags in the [`flags`] module. + const FLAGS: u32 = 0; + + /// The friendly name of this PHY type. + const NAME: &'static CStr; + + /// This driver only works for PHYs with IDs which match this field. + /// The default id and mask are zero. + const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0); + + /// Issues a PHY software reset. + fn soft_reset(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Probes the hardware to determine what abilities it has. + fn get_features(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Returns true if this is a suitable driver for the given phydev. + /// If not implemented, matching is based on [`Driver::PHY_DEVICE_ID`]. + fn match_phy_device(_dev: &Device) -> bool { + false + } + + /// Configures the advertisement and resets auto-negotiation + /// if auto-negotiation is enabled. + fn config_aneg(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Determines the negotiated speed and duplex. + fn read_status(_dev: &mut Device) -> Result<u16> { + Err(code::ENOTSUPP) + } + + /// Suspends the hardware, saving state if needed. + fn suspend(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Resumes the hardware, restoring state if needed. + fn resume(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Overrides the default MMD read function for reading a MMD register. + fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> { + Err(code::ENOTSUPP) + } + + /// Overrides the default MMD write function for writing a MMD register. + fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result { + Err(code::ENOTSUPP) + } + + /// Callback for notification of link change. + fn link_change_notify(_dev: &mut Device) {} +} + +/// Registration structure for PHY drivers. +/// +/// Registers [`DriverVTable`] instances with the kernel. They will be unregistered when dropped. +/// +/// # Invariants +/// +/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`. +pub struct Registration { + drivers: Pin<&'static mut [DriverVTable]>, +} + +impl Registration { + /// Registers a PHY driver. + pub fn register( + module: &'static crate::ThisModule, + drivers: Pin<&'static mut [DriverVTable]>, + ) -> Result<Self> { + if drivers.is_empty() { + return Err(code::EINVAL); + } + // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of + // the `drivers` slice are initialized properly. `drivers` will not be moved. + // So an FFI call with a valid pointer. + to_result(unsafe { + bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0) + })?; + // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`. + Ok(Registration { drivers }) + } +} + +impl Drop for Registration { + fn drop(&mut self) { + // SAFETY: The type invariants guarantee that `self.drivers` is valid. + // So an FFI call with a valid pointer. + unsafe { + bindings::phy_drivers_unregister(self.drivers[0].0.get(), self.drivers.len() as i32) + }; + } +} + +/// An identifier for PHY devices on an MDIO/MII bus. +/// +/// Represents the kernel's `struct mdio_device_id`. This is used to find an appropriate +/// PHY driver. +pub struct DeviceId { + id: u32, + mask: DeviceMask, +} + +impl DeviceId { + /// Creates a new instance with the exact match mask. + pub const fn new_with_exact_mask(id: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Exact, + } + } + + /// Creates a new instance with the model match mask. + pub const fn new_with_model_mask(id: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Model, + } + } + + /// Creates a new instance with the vendor match mask. + pub const fn new_with_vendor_mask(id: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Vendor, + } + } + + /// Creates a new instance with a custom match mask. + pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Custom(mask), + } + } + + /// Creates a new instance from [`Driver`]. + pub const fn new_with_driver<T: Driver>() -> Self { + T::PHY_DEVICE_ID + } + + /// Get a `mask` as u32. + pub const fn mask_as_int(&self) -> u32 { + self.mask.as_int() + } + + // macro use only + #[doc(hidden)] + pub const fn mdio_device_id(&self) -> bindings::mdio_device_id { + bindings::mdio_device_id { + phy_id: self.id, + phy_id_mask: self.mask.as_int(), + } + } +} + +enum DeviceMask { + Exact, + Model, + Vendor, + Custom(u32), +} + +impl DeviceMask { + const MASK_EXACT: u32 = !0; + const MASK_MODEL: u32 = !0 << 4; + const MASK_VENDOR: u32 = !0 << 10; + + const fn as_int(&self) -> u32 { + match self { + DeviceMask::Exact => Self::MASK_EXACT, + DeviceMask::Model => Self::MASK_MODEL, + DeviceMask::Vendor => Self::MASK_VENDOR, + DeviceMask::Custom(mask) => *mask, + } + } +}
This patch adds abstractions to implement network PHY drivers; the driver registration and bindings for some of callback functions in struct phy_driver and many genphy_ functions. This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y. This patch enables unstable const_maybe_uninit_zeroed feature for kernel crate to enable unsafe code to handle a constant value with uninitialized data. With the feature, the abstractions can initialize a phy_driver structure with zero easily; instead of initializing all the members by hand. It's supposed to be stable in the not so distant future. Link: https://github.com/rust-lang/rust/pull/116218 Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- drivers/net/phy/Kconfig | 8 + rust/bindings/bindings_helper.h | 3 + rust/kernel/lib.rs | 3 + rust/kernel/net.rs | 6 + rust/kernel/net/phy.rs | 754 ++++++++++++++++++++++++++++++++ 5 files changed, 774 insertions(+) create mode 100644 rust/kernel/net.rs create mode 100644 rust/kernel/net/phy.rs