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

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

[Hans-Frieder Vogt]

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@gmx.net>
---
 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)
+	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;
+
+	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);
+
+	/* 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);
+	if (len < 0)
+		return len;	/* should never happen */
+
+	if (name)
+		*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

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

[Simon Horman]

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@gmx.net>

Please consider running this patch through:

./scripts/checkpatch.pl --strict --codespell --max-line-length=80

> ---
>  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.

> +	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.

> +
> +	/* 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.

> +
> +	/* 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.

> +	if (len < 0)
> +		return len;	/* should never happen */
> +
> +	if (name)

name is never NULL. I would drop this condition.

> +		*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
> 
> 

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

[Andrew Lunn]

> +/* 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

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

[Hans-Frieder Vogt]

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

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

[Hans-Frieder Vogt]

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@gmx.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!