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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <58560556-c288-4ea7-bea9-7e9083628dbf@gmx.net>
Date: Mon, 26 Aug 2024 19:49:49 +0200
From: Hans-Frieder Vogt <hfdevel@....net>
To: Andrew Lunn <andrew@...n.ch>
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

On 26.08.2024 03.17, Andrew Lunn wrote:
>> +/* 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
Thanks Andrew!
I have checked many cases, but obviously haven't considered enough the
multiplicity of network device topologies.
Thanks very much for the hint with the txgbe driver. I will study it and
come
back with hopefully a generic approach.

         Hans