| Message ID | 20231211194909.588574-1-boqun.feng@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] rust: net: phy: Correct the safety comment for impl Sync | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
+ Tomo On Mon, Dec 11, 2023 at 11:49:09AM -0800, Boqun Feng wrote: > The current safety comment for impl Sync for DriverVTable has two > problem: > > * the correctness is unclear, since all types impl Any[1], therefore all > types have a `&self` method (Any::type_id). > > * it doesn't explain why useless of immutable references can ensure the > safety. > > Fix this by rewritting the comment. > > [1]: https://doc.rust-lang.org/std/any/trait.Any.html > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > This is a follow-up for my ignored feedback: > > https://lore.kernel.org/rust-for-linux/ZV5FjEM1EWm6iTAm@boqun-archlinux/ > > Honestly, I believe that people who are active in the review process all > get it right, but I want to make sure who read the code later can also > understand it and reuse the reasoning in their code. Hence this > improvement. > > Tomo, feel free to fold it in your patch if you and others think the > wording is fine. > > rust/kernel/net/phy.rs | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs > index d9cec139324a..e3377f8f36b7 100644 > --- a/rust/kernel/net/phy.rs > +++ b/rust/kernel/net/phy.rs > @@ -489,8 +489,8 @@ impl<T: Driver> Adapter<T> { > #[repr(transparent)] > pub struct DriverVTable(Opaque<bindings::phy_driver>); > > -// SAFETY: `DriverVTable` has no &self methods, so immutable references to it > -// are useless. > +// SAFETY: `DriverVTable` doesn't expose any &self method to access internal data, so it's safe to > +// share `&DriverVTable` across execution context boundries. > unsafe impl Sync for DriverVTable {} > > /// Creates a [`DriverVTable`] instance from [`Driver`]. > -- > 2.43.0 >
On 12/11/23 20:49, Boqun Feng wrote: > The current safety comment for impl Sync for DriverVTable has two > problem: > > * the correctness is unclear, since all types impl Any[1], therefore all > types have a `&self` method (Any::type_id). > > * it doesn't explain why useless of immutable references can ensure the > safety. > > Fix this by rewritting the comment. > > [1]: https://doc.rust-lang.org/std/any/trait.Any.html > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> It's fine if you want to change it, but I think the current safety comment is good enough. Alice
On Mon, Dec 11, 2023 at 10:50:02PM +0100, Alice Ryhl wrote: > On 12/11/23 20:49, Boqun Feng wrote: > > The current safety comment for impl Sync for DriverVTable has two > > problem: > > > > * the correctness is unclear, since all types impl Any[1], therefore all > > types have a `&self` method (Any::type_id). > > > > * it doesn't explain why useless of immutable references can ensure the > > safety. > > > > Fix this by rewritting the comment. > > > > [1]: https://doc.rust-lang.org/std/any/trait.Any.html > > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > > It's fine if you want to change it, Does it mean you are OK with the new version in this patch? If so... > but I think the current safety comment is good enough. ... let's change it since the current version doesn't look good enough to me as I explained above (it's not wrong, but less straight-forward to me). Regards, Boqun > > Alice
On Mon, 11 Dec 2023 15:22:54 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Mon, Dec 11, 2023 at 10:50:02PM +0100, Alice Ryhl wrote: >> On 12/11/23 20:49, Boqun Feng wrote: >> > The current safety comment for impl Sync for DriverVTable has two >> > problem: >> > >> > * the correctness is unclear, since all types impl Any[1], therefore all >> > types have a `&self` method (Any::type_id). >> > >> > * it doesn't explain why useless of immutable references can ensure the >> > safety. >> > >> > Fix this by rewritting the comment. >> > >> > [1]: https://doc.rust-lang.org/std/any/trait.Any.html >> > >> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> >> >> It's fine if you want to change it, > > Does it mean you are OK with the new version in this patch? If so... > >> but I think the current safety comment is good enough. > > ... let's change it since the current version doesn't look good enough > to me as I explained above (it's not wrong, but less straight-forward to > me). I'll leave this alone and wait for opinions from other reviewers since you guys have different options. It's improvement so I don't need to hurry.
On Tue, Dec 12, 2023 at 12:55 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Mon, 11 Dec 2023 15:22:54 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > > > On Mon, Dec 11, 2023 at 10:50:02PM +0100, Alice Ryhl wrote: > >> On 12/11/23 20:49, Boqun Feng wrote: > >> > The current safety comment for impl Sync for DriverVTable has two > >> > problem: > >> > > >> > * the correctness is unclear, since all types impl Any[1], therefore all > >> > types have a `&self` method (Any::type_id). > >> > > >> > * it doesn't explain why useless of immutable references can ensure the > >> > safety. > >> > > >> > Fix this by rewritting the comment. > >> > > >> > [1]: https://doc.rust-lang.org/std/any/trait.Any.html > >> > > >> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > >> > >> It's fine if you want to change it, > > > > Does it mean you are OK with the new version in this patch? If so... > > > >> but I think the current safety comment is good enough. > > > > ... let's change it since the current version doesn't look good enough > > to me as I explained above (it's not wrong, but less straight-forward to > > me). > > I'll leave this alone and wait for opinions from other reviewers since > you guys have different options. It's improvement so I don't need to > hurry. To clarify, the modified safety comment is also okay with me. Alice
On Tue, 12 Dec 2023 10:17:40 +0100 Alice Ryhl <aliceryhl@google.com> wrote: > On Tue, Dec 12, 2023 at 12:55 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> On Mon, 11 Dec 2023 15:22:54 -0800 >> Boqun Feng <boqun.feng@gmail.com> wrote: >> >> > On Mon, Dec 11, 2023 at 10:50:02PM +0100, Alice Ryhl wrote: >> >> On 12/11/23 20:49, Boqun Feng wrote: >> >> > The current safety comment for impl Sync for DriverVTable has two >> >> > problem: >> >> > >> >> > * the correctness is unclear, since all types impl Any[1], therefore all >> >> > types have a `&self` method (Any::type_id). >> >> > >> >> > * it doesn't explain why useless of immutable references can ensure the >> >> > safety. >> >> > >> >> > Fix this by rewritting the comment. >> >> > >> >> > [1]: https://doc.rust-lang.org/std/any/trait.Any.html >> >> > >> >> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> >> >> >> >> It's fine if you want to change it, >> > >> > Does it mean you are OK with the new version in this patch? If so... >> > >> >> but I think the current safety comment is good enough. >> > >> > ... let's change it since the current version doesn't look good enough >> > to me as I explained above (it's not wrong, but less straight-forward to >> > me). >> >> I'll leave this alone and wait for opinions from other reviewers since >> you guys have different options. It's improvement so I don't need to >> hurry. > > To clarify, the modified safety comment is also okay with me. Thanks for the clarification. Then I'll fold this in the patchset.
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index d9cec139324a..e3377f8f36b7 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -489,8 +489,8 @@ impl<T: Driver> Adapter<T> { #[repr(transparent)] pub struct DriverVTable(Opaque<bindings::phy_driver>); -// SAFETY: `DriverVTable` has no &self methods, so immutable references to it -// are useless. +// SAFETY: `DriverVTable` doesn't expose any &self method to access internal data, so it's safe to +// share `&DriverVTable` across execution context boundries. unsafe impl Sync for DriverVTable {} /// Creates a [`DriverVTable`] instance from [`Driver`].
The current safety comment for impl Sync for DriverVTable has two problem: * the correctness is unclear, since all types impl Any[1], therefore all types have a `&self` method (Any::type_id). * it doesn't explain why useless of immutable references can ensure the safety. Fix this by rewritting the comment. [1]: https://doc.rust-lang.org/std/any/trait.Any.html Signed-off-by: Boqun Feng <boqun.feng@gmail.com> --- This is a follow-up for my ignored feedback: https://lore.kernel.org/rust-for-linux/ZV5FjEM1EWm6iTAm@boqun-archlinux/ Honestly, I believe that people who are active in the review process all get it right, but I want to make sure who read the code later can also understand it and reuse the reasoning in their code. Hence this improvement. Tomo, feel free to fold it in your patch if you and others think the wording is fine. rust/kernel/net/phy.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)