Re: [PATCH net-next v2 2/6] rust: net::phy support probe callback

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Aug 1, 2024 at 2:17 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Thanks for the review!
>
> On Wed, 31 Jul 2024 14:32:18 +0200
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> > +    /// # Safety
> >> > +    ///
> >> > +    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> >> > +    unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
> >> > +        from_result(|| {
> >> > +            // SAFETY: This callback is called only in contexts
> >> > +            // where we can exclusively access to `phy_device`, so the accessors on
> >> > +            // `Device` are okay to call.
> >>
> >> This one is slightly different to other callbacks. probe is called
> >> without the mutex. Instead, probe is called before the device is
> >> published. So the comment is correct, but given how important Rust
> >> people take these SAFETY comments, maybe it should indicate it is
> >> different to others?
> >
> > Interesting. Given that we don't hold the mutex, does that mean that
> > some of the methods on Device are not safe to call in this context? Or
> > is there something else that makes it okay to call them despite not
> > holding the mutex?
>
> Before the callback, the device object was initialized properly by
> PHYLIB and no concurrent access so all the methods can be called
> safely (no kernel panic), I think.
>
> If the safety comment needs to updated, how about the following?
>
> SAFETY: This callback is called only in contexts where we can
> exclusively access to `phy_device` because it's not published yet, so
> the accessors on `Device` are okay to call.

Yes, that sounds good to me. With the updated safety comment, feel
free to include my Reviewed-by in your next version.

Alice