netdev - Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

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]

Date: Fri, 15 Dec 2023 15:52:23 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, kabel@...nel.org, andrew@...n.ch,
	hkallweit1@...il.com, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	netdev@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware
 loading on 88X3310

On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
> +	MV_PMA_BOOT_PRGS_MASK	= 0x0006,
> +	MV_PMA_BOOT_PRGS_INIT	= 0x0000,
> +	MV_PMA_BOOT_PRGS_WAIT	= 0x0002,
> +	MV_PMA_BOOT_PRGS_CSUM	= 0x0004,
> +	MV_PMA_BOOT_PRGS_JRAM	= 0x0006,

You only seem to use PRGS_WAIT, the rest seem unused.

> +struct mv3310_fw_hdr {
> +	struct {
> +		u32 size;
> +		u32 addr;
> +		u16 csum;
> +	} __packed data;

It's probably better to get rid of this embedded struct and just place
the members in the parent struct (although csum woul dneed to be
renamed).

> +
> +	u8 flags;
> +#define MV3310_FW_HDR_DATA_ONLY BIT(6)
> +
> +	u8 port_skip;
> +	u32 next_hdr;
> +	u16 csum;
> +
> +	u8 pad[14];
> +} __packed;
> +
> +static int mv3310_load_fw_sect(struct phy_device *phydev,
> +			       const struct mv3310_fw_hdr *hdr, const u8 *data)
> +{
> +	int err = 0;
> +	size_t i;
> +	u16 csum;
> +
> +	dev_dbg(&phydev->mdio.dev, "Loading %u byte %s section at 0x%08x\n",
> +		hdr->data.size,
> +		(hdr->flags & MV3310_FW_HDR_DATA_ONLY) ? "data" : "executable",
> +		hdr->data.addr);
> +
> +	for (i = 0, csum = 0; i < hdr->data.size; i++)
> +		csum += data[i];
> +
> +	if ((u16)~csum != hdr->data.csum) {
> +		dev_err(&phydev->mdio.dev, "Corrupt section data\n");
> +		return -EINVAL;
> +	}
> +
> +	phy_lock_mdio_bus(phydev);
> +
> +	/* Any existing checksum is cleared by a read */
> +	__phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
> +
> +	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_LOW,  hdr->data.addr & 0xffff);
> +	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_HIGH, hdr->data.addr >> 16);
> +
> +	for (i = 0; i < hdr->data.size; i += 2) {
> +		__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_DATA,
> +				(data[i + 1] << 8) | data[i]);
> +	}
> +
> +	csum = __phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
> +	if ((u16)~csum != hdr->data.csum) {
> +		dev_err(&phydev->mdio.dev, "Download failed\n");
> +		err = -EIO;
> +		goto unlock;
> +	}
> +
> +	if (hdr->flags & MV3310_FW_HDR_DATA_ONLY)
> +		goto unlock;
> +
> +	__phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT, 0, MV_PMA_BOOT_APP_LOADED);
> +	mdelay(200);
> +	if (!(__phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT) & MV_PMA_BOOT_APP_STARTED)) {
> +		dev_err(&phydev->mdio.dev, "Application did not startup\n");
> +		err = -ENODEV;
> +	}

I'm confused why this is done here - after each section in the firmware
file, rather than having loaded all sections in the firmware file and
only then starting the application. Surely if there's multiple sections
that we're going to load, we want to load _all_ sections before starting
the application?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!