diff mbox series

[RFC,net-next,1/4] net: marvell: prestera: try to load previous fw version

Message ID 20210519143321.849-2-vadym.kochan@plvision.eu (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Marvell Prestera Switchdev initial updates for firmware version 3.0 | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 128 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Vadym Kochan May 19, 2021, 2:33 p.m. UTC 
From: Vadym Kochan <vkochan@marvell.com>

Lets try to load previous fw version in case the latest one is missing on
existing system.

Signed-off-by: Vadym Kochan <vkochan@marvell.com>
---
 .../ethernet/marvell/prestera/prestera_pci.c  | 75 ++++++++++++++-----
 1 file changed, 56 insertions(+), 19 deletions(-)

Comments

Andrew Lunn May 20, 2021, 1:12 a.m. UTC | #1 
> +static int prestera_fw_get(struct prestera_fw *fw)
> +{
> +	int ver_maj = PRESTERA_SUPP_FW_MAJ_VER;
> +	int ver_min = PRESTERA_SUPP_FW_MIN_VER;
> +	char fw_path[128];
> +	int err;
> +
> +pick_fw_ver:
> +	snprintf(fw_path, sizeof(fw_path), PRESTERA_FW_PATH_FMT,
> +		 ver_maj, ver_min);
> +
> +	err = request_firmware_direct(&fw->bin, fw_path, fw->dev.dev);
> +	if (err) {
> +		if (ver_maj == PRESTERA_SUPP_FW_MAJ_VER) {
> +			ver_maj--;
> +			goto pick_fw_ver;
> +		} else {
> +			dev_err(fw->dev.dev, "failed to request firmware file\n");
> +			return err;
> +		}
> +	}

So lets say for the sake of the argument, you have version 3.0 and
2.42. It looks like this code will first try to load version 3.0. If
that fails, ver_maj is decremented, so it tries 2.0, which also fails
because it should be looking for 2.42. I don't think this decrement is
a good idea.

Also:

> +			dev_err(fw->dev.dev, "failed to request firmware file\n");

It would be useful to say what version you are actually looking for,
so the user can go find the correct firmware.

   Andrew
Vadym Kochan May 20, 2021, 10:48 a.m. UTC | #2 
On Thu, May 20, 2021 at 03:12:02AM +0200, Andrew Lunn wrote:
> > +static int prestera_fw_get(struct prestera_fw *fw)
> > +{
> > +	int ver_maj = PRESTERA_SUPP_FW_MAJ_VER;
> > +	int ver_min = PRESTERA_SUPP_FW_MIN_VER;
> > +	char fw_path[128];
> > +	int err;
> > +
> > +pick_fw_ver:
> > +	snprintf(fw_path, sizeof(fw_path), PRESTERA_FW_PATH_FMT,
> > +		 ver_maj, ver_min);
> > +
> > +	err = request_firmware_direct(&fw->bin, fw_path, fw->dev.dev);
> > +	if (err) {
> > +		if (ver_maj == PRESTERA_SUPP_FW_MAJ_VER) {
> > +			ver_maj--;
> > +			goto pick_fw_ver;
> > +		} else {
> > +			dev_err(fw->dev.dev, "failed to request firmware file\n");
> > +			return err;
> > +		}
> > +	}
> 
> So lets say for the sake of the argument, you have version 3.0 and
> 2.42. It looks like this code will first try to load version 3.0. If
> that fails, ver_maj is decremented, so it tries 2.0, which also fails
> because it should be looking for 2.42. I don't think this decrement is
> a good idea.

Ahh, you are correct, I was too focused on a major version. So the only
option which I see is to hard-code also the previous version.

> 
> Also:
> 
> > +			dev_err(fw->dev.dev, "failed to request firmware file\n");
> 
> It would be useful to say what version you are actually looking for,
> so the user can go find the correct firmware.

I agree.

> 
>    Andrew

Thanks,
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
index 298110119272..d384dcacd579 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_pci.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
@@ -166,6 +166,8 @@  struct prestera_fw_evtq {
 };
 
 struct prestera_fw {
+	struct prestera_fw_rev rev_supp;
+	const struct firmware *bin;
 	struct workqueue_struct *wq;
 	struct prestera_device dev;
 	u8 __iomem *ldr_regs;
@@ -576,25 +578,24 @@  static void prestera_fw_rev_parse(const struct prestera_fw_header *hdr,
 static int prestera_fw_rev_check(struct prestera_fw *fw)
 {
 	struct prestera_fw_rev *rev = &fw->dev.fw_rev;
-	u16 maj_supp = PRESTERA_SUPP_FW_MAJ_VER;
-	u16 min_supp = PRESTERA_SUPP_FW_MIN_VER;
 
-	if (rev->maj == maj_supp && rev->min >= min_supp)
+	if (rev->maj == fw->rev_supp.maj && rev->min >= fw->rev_supp.min)
 		return 0;
 
 	dev_err(fw->dev.dev, "Driver supports FW version only '%u.%u.x'",
-		PRESTERA_SUPP_FW_MAJ_VER, PRESTERA_SUPP_FW_MIN_VER);
+		fw->rev_supp.maj, fw->rev_supp.min);
 
 	return -EINVAL;
 }
 
-static int prestera_fw_hdr_parse(struct prestera_fw *fw,
-				 const struct firmware *img)
+static int prestera_fw_hdr_parse(struct prestera_fw *fw)
 {
-	struct prestera_fw_header *hdr = (struct prestera_fw_header *)img->data;
 	struct prestera_fw_rev *rev = &fw->dev.fw_rev;
+	struct prestera_fw_header *hdr;
 	u32 magic;
 
+	hdr = (struct prestera_fw_header *)fw->bin->data;
+
 	magic = be32_to_cpu(hdr->magic_number);
 	if (magic != PRESTERA_FW_HDR_MAGIC) {
 		dev_err(fw->dev.dev, "FW img hdr magic is invalid");
@@ -609,11 +610,51 @@  static int prestera_fw_hdr_parse(struct prestera_fw *fw,
 	return prestera_fw_rev_check(fw);
 }
 
+static int prestera_fw_get(struct prestera_fw *fw)
+{
+	int ver_maj = PRESTERA_SUPP_FW_MAJ_VER;
+	int ver_min = PRESTERA_SUPP_FW_MIN_VER;
+	char fw_path[128];
+	int err;
+
+pick_fw_ver:
+	snprintf(fw_path, sizeof(fw_path), PRESTERA_FW_PATH_FMT,
+		 ver_maj, ver_min);
+
+	err = request_firmware_direct(&fw->bin, fw_path, fw->dev.dev);
+	if (err) {
+		if (ver_maj == PRESTERA_SUPP_FW_MAJ_VER) {
+			ver_maj--;
+			goto pick_fw_ver;
+		} else {
+			dev_err(fw->dev.dev, "failed to request firmware file\n");
+			return err;
+		}
+	}
+
+	if (ver_maj < PRESTERA_SUPP_FW_MAJ_VER)
+		dev_warn(fw->dev.dev,
+			 "using older FW version %u.%u than expected %u.%u\n",
+			 ver_maj, ver_min,
+			 PRESTERA_SUPP_FW_MAJ_VER, PRESTERA_SUPP_FW_MIN_VER);
+
+	dev_info(fw->dev.dev, "Loading %s ...", fw_path);
+
+	fw->rev_supp.maj = ver_maj;
+	fw->rev_supp.min = ver_min;
+	fw->rev_supp.sub = 0;
+
+	return 0;
+}
+
+static void prestera_fw_put(struct prestera_fw *fw)
+{
+	release_firmware(fw->bin);
+}
+
 static int prestera_fw_load(struct prestera_fw *fw)
 {
 	size_t hlen = sizeof(struct prestera_fw_header);
-	const struct firmware *f;
-	char fw_path[128];
 	int err;
 
 	err = prestera_ldr_wait_reg32(fw, PRESTERA_LDR_READY_REG,
@@ -632,30 +673,26 @@  static int prestera_fw_load(struct prestera_fw *fw)
 
 	fw->ldr_wr_idx = 0;
 
-	snprintf(fw_path, sizeof(fw_path), PRESTERA_FW_PATH_FMT,
-		 PRESTERA_SUPP_FW_MAJ_VER, PRESTERA_SUPP_FW_MIN_VER);
-
-	err = request_firmware_direct(&f, fw_path, fw->dev.dev);
+	err = prestera_fw_get(fw);
 	if (err) {
 		dev_err(fw->dev.dev, "failed to request firmware file\n");
 		return err;
 	}
 
-	err = prestera_fw_hdr_parse(fw, f);
+	err = prestera_fw_hdr_parse(fw);
 	if (err) {
 		dev_err(fw->dev.dev, "FW image header is invalid\n");
 		goto out_release;
 	}
 
-	prestera_ldr_write(fw, PRESTERA_LDR_IMG_SIZE_REG, f->size - hlen);
+	prestera_ldr_write(fw, PRESTERA_LDR_IMG_SIZE_REG, fw->bin->size - hlen);
 	prestera_ldr_write(fw, PRESTERA_LDR_CTL_REG, PRESTERA_LDR_CTL_DL_START);
 
-	dev_info(fw->dev.dev, "Loading %s ...", fw_path);
-
-	err = prestera_ldr_fw_send(fw, f->data + hlen, f->size - hlen);
+	err = prestera_ldr_fw_send(fw, fw->bin->data + hlen,
+				   fw->bin->size - hlen);
 
 out_release:
-	release_firmware(f);
+	prestera_fw_put(fw);
 	return err;
 }