netdev - Re: [PATCH net-next v3 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: <20240816.061710.793938744815241582.fujita.tomonori@gmail.com>
Date: Fri, 16 Aug 2024 06:17:10 +0000 (UTC)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: andrew@...n.ch
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, tmgross@...ch.edu,
 miguel.ojeda.sandonis@...il.com, benno.lossin@...ton.me,
 aliceryhl@...gle.com
Subject: Re: [PATCH net-next v3 6/6] net: phy: add Applied Micro QT2025 PHY
 driver

On Fri, 16 Aug 2024 03:45:09 +0200
Andrew Lunn <andrew@...n.ch> 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 hardware is configurable?
>> +        let hw_id = dev.read(C45::new(Mmd::PMAPMD, 0xd001))?;
>> +        if (hw_id >> 8) & 0xff != 0xb3 {
>> +            return Ok(());
>> +        }
> 
> I don't understand this bit of code. At a guess, if the upper bytes of
> that register is not 0xb3, the firmware has already been loaded into
> the device?

I've just added debug code and found that the upper bytes of the
register is 0xb3 even after loading the firmware.

I checked the original code again and found that if the bytes isn't
0xb3, the driver initialization fails. I guess that the probe should
return an error here (ENODEV?). 

>> +        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)?;
> 
> 802.3 says:
> 
> 3.38 through 3.4110/25GBASE-R PCS test pattern seed B ????

Yeah, strange. But I can't find any hints on them in the datasheet or
the original code.

>> +        let fw = Firmware::request(c_str!("qt2025-2.0.3.3.fw"), dev.as_ref())?;
>> +        if fw.data().len() > SZ_16K + SZ_8K {
>> +            return Err(code::EFBIG);
>> +        }
>> +
>> +        // 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.
>> +        // The next 8kB of memory is located at 4.8000h - 4.9FFFh.
>> +        let mut j = SZ_32K;
>> +        for (i, val) in fw.data().iter().enumerate() {
>> +            if i == SZ_16K {
>> +                j = SZ_32K;
>> +            }
>> +
>> +            let mmd = if i < SZ_16K { Mmd::PCS } else { Mmd::PHYXS };
>> +            dev.write(
>> +                C45::new(mmd, j as u16),
>> +                <u8 as Into<u16>>::into(*val).to_le(),
> 
> This is well past my level of Rust. I assume fw.data is a collection
> of bytes, and you enumerate it as bytes. A byte has no endiannes, so
> why do you need to convert it to little endian?

I messed up here. No need. I'll remove the conversion in the next version.

Thanks!