[PATCH net-next v2 1/1] qed: add prints if request_firmware() failed

If driver load failed due to request_firmware() not finding the device
firmware file, add prints that help remedy the situation.

Signed-off-by: Ariel Elior <redacted>
Signed-off-by: Alok Prasad <redacted>
Signed-off-by: Venkata Sudheer Kumar Bhavaraju <redacted>
---
Changes in v2:
 - Rename QED_FW_REPO to FW_REPO
 - Move FW_REPO macro to qed_if.h
---
 drivers/net/ethernet/qlogic/qed/qed_main.c | 3 +++
 include/linux/qed/qed_if.h                 | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
index 46d4207f22a3..845c151c5def 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c

@@ -1285,6 +1285,9 @@ static int qed_slowpath_start(struct qed_dev *cdev,
 			DP_NOTICE(cdev,
 				  "Failed to find fw file - /lib/firmware/%s\n",
 				  QED_FW_FILE_NAME);
+			DP_NOTICE(cdev,
+				  "you may need to download firmware from %s",
+				  FW_REPO);
 			goto err;
 		}
 

diff --git a/include/linux/qed/qed_if.h b/include/linux/qed/qed_if.h
index 6dc4943d8aec..f063d82ef8f9 100644
--- a/include/linux/qed/qed_if.h
+++ b/include/linux/qed/qed_if.h

@@ -1210,6 +1210,9 @@ struct qed_common_ops {
 	int (*get_esl_status)(struct qed_dev *cdev, bool *esl_active);
 };
 
+#define FW_REPO		\
+	"https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git"
+
 #define MASK_FIELD(_name, _value) \
 	((_value) &= (_name ## _MASK))
 

-- 
2.27.0

On Mon, Dec 27, 2021 at 09:56:56AM -0800, Venkata Sudheer Kumar Bhavaraju wrote:

If driver load failed due to request_firmware() not finding the device
firmware file, add prints that help remedy the situation.

Signed-off-by: Ariel Elior <redacted>
Signed-off-by: Alok Prasad <redacted>
Signed-off-by: Venkata Sudheer Kumar Bhavaraju <redacted>
---
Changes in v2:
 - Rename QED_FW_REPO to FW_REPO
 - Move FW_REPO macro to qed_if.h

Hi Venkata

When you decide to do something different to what has been requested,
it is a good idea to say why. There might be a very good reason for
this, but unless you explain it, i have no idea what it is.

   Andrew

Hi Venkata

When you decide to do something different to what has been requested,
it is a good idea to say why. There might be a very good reason for
this, but unless you explain it, i have no idea what it is.

   Andrew

Hello Andrew,

I moved the FW_REPO macro to qed_if.h under include/linux since I didn't
want to bloat something like include/linux/firmware.h. It's really used
(exact URL in a print after request_firmware() fails) at two other places.

If you think it's more useful in include/linux/firmware.h so that other
drivers can make use of it in future, I can move it there.

-Venkata

On Wed, 29 Dec 2021 03:02:32 -0800 Venkata Sudheer Kumar Bhavaraju
wrote:

quoted

Hi Venkata

When you decide to do something different to what has been requested,
it is a good idea to say why. There might be a very good reason for
this, but unless you explain it, i have no idea what it is.

I moved the FW_REPO macro to qed_if.h under include/linux since I didn't
want to bloat something like include/linux/firmware.h. It's really used
(exact URL in a print after request_firmware() fails) at two other places.

If you think it's more useful in include/linux/firmware.h so that other
drivers can make use of it in future, I can move it there.

If printing this information made sense it should have been done by the
core, not each driver. In fact your existing print:

 			DP_NOTICE(cdev,
 				  "Failed to find fw file - /lib/firmware/%s\n",
 				  QED_FW_FILE_NAME);

is redundant and potentially incorrect. Firmware path is configurable,
it does not have to be /lib/firmware. request_firmware() does not set
FW_OPT_NO_WARN, so core will already print the warning.

As for your current patch vast majority of users will get the firmware
from distro packages. I don't know why you're trying to print the repo
link.

On Wed, Dec 29, 2021 at 03:02:32AM -0800, Venkata Sudheer Kumar Bhavaraju wrote:

quoted

Hi Venkata

When you decide to do something different to what has been requested,
it is a good idea to say why. There might be a very good reason for
this, but unless you explain it, i have no idea what it is.

   Andrew

Hello Andrew,

I moved the FW_REPO macro to qed_if.h under include/linux since I didn't
want to bloat something like include/linux/firmware.h. It's really used
(exact URL in a print after request_firmware() fails) at two other places.

So you think the intel drivers are going to include qed_if.h? Don't
you think something vendor neutral would be better?

Anyway, from what Jakub is saying, you don't need the print at all.

	Andrew

Anyway, from what Jakub is saying, you don't need the print at all.

Thanks for the inputs. We can close this patch request.