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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o7gtggyt.fsf@metaspace.dk>
Date: Fri, 20 Oct 2023 22:30:49 +0200
From: "Andreas Hindborg (Samsung)" <nmi@...aspace.dk>
To: Andrew Lunn <andrew@...n.ch>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com,
 tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com,
 benno.lossin@...ton.me, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY
 drivers


Andrew Lunn <andrew@...n.ch> writes:

> On Fri, Oct 20, 2023 at 07:26:50PM +0200, Andreas Hindborg (Samsung) wrote:
>> 
>> Hi,
>> 
>> FUJITA Tomonori <fujita.tomonori@...il.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.

Thanks, I'll read through it.

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

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