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: <20240819.121936.1897793847560374944.fujita.tomonori@gmail.com>
Date: Mon, 19 Aug 2024 12:19:36 +0000 (UTC)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: tmgross@...ch.edu
Cc: fujita.tomonori@...il.com, 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, 19 Aug 2024 04:07:30 -0500
Trevor Gross <tmgross@...ch.edu> wrote:

>> +#[vtable]
>> +impl Driver for PhyQT2025 {
>> +    const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+");
>> +    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x0043A400);
>> +
>> +    fn probe(dev: &mut phy::Device) -> Result<()> {
>> +        // The vendor driver does the following checking but we have no idea why.
> 
> 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?

>> +        let hw_id = dev.read(C45::new(Mmd::PMAPMD, 0xd001))?;
>> +        if (hw_id >> 8) & 0xff != 0xb3 {
>> +            return Err(code::ENODEV);
>> +        }
>> +
>> +        // The 8051 will remain in the reset state.
> 
> What is the 8051 here?

Intel 8051. I'll update the comment.

>> +        dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0000)?;
>> +        // Configure the 8051 clock frequency.
>> +        dev.write(C45::new(Mmd::PMAPMD, 0xC302), 0x0004)?;
>> +        // Non loopback mode.
>> +        dev.write(C45::new(Mmd::PMAPMD, 0xC319), 0x0038)?;
>> +        // Global control bit to select between LAN and WAN (WIS) mode.
>> +        dev.write(C45::new(Mmd::PMAPMD, 0xC31A), 0x0098)?;
>> +        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?

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

> 1. Hint the MMD name in the comments
> 2. Give i and j descriptive names (I used `src_idx` and `dst_offset`)
> 3. Set `mmd` once at the same time you reset the address offset
> 4. Tracking the offset from 0 rather than from SZ_32K seems more readable
> 
> 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).

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