netdev - Re: [PATCH net-next 1/2] net: phy: aquantia: create firmware name for aqr PHYs at runtime

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: <d0a46a2a-af5c-4616-a092-a9b65a1e15e2@lunn.ch>
Date: Mon, 26 Aug 2024 03:17:23 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Hans-Frieder Vogt <hfdevel@....net>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	Vladimir Oltean <vladimir.oltean@....com>,
	Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: create firmware name
 for aqr PHYs at runtime

> +/* derive the filename of the firmware file from the PHY and the MDIO names
> + * Parts of filename:
> + *   mdio/phy-mdio_suffix
> + *    1    2   3    4
> + * allow name components 1 (= 3) and 2 to have same maximum length
> + */
> +static int aqr_firmware_name(struct phy_device *phydev, const char **name)
> +{
> +#define AQUANTIA_FW_SUFFIX "_fw.cld"
> +#define AQUANTIA_NAME "Aquantia "
> +/* including the trailing zero */
> +#define FIRMWARE_NAME_SIZE 64
> +/* length of the name components 1, 2, 3 without the trailing zero */
> +#define NAME_PART_SIZE ((FIRMWARE_NAME_SIZE - sizeof(AQUANTIA_FW_SUFFIX) - 2) / 3)
> +	ssize_t len, mac_len;
> +	char *fw_name;
> +	int i, j;
> +
> +	/* sanity check: the phydev drv name needs to start with AQUANTIA_NAME */
> +	if (strncmp(AQUANTIA_NAME, phydev->drv->name, strlen(AQUANTIA_NAME)))
> +		return -EINVAL;
> +
> +	/* sanity check: the phydev drv name may not be longer than NAME_PART_SIZE */
> +	if (strlen(phydev->drv->name) - strlen(AQUANTIA_NAME) > NAME_PART_SIZE)
> +		return -E2BIG;
> +
> +	/* sanity check: the MDIO name must not be empty */
> +	if (!phydev->mdio.bus->id[0])
> +		return -EINVAL;

I'm not sure how well that is going to work. It was never intended to
be used like this, so the names are not going to be as nice as your
example.

apm/xgene-v2/mdio.c:	snprintf(mdio_bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(dev));
apm/xgene/xgene_enet_hw.c:	snprintf(mdio_bus->id, MII_BUS_ID_SIZE, "%s-%s", "xgene-mii",
hisilicon/hix5hd2_gmac.c:	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev));
intel/ixgbe/ixgbe_phy.c:	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mdio-%s", ixgbe_driver_name,

I expect some of these IDs contain more than the MAC, eg. include the
PCI slot information, or physical address of the device. Some might
indicate the name of the MDIO controller, not the MAC.

Aquantia PHYs firmware is not nice. It contains more than firmware. I
think it has SERDES eye information. It also contain a section which
sets the reset values of various registers. It could well be, if you
have a board with two MACs and two PHYs, each needs it own firmware,
because it needs different eye information. So what you propose works
for your limited use case, but i don't think it is a general solution.

Device tree works, because you can specific a specific filename per
PHY. I think you need a similar solution. Please look at how txgbe
uses swnodes. See if you can populate the firmware-name node from the
MAC driver. That should be a generic solution.

	Andrew