| Message ID | trinity-916ed524-e8a5-4534-b059-3ed707ec3881-1724520847823@3c-app-gmx-bs42 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: aquantia: enable firmware loading for aqr105 on PCIe cards | expand |
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 > >
> +/* 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
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
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;
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(+) -- 2.43.0