Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Oct 20, 2023 at 07:26:50PM +0200, Andreas Hindborg (Samsung) wrote:
> 
> Hi,
> 
> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> 
> <cut>
> 
> > +
> > +    /// Returns true if the link is up.
> > +    pub fn get_link(&self) -> bool {
> > +        const LINK_IS_UP: u32 = 1;
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        let phydev = unsafe { *self.0.get() };
> > +        phydev.link() == LINK_IS_UP
> > +    }
> 
> I would prefer `is_link_up` or `link_is_up`.

Hi Andreas

Have you read the comment on the previous versions of this patch. It
seems like everybody wants a different name for this, and we are going
round and round and round. Please could you spend the time to read all
the previous comments and then see if you still want this name, and
explain why. Otherwise i doubt we are going to reach consensus.

> If this function is called with `u32::MAX` `(*phydev).speed` will become -1. Is that OK?

Have you ever seen a Copper Ethernet interface which can do u32:MAX
Mbps? Do you think such a thing will ever be possible?

> > +    /// Callback for notification of link change.
> > +    fn link_change_notify(_dev: &mut Device) {}
> 
> It is probably an error if these functions are called, and so BUG() would be
> appropriate? See the discussion in [1].

Do you really want to kill the machine dead, no syncing of the disk,
loose everything not yet written to the disk, maybe corrupt the disk
etc?

      Andrew