netdev - Re: [PATCH net-next v5 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALNs47u8=J14twTLGos6MM6fZWSiR5GVVyooLt7mxUyX4XhHcQ@mail.gmail.com>
Date: Mon, 19 Aug 2024 15:30:57 -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 v5 6/6] net: phy: add Applied Micro QT2025 PHY driver

On Mon, Aug 19, 2024 at 7:20 AM FUJITA Tomonori
<fujita.tomonori@...il.com> wrote:
>
> On Mon, 19 Aug 2024 04:07:30 -0500
> Trevor Gross <tmgross@...ch.edu> wrote:
> > [...]
> > In the module doc comment, could you add a note about where the vendor
> > driver came from? I am not sure how to find it.
>
> For example, it's available at Edimax site:
>
> https://www.edimax.com/edimax/download/download/data/edimax/global/download/smb_network_adapters_pci_card/en-9320sfp_plus
>
> I could add it to the module comment but not sure if the URL will be
> available for for long-term use. How about uploading the code to github
> and adding the link?

Great, thanks for the link. I don't even know that you need to include
it directly, maybe something like

    //!
    //! This driver is based on the vendor driver `qt2025_phy.c` This source
    //! and firmware can be downloaded on the EN-9320SFP+ support site.

so anyone reading in the future knows what to look for without relying
on a link. But I don't know what the policy is here.

> >> +        dev.write(C45::new(Mmd::PCS, 0x0026), 0x0E00)?;
> >> +        dev.write(C45::new(Mmd::PCS, 0x0027), 0x0893)?;
> >> +        dev.write(C45::new(Mmd::PCS, 0x0028), 0xA528)?;
> >> +        dev.write(C45::new(Mmd::PCS, 0x0029), 0x0003)?;
> >
> > The above four writes should probably get a comment based on the
> > discussion at [1].
>
> // The following for writes use standardized registers (3.38 through
> // 3.41 5/10/25GBASE-R PCS test pattern seed B) for something else.
> // We don't know what.
>
> Looks good?

Seems reasonable to me, thanks.

> >> +        // Configure transmit and recovered clock.
> >> +        dev.write(C45::new(Mmd::PMAPMD, 0xC30A), 0x06E1)?;
> >> +        // The 8051 will finish the reset state.
> >> +        dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0002)?;
> >> +        // The 8051 will start running from the boot ROM.
> >> +        dev.write(C45::new(Mmd::PCS, 0xE854), 0x00C0)?;
> >> +
> >> +        let fw = Firmware::request(c_str!("qt2025-2.0.3.3.fw"), dev.as_ref())?;
> >
> > I don't know if this works, but can you put `qt2025-2.0.3.3.fw` in a
> > const to use both here and in the `module_phy_driver!` macro?
>
> It doesn't work. Variables can't be used in the `module_phy_driver!`
> macro.

Ah, that is unfortunate. Maybe we should try to fix that if the
firmware name isn't actually needed at compile time (not here of
course).

> > E.g.:
> >
> >     // The 24kB of program memory space is accessible by MDIO.
> >     // The first 16kB of memory is located in the address range
> > 3.8000h - 3.BFFFh (PCS).
> >     // The next 8kB of memory is located at 4.8000h - 4.9FFFh (PHYXS).
> >     let mut dst_offset = 0;
> >     let mut dst_mmd = Mmd::PCS;
> >     for (src_idx, val) in fw.data().iter().enumerate() {
> >         if src_idx == SZ_16K {
> >             // Start writing to the next register with no offset
> >             dst_offset = 0;
> >             dst_mmd = Mmd::PHYXS;
> >         }
> >
> >         dev.write(C45::new(dst_mmd, 0x8000 + dst_offset), (*val).into())?;
> >
> >         dst_offset += 1;
> >     }
>
> Surely, more readable. I'll update the code (I need to add
> #[derive(Copy, Clone)] to reg::Mmd with this change).

Didn't think about that, but sounds reasonable. `C22` and `C45` as
well probably, maybe `Debug` would come in handy in the future too.

> > Alternatively you could split the iterator with
> > `.by_ref().take(SZ_16K)`, but that may not be more readable.
> >
> >> +        // The 8051 will start running from SRAM.
> >> +        dev.write(C45::new(Mmd::PCS, 0xE854), 0x0040)?;
> >> +
> >> +        Ok(())
> >> +    }
> >> +
> >> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
> >> +        dev.genphy_read_status::<C45>()
> >> +    }
> >> +}
> >
> > Overall this looks pretty reasonable to me, I just don't know what to
> > reference for the initialization sequence.
>
> You can find the initialization sequence of the vendor driver at:
>
> https://github.com/acooks/tn40xx-driver/blob/vendor-drop/v0.3.6.15/QT2025_phy.c
>
> Thanks a lot!

Thanks! I'll try to cross check against that code later.

- Trevor