Message-ID: <a5eb4909-afd4-4a23-93f9-7d9c1d9ac893@gmx.net>
Date: Mon, 26 Aug 2024 19:35:32 +0200
From: Hans-Frieder Vogt <hfdevel@....net>
To: Simon Horman <horms@...nel.org>
Cc: Andrew Lunn <andrew@...n.ch>, 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 25.08.2024 10.43, Simon Horman wrote:
> On Sat, Aug 24, 2024 at 07:34:07PM +0200, Hans-Frieder Vogt wrote:
>> Aquantia PHYs without EEPROM have to load the firmware via the file system and
>> upload it to the PHY via MDIO.
>> Because the Aquantia PHY firmware is different for the same PHY depending on the
>> MAC it is connected to, it is not possible to statically define the firmware name.
>> When in an embedded environment, the device-tree can provide the file name. But when the PHY is on a PCIe card, the file name needs to be provided in a different
>> way.
>>
>> This patch creates a firmware file name at run time, based on the Aquantia PHY
>> name and the MDIO name. By this, firmware files for ths same PHY, but combined
>> with different MACs are distinguishable.
>>
>> The proposed naming uses the scheme:
>> 	mdio/phy-mdio_suffix
>> Or, in the case of the Tehuti TN9510 card (TN4010 MAC and AQR105 PHY), the firmware
>> file name will be
>> 	tn40xx/aqr105-tn40xx_fw.cld
>>
>> This naming style has been chosen in order to make the filename unique, but also
>> to place the firmware in a directory named after the MAC, where different firmwares
>> could be collected.
>>
>> Signed-off-by: Hans-Frieder Vogt<hfdevel@....net>
> Please consider running this patch through:
>
> ./scripts/checkpatch.pl --strict --codespell --max-line-length=80
Sure, will do.
>> ---
>>   drivers/net/phy/aquantia/aquantia_firmware.c | 78 ++++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>>
>> diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c
>> index 524627a36c6f..265bd6ee21da 100644
>> --- a/drivers/net/phy/aquantia/aquantia_firmware.c
>> +++ b/drivers/net/phy/aquantia/aquantia_firmware.c
>> @@ -5,6 +5,7 @@
>>   #include <linux/firmware.h>
>>   #include <linux/crc-itu-t.h>
>>   #include <linux/nvmem-consumer.h>
>> +#include <linux/ctype.h>	/* for tolower() */
>>
>>   #include <asm/unaligned.h>
>>
>> @@ -321,6 +322,81 @@ static int aqr_firmware_load_nvmem(struct phy_device *phydev)
>>   	return ret;
>>   }
>>
>> +/* 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)
> nit: I would have made these declarations outside of aqr_firmware_name(),
>       probably near the top of this file.
will follow  your advice (if still needed in the next version of the patch)
>> +	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;
> A general comment: I've been over the string handling in this file.
> And it seems correct to me. But it is pretty hairy, and I could
> well have missed a problem. String handling in C is like that.
To be honest, I am also not overly happy about the string handling. At
least I
tried to implement it as safe as possible by using strscpy wherever I
saw a risk.
>> +
>> +	/* 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;
>> +
>> +	fw_name = devm_kzalloc(&phydev->mdio.dev, FIRMWARE_NAME_SIZE, GFP_KERNEL);
>> +	if (!fw_name)
>> +		return -ENOMEM;
>> +
>> +	/* first the directory name = MDIO bus name
>> +	 * (only name component, firmware name part 1; remove busids and the likes)
>> +	 * ignore the return value of strscpy: if the MAC/MDIO name is too long,
>> +	 * it will just be truncated
>> +	 */
>> +	strscpy(fw_name, phydev->mdio.bus->id, NAME_PART_SIZE + 1);
>> +	for (i = 0; fw_name[i]; i++) {
>> +		if (fw_name[i] == '-' || fw_name[i] == '_' || fw_name[i] == ':')
>> +			break;
>> +	}
>> +	mac_len = i;	/* without trailing zero */
>> +
>> +	fw_name[i++] = '/';
>> +
>> +	/* copy name part beyond AQUANTIA_NAME into our name buffer - name part 2 */
>> +	len = strscpy(&fw_name[i], phydev->drv->name + strlen(AQUANTIA_NAME),
>> +		      FIRMWARE_NAME_SIZE - i);
>> +	if (len < 0)
>> +		return len;	/* should never happen */
>> +
>> +	/* convert the name to lower case */
>> +	for (j = i; j < i + len; j++)
>> +		fw_name[j] = tolower(fw_name[j]);
>> +	i += len;
>> +
>> +	/* split the phy and mdio components with a dash */
>> +	fw_name[i++] = '-';
>> +
>> +	/* copy again the mac_name into fw_name - name part 3 */
>> +	memcpy(&fw_name[i], fw_name, mac_len);
> Are you completely sure that there are mac_len bytes available here?
> I appreciate that you need to clamp the number of source bytes.
> But elsewhere, where strscpy(), the space available at the destination
> is bounded for safety. And that is missing here.
mac_len is always defined and between 0 and NAME_PART_SIZE. And by
ensuring that
NAME_PART_SIZE is less than 1/3 of the length of the allocated string, I
consider
this safe.
However, it is definitely not beautiful.
>> +
>> +	/* copy file suffix (name part 4 - don't forget the trailing '\0') */
>> +	len = strscpy(&fw_name[i + mac_len], AQUANTIA_FW_SUFFIX, FIRMWARE_NAME_SIZE - i - mac_len);
> nit: I might have incremented i by mac_len to slightly simplify the above.
good point!
>> +	if (len < 0)
>> +		return len;	/* should never happen */
>> +
>> +	if (name)
> name is never NULL. I would drop this condition.
sure.
>> +		*name = fw_name;
>> +	return 0;
>> +}
>> +
>>   static int aqr_firmware_load_fs(struct phy_device *phydev)
>>   {
>>   	struct device *dev = &phydev->mdio.dev;
>> @@ -330,6 +406,8 @@ static int aqr_firmware_load_fs(struct phy_device *phydev)
>>
>>   	ret = of_property_read_string(dev->of_node, "firmware-name",
>>   				      &fw_name);
>> +	if (ret)
>> +		ret = aqr_firmware_name(phydev, &fw_name);
>>   	if (ret)
>>   		return ret;
>>
>> --
>> 2.43.0
>>
>>
Thanks Simon, for your review comments!

Powered by blists - more mailing lists