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: <ec02de97-2521-4897-9076-cabb8f057c8b@lunn.ch>
Date: Fri, 16 Aug 2024 22:47:04 +0200
From: Andrew Lunn <andrew@...n.ch>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: 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, Aug 16, 2024 at 06:17:10AM +0000, FUJITA Tomonori wrote:
> 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?). 

O.K. Maybe add a comment that the vendor driver does this, but we have
no idea why.

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

Seems unlikely it is seeding anything. So i guess they have reused the
registers for something else. This is actually a bit odd, because all
the other registers are in the ranges reserved for vendor usage.

If these were legitimate register usage, i would of suggested adding
#define to include/uapi/linux/mdio.h for them.

	Andrew