netdev - Re: [PATCH net-next v6 6/6] net: phy: add Applied Micro QT2025 PHY driver

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] [day] [month] [year] [list]

Message-ID: <CALNs47u6+EFYkvpyHZD5zLcjQeb2CuZNTOjPuZ4MKewoKZYPMg@mail.gmail.com>
Date: Fri, 23 Aug 2024 14:13:25 -0500
From: Trevor Gross <tmgross@...ch.edu>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, 
	miguel.ojeda.sandonis@...il.com, benno.lossin@...ton.me, aliceryhl@...gle.com
Subject: Re: [PATCH net-next v6 6/6] net: phy: add Applied Micro QT2025 PHY driver

On Fri, Aug 23, 2024 at 8:37 AM FUJITA Tomonori
<fujita.tomonori@...il.com> wrote:
> > At this point the vendor driver looks like it does some verification:
> > it attempts to read 3.d7fd until it returns something other than 0x10
> > or 0, or times out. Could that be done here?
>
> Yeah, we better to wait here until the hw becomes ready (since the
> 8051 has just started) and check if it works correctly. A new Rust
> abstraction for msleep() is necessary.
>
> Even without the logic, the driver starts to work eventually (if the
> hw isn't broken) so I didn't include it in the patchset. I'll work on
> the abstraction and update the driver after this is merged.

It sounds okay to me to not block on this as long as it isn't glitchy
- It should probably be a FIXME?

> > Consistency nit: this file uses a mix of upper and lowercase hex
> > (mostly uppercase here) - we should probably be consistent. A quick
> > regex search looks like lowercase hex is about twice as common in the
> > kernel as uppercase so I think this may as well be updated.
>
> Ah, I'll use lowercase for all the hex in the driver.
>
> It will be a new coding rule for rust code in kernel? If so, can a
> checker tool warn this?

I don't know of any rule for this, I just noticed that the patch had
both and it made me take a look at what is used elsewhere. rustfmt has
an option to just commonize it for you, `hex_literal_case` [1], but it
is unstable. That would probably be nice at some point.

> > Overall this looks pretty good to me, checking against both the
> > datasheet and the vendor driver we have. Mostly small suggestions
> > here, I'm happy to add a RB with my verification question addressed
> > and some rewording of the 0xd001 (phy revision) comment.
>
> Thanks a lot! I'll send v7 soon.

Thanks!

- Trevor

[1]: https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#hex_literal_case