[3/3] media: si2157: rework the firmware load logic

Commit Message

Mauro Carvalho Chehab Dec. 8, 2021, 10:13 a.m. UTC

Loading a firmware file should not be mandatory, as devices
could work with an eeprom firmware, if available.

Yet, using the eeprom firmware could lead into unpredictable
results, so the best is to warn about that.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
See [PATCH 0/3] at: https://lore.kernel.org/all/[e-mail]/

 drivers/media/tuners/si2157.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Comments

Robert Schlabbach Dec. 8, 2021, 10:37 p.m. UTC | #1

> Loading a firmware file should not be mandatory, as devices
> could work with an eeprom firmware, if available.
>
> Yet, using the eeprom firmware could lead into unpredictable
> results, so the best is to warn about that.

As there is no proof of an EEPROM being involved in any of
that, but strong evidence that all these devices actually have
an embedded firmware ROM, I propose changing that to "ROM"
instead.

> + bool warn_firmware_not_loaded = false;
> unsigned int chip_id, xtal_trim;
> - unsigned int fw_required;
> + bool fw_required = true;

To me, this is getting too ugly. All these per-model special
flags set somewhere in the code.

I propose removing BOTH these flags. Review of the SiLabs code
revealed:

1. ALL of the tuner models this driver supports have a firmware
   patch from SiLabs available.

2. NONE of them seems to require it. At least all the SiLabs
   drivers allow disabling the download.

So my proposal is:

1. Add firmware download support to all tuner models (this
   means adding some new firmware file names)

2. When a firmware file is not available, log an info (not
   warning) message and proceed.

None of the above boolean flags are needed then. The
dont_load_firmware flag from the config should of course be
kept as it is.

> + dev_warn(&client->dev, "firmware file '%s' not found. Using firmware from eeprom.\n",

Aside from using dev_info instead and changing the text to
"ROM firmware will be used.", this would also be a duplicate
message, as firmware_request() also logs a message when a
requested firmware file is not found.

So I propose also changing the firmware_request() call to
request_firmware_nowarn() instead to suppress the message
from the firmware loader.

Best Regards,
-Robert Schlabbach

Mauro Carvalho Chehab Dec. 9, 2021, 11:34 a.m. UTC | #2

Em Wed, 8 Dec 2021 23:37:33 +0100
Robert Schlabbach <[e-mail]> escreveu:

> > Loading a firmware file should not be mandatory, as devices
> > could work with an eeprom firmware, if available.
> >
> > Yet, using the eeprom firmware could lead into unpredictable
> > results, so the best is to warn about that.
> 
> As there is no proof of an EEPROM being involved in any of
> that, but strong evidence that all these devices actually have
> an embedded firmware ROM, I propose changing that to "ROM"
> instead.

Agreed. Will do such changes on patch 4, to be added to this series.

> > + bool warn_firmware_not_loaded = false;
> > unsigned int chip_id, xtal_trim;
> > - unsigned int fw_required;
> > + bool fw_required = true;
> 
> To me, this is getting too ugly. All these per-model special
> flags set somewhere in the code.
> 
> I propose removing BOTH these flags. Review of the SiLabs code
> revealed:
> 
> 1. ALL of the tuner models this driver supports have a firmware
>    patch from SiLabs available.

OK.

> 2. NONE of them seems to require it. At least all the SiLabs
>    drivers allow disabling the download.

Not true. if you check the code for si2148, it doesn't have
an option to skip firmware load.

The same is also true for other currently unsupported models.

> So my proposal is:
> 
> 1. Add firmware download support to all tuner models (this
>    means adding some new firmware file names)

Ok.

> 2. When a firmware file is not available, log an info (not
>    warning) message and proceed.

I guess this shouldn't be allowed for si2148 devices.

> None of the above boolean flags are needed then. The
> dont_load_firmware flag from the config should of course be
> kept as it is.
> 
> > + dev_warn(&client->dev, "firmware file '%s' not found. Using firmware from eeprom.\n",
> 
> Aside from using dev_info instead and changing the text to
> "ROM firmware will be used.", this would also be a duplicate
> message, as firmware_request() also logs a message when a
> requested firmware file is not found.
> 
> So I propose also changing the firmware_request() call to
> request_firmware_nowarn() instead to suppress the message
> from the firmware loader.

I can't see a request_firmware_nowarn() function, but year, the
printed messages can be simplified.

Thanks,
Mauro

Robert Schlabbach Dec. 9, 2021, 7:37 p.m. UTC | #3

> Not true. if you check the code for si2148, it doesn't have
> an option to skip firmware load.

You're right. I thought I had checked all code, but I must have
missed that one.

Or I was distracted by the fact that for Si2148 with romid 0x33,
a "dummy patch" is used, which according to the code comment
skips the firmware download and boots from NVM only. So I suppose
that version does not actually need the firmware...?!?

> I can't see a request_firmware_nowarn() function

Sorry, it's:

EXPORT_SYMBOL_GPL(firmware_request_nowarn);

They swapped the words around vs. the original function, for
whatever reason. Anyway, please use "firmware_request_nowarn()"
which does not log any message when the file is not found, so
that only the message logged from the si2157 shows up in the
kernel log.

Best Regards,
-Robert Schlabbach

Mauro Carvalho Chehab Dec. 10, 2021, 6:45 a.m. UTC | #4

Em Thu, 9 Dec 2021 20:37:29 +0100
Robert Schlabbach <[e-mail]> escreveu:

> > Not true. if you check the code for si2148, it doesn't have
> > an option to skip firmware load.  
> 
> You're right. I thought I had checked all code, but I must have
> missed that one.
> 
> Or I was distracted by the fact that for Si2148 with romid 0x33,
> a "dummy patch" is used, which according to the code comment
> skips the firmware download and boots from NVM only. So I suppose
> that version does not actually need the firmware...?!?

That's a good question. It sounds funny to have a "dummy patch"
loaded that would "skip firmware download", as the same would happen
without a firmware patch :-)

Hard to know for sure, but maybe the comment there was just outdated.
E. g. on a previous release it would have the code below such comment
also commented, but a new patch was then added, but someone forgot
to remove the comments.

> > I can't see a request_firmware_nowarn() function  
> 
> Sorry, it's:
> 
> EXPORT_SYMBOL_GPL(firmware_request_nowarn);

Ah ;-)

