netdev - Re: [PATCH] net: phy: marvell10g: add firmware load support

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: <ad024231-40bd-c82f-e6d0-3b1b00c93e9a@gmail.com>
Date:   Tue, 31 Mar 2020 12:37:34 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Baruch Siach <baruch@...s.co.il>,
        Russell King <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, Shmuel Hazan <sh@...s.co.il>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH] net: phy: marvell10g: add firmware load support



On 3/31/2020 10:47 AM, Baruch Siach wrote:
> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
> bit is pulled up, the host must load firmware to the PHY after reset.
> Add support for loading firmware.
> 
> Firmware files are available from Marvell under NDA.
> 
> Signed-off-by: Baruch Siach <baruch@...s.co.il>
> ---

[snip]

> +static int mv3310_write_firmware(struct phy_device *phydev, const u8 *data,
> +		unsigned int size)
> +{
> +	unsigned int low_byte, high_byte;
> +	u16 checksum = 0, ram_checksum;
> +	unsigned int i = 0;
> +
> +	while (i < size) {
> +		low_byte = data[i++];
> +		high_byte = data[i++];
> +		checksum += low_byte + high_byte;
> +		phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_DATA,
> +				(high_byte << 8) | low_byte);

If there is an error upon write, there is really no point in continuing
any further, so you should just break out of the loop and return an
error. Having to continue and then fail the checksum is going to take an
awful amount of time.

> +		cond_resched();
> +	}
> +
> +	ram_checksum = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);

Likewise, you need to check for phy_read_mmd() returning < 0.

> +	if (ram_checksum != checksum) {
> +		dev_err(&phydev->mdio.dev, "firmware checksum failed");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mv3310_report_firmware_rev(struct phy_device *phydev)
> +{
> +	int rev1, rev2;
> +
> +	rev1 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV1);
> +	rev2 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV2);
> +	if (rev1 < 0 || rev2 < 0)
> +		return;
> +
> +	dev_info(&phydev->mdio.dev, "Loaded firmware revision %d.%d.%d.%d",
> +			(rev1 & 0xff00) >> 8, rev1 & 0x00ff,
> +			(rev2 & 0xff00) >> 8, rev2 & 0x00ff);
> +}
> +
> +static int mv3310_load_firmware(struct phy_device *phydev)
> +{
> +	const struct firmware *fw_entry;
> +	char *fw_file;
> +	int ret;
> +
> +	switch (phydev->drv->phy_id) {
> +	case MARVELL_PHY_ID_88X3310:
> +		fw_file = "mrvl/x3310fw.hdr";
> +		break;
> +	case MARVELL_PHY_ID_88E2110:
> +		fw_file = "mrvl/e21x0fw.hdr";
> +		break;
> +	default:
> +		dev_warn(&phydev->mdio.dev, "unknown firmware file for %s PHY",
> +				phydev->drv->name);
> +		return -EINVAL;
> +	}
> +
> +	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Firmware size must be larger than header, and even */
> +	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
> +			(fw_entry->size % 2) != 0) {
> +		dev_err(&phydev->mdio.dev, "firmware file invalid");
> +		return -EINVAL;
> +	}

You need to release the firmware file here. There is also possibly
another case that you are not covering here, which is that the firmware
on disk is newer than the firmware *already* loaded in the PHY, this
should presumably update the running firmware to the latest copy.

Without being able to publish the firmware in linux-firmware though, all
of this may be moot.

> +
> +	/* Clear checksum register */
> +	phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
> +
> +	/* Set firmware load address */
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_LOW_WORD, 0);
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_HIGH_WORD, 0x0010);
> +
> +	ret = mv3310_write_firmware(phydev,
> +			fw_entry->data + MV_FIRMWARE_HEADER_SIZE,
> +			fw_entry->size - MV_FIRMWARE_HEADER_SIZE);
> +	if (ret < 0)
> +		return ret;

And here too.

> +
> +	phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT,
> +			MV_PMA_BOOT_FW_LOADED, MV_PMA_BOOT_FW_LOADED);
> +
> +	release_firmware(fw_entry);
-- 
Florian