Andrew Lunn [off-list ref] writes:

On Fri, Oct 20, 2023 at 07:26:50PM +0200, Andreas Hindborg (Samsung) wrote:

quoted

Hi,

FUJITA Tomonori [off-list ref] writes:

<cut>

quoted

+
+    /// 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.

Thanks, I'll read through it.

quoted

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?

Probably not. Maybe a dummy device for testing? I don't know if it would
be OK with a negative value, hence the question. If things break with a
negative value, it makes sense to force the value into the valid range.
Make it impossible to break it, instead of relying on nobody ever doing
things you did not expect.

quoted

quoted

+    /// 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?

A WARN then? Benno suggests a compile time error, that is probably a
better approach. The code should not be called, so I would really want
to know if that was ever the case.

BR Andreas