netdev - Re: [PATCH net-next v1 4/4] 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: <adc1ba25-16f0-4cf4-a1f0-bac8820cec2e@lunn.ch>
Date: Mon, 15 Apr 2024 18:53:25 +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
Subject: Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY
 driver

> +const MDIO_MMD_PMAPMD: u8 = uapi::MDIO_MMD_PMAPMD as u8;
> +const MDIO_MMD_PCS: u8 = uapi::MDIO_MMD_PCS as u8;
> +const MDIO_MMD_PHYXS: u8 = uapi::MDIO_MMD_PHYXS as u8;

These probably belong somewhere else, where all Rust PHY drivers can
use them.

> +
> +struct PhyQT2025;
> +
> +#[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 config_init(dev: &mut phy::Device) -> Result<()> {
> +        let fw = Firmware::new(c_str!("qt2025-2.0.3.3.fw"), dev)?;
> +
> +        let phy_id = dev.c45_read(MDIO_MMD_PMAPMD, 0xd001)?;
> +        if (phy_id >> 8) & 0xff != 0xb3 {
> +            return Ok(());
> +        }

I'm guessing that is checking if the firmware has already been
downloaded? It would be good to add a comment about this. I also
wounder about the name phy_id? They are normally stored in registers
0x2 and 0x3, not 0xd001. What sort of ID is this?

> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0000)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC302, 0x4)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC319, 0x0038)?;
> +
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC31A, 0x0098)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0026, 0x0E00)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0027, 0x0893)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0028, 0xA528)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0029, 0x03)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC30A, 0x06E1)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0002)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0xE854, 0x00C0)?;

Some of these registers are partially documented in the
datasheet. e854 controls where the 8051 executes code from. If bit 6
is set, it executes from SRAM, if it is cleared, to runs the Boot
ROM. Bit 7 is not defined, but i guess it halts the 8051 when
set. Please add some const for these. C302 is setting the CPU clock
speed, c30A is about TX clock rate. Please try to document what you
can using information from the datasheet.

> +
> +        let mut j = 0x8000;
> +        let mut a = MDIO_MMD_PCS;
> +        for (i, val) in fw.data().iter().enumerate() {
> +            if i == 0x4000 {

Is this was C code, i would of said use SZ_16K, to give a hint this is
about reading the first 16K of the firmware. The device actually has
24K of SRAM, which gets mapped into two different C45 address spaces.
You should also add a check that the firmware does not have a total
size of > 24K. Never trust firmware blobs.

> +                a = MDIO_MMD_PHYXS;
> +                j = 0x8000;
> +            }
> +            dev.c45_write(a, j, (*val).into())?;
> +
> +            j += 1;
> +        }
> +        dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;
> +
> +        Ok(())
> +    }

  Andrew