> They swapped the words around vs. the original function, for
> whatever reason. Anyway, please use "firmware_request_nowarn()"
> which does not log any message when the file is not found, so
> that only the message logged from the si2157 shows up in the
> kernel log.

Yeah, makes sense, especially since we'll be trying to load two
firmware files, at least for some of the tuners.

Thanks,
Mauro

Patch

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index ed28672c060d..5f4ae8593864 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -129,8 +129,9 @@  static int si2157_init(struct dvb_frontend *fe)
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
 	struct i2c_client *client = fe->tuner_priv;
 	struct si2157_dev *dev = i2c_get_clientdata(client);
+	bool warn_firmware_not_loaded = false;
 	unsigned int chip_id, xtal_trim;
-	unsigned int fw_required;
+	bool fw_required = true;
 	struct si2157_cmd cmd;
 	const char *fw_name;
 	int ret;
@@ -199,10 +200,6 @@  static int si2157_init(struct dvb_frontend *fe)
 	#define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0)
 	#define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0)
 
-	/* assume firmware is required, unless verified not to be */
-	/* only the SI2157_A30 has been verified not to yet */
-	fw_required = true;
-
 	switch (chip_id) {
 	case SI2158_A20:
 	case SI2148_A20:
@@ -212,9 +209,8 @@  static int si2157_init(struct dvb_frontend *fe)
 		fw_name = SI2141_A10_FIRMWARE;
 		break;
 	case SI2157_A30:
-		fw_name = SI2157_A30_FIRMWARE;
 		fw_required = false;
-		break;
+		fallthrough;
 	case SI2177_A30:
 		fw_name = SI2157_A30_FIRMWARE;
 		break;
@@ -237,12 +233,11 @@  static int si2157_init(struct dvb_frontend *fe)
 		goto skip_fw_download;
 
 	ret = si2157_load_firmware(fe, fw_name);
-	if (ret) {
-		if (!fw_required)
-			goto skip_fw_download;
-
-		dev_err(&client->dev, "firmware file '%s' not found\n",
-			fw_name);
+	if (fw_required && ret == -ENOENT)
+		warn_firmware_not_loaded = true;
+	else if (ret < 0) {
+		dev_err(&client->dev, "error %d when loading firmware file '%s'\n",
+			ret, fw_name);
 		goto err;
 	}
 
@@ -263,6 +258,11 @@  static int si2157_init(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
+	if (warn_firmware_not_loaded) {
+		dev_warn(&client->dev, "firmware file '%s' not found. Using firmware from eeprom.\n",
+			 fw_name);
+		warn_firmware_not_loaded = false;
+	}
 	dev_info(&client->dev, "firmware version: %c.%c.%d\n",
 			cmd.args[6], cmd.args[7], cmd.args[8]);
 
@@ -298,6 +298,11 @@  static int si2157_init(struct dvb_frontend *fe)
 	return 0;
 
 err:
+	if (warn_firmware_not_loaded)
+		dev_err(&client->dev,
+			"firmware file '%s' not found. Can't continue without a firmware.\n",
+			fw_name);
+
 	dev_dbg(&client->dev, "failed=%d\n", ret);
 	return ret;
 }