[PATCH 2/2] media: si2157: Add optional firmware download

[PATCH 2/2] media: si2157: Add optional firmware download

[Robert Schlabbach]

The Si2157 (A30) is functional with the ROM firmware 3.0.5, but can also
be patched at runtime, e.g. to firmware 3.1.3. However, although a
firmware filename for its firmware patch exists, that has only been used
for the Si2177 (A30) so far (which indeed takes the binary identical
firmware patch).

Add support for downloading firmware patches into the Si2157 (A30), but
make it optional, so that initialization can succeed with and without a
firmware patch being available. Keep the use of request_firmware() for
this purpose rather than firmware_request_nowarn(), so that the warning
in the log makes users aware that it is possible to provide a firmware
for this tuner.

The firmware patch is probably also optional for other (if not all)
tuners supported by the driver, but since I do not have the others
available to test, they are kept mandatory for now to avoid regressions.

Signed-off-by: Robert Schlabbach <robert_s@gmx.net>
---
 drivers/media/tuners/si2157.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 75ddf7ed1faf..757b27d1605a 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -85,6 +85,7 @@ static int si2157_init(struct dvb_frontend *fe)
 	struct si2157_cmd cmd;
 	const struct firmware *fw;
 	const char *fw_name;
+	unsigned int fw_required;
 	unsigned int chip_id, xtal_trim;

 	dev_dbg(&client->dev, "\n");
@@ -151,6 +152,10 @@ 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:
@@ -159,10 +164,13 @@ static int si2157_init(struct dvb_frontend *fe)
 	case SI2141_A10:
 		fw_name = SI2141_A10_FIRMWARE;
 		break;
+	case SI2157_A30:
+		fw_name = SI2157_A30_FIRMWARE;
+		fw_required = false;
+		break;
 	case SI2177_A30:
 		fw_name = SI2157_A30_FIRMWARE;
 		break;
-	case SI2157_A30:
 	case SI2147_A30:
 	case SI2146_A10:
 		fw_name = NULL;
@@ -184,6 +192,9 @@ static int si2157_init(struct dvb_frontend *fe)
 	/* request the firmware, this will block and timeout */
 	ret = request_firmware(&fw, fw_name, &client->dev);
 	if (ret) {
+		if (!fw_required)
+			goto skip_fw_download;
+
 		dev_err(&client->dev, "firmware file '%s' not found\n",
 				fw_name);
 		goto err;
--
2.17.1

Re: [PATCH 2/2] media: si2157: Add optional firmware download

[Mauro Carvalho Chehab]

Hi Robert,

Em Wed, 1 Dec 2021 22:10:58 +0100
Robert Schlabbach <robert_s@gmx.net> escreveu:

> The Si2157 (A30) is functional with the ROM firmware 3.0.5, but can also
> be patched at runtime, e.g. to firmware 3.1.3. However, although a
> firmware filename for its firmware patch exists, that has only been used
> for the Si2177 (A30) so far (which indeed takes the binary identical
> firmware patch).
> 
> Add support for downloading firmware patches into the Si2157 (A30), but
> make it optional, so that initialization can succeed with and without a
> firmware patch being available. Keep the use of request_firmware() for
> this purpose rather than firmware_request_nowarn(), so that the warning
> in the log makes users aware that it is possible to provide a firmware
> for this tuner.
> 
> The firmware patch is probably also optional for other (if not all)
> tuners supported by the driver, but since I do not have the others
> available to test, they are kept mandatory for now to avoid regressions.

This patch seems too risky. While I don't know the specifics of this
specific chipset, usually the ROM is on a separate chip that may or
may not be present. It may even have an unusable or problematic
firmware, depending on when the firmware was flashed.

What it could make sense, instead, would be to have a smarter error
logic that would:

	1. print an error/warn message if the firmware file was
	   not found;

	2. check if the device already come with a firmware that
	   it is known to work. On such case, allows the init
	   to proceed;

	3. if no firmware or too old/broken firmware, return an
	   error.

Regards,
Mauro

> 
> Signed-off-by: Robert Schlabbach <robert_s@gmx.net>
> ---
>  drivers/media/tuners/si2157.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> index 75ddf7ed1faf..757b27d1605a 100644
> --- a/drivers/media/tuners/si2157.c
> +++ b/drivers/media/tuners/si2157.c
> @@ -85,6 +85,7 @@ static int si2157_init(struct dvb_frontend *fe)
>  	struct si2157_cmd cmd;
>  	const struct firmware *fw;
>  	const char *fw_name;
> +	unsigned int fw_required;
>  	unsigned int chip_id, xtal_trim;
> 
>  	dev_dbg(&client->dev, "\n");
> @@ -151,6 +152,10 @@ 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:
> @@ -159,10 +164,13 @@ static int si2157_init(struct dvb_frontend *fe)
>  	case SI2141_A10:
>  		fw_name = SI2141_A10_FIRMWARE;
>  		break;
> +	case SI2157_A30:
> +		fw_name = SI2157_A30_FIRMWARE;
> +		fw_required = false;
> +		break;
>  	case SI2177_A30:
>  		fw_name = SI2157_A30_FIRMWARE;
>  		break;
> -	case SI2157_A30:
>  	case SI2147_A30:
>  	case SI2146_A10:
>  		fw_name = NULL;
> @@ -184,6 +192,9 @@ static int si2157_init(struct dvb_frontend *fe)
>  	/* request the firmware, this will block and timeout */
>  	ret = request_firmware(&fw, fw_name, &client->dev);
>  	if (ret) {
> +		if (!fw_required)
> +			goto skip_fw_download;
> +
>  		dev_err(&client->dev, "firmware file '%s' not found\n",
>  				fw_name);
>  		goto err;
> --
> 2.17.1
> 



Thanks,
Mauro

Aw: Re: [PATCH 2/2] media: si2157: Add optional firmware download

[Robert Schlabbach]

Von: "Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>
> This patch seems too risky. While I don't know the specifics of this
> specific chipset, usually the ROM is on a separate chip that may or
> may not be present. It may even have an unusable or problematic
> firmware, depending on when the firmware was flashed.

Hi Mauro, thanks for your review. Could you help me understand what
risk you mean?

Before this change _all_ users of this driver had to rely on the ROM
firmware, because the driver simply never downloaded any firmware
patches into it.

With my change, the following scenarios are possible:

1. The user has no si2157 firmware patch file in /lib/firmware. That
   will probably be the case for the vast majority of users, as this
   file is not found e.g. in linux-firmware.git.
   In this case the device will continue to work just as it did before,
   no regressions, no improvements.

2. The user has a valid si2157 firmware patch file in /lib/firmware,
   which is downloaded into the si2157. Should the user experience any
   issues with the updated firmware (which I think is rather unlikely,
   as I would expect SiLabs to have revoked it otherwise), simply
   deleting the firmware patch file from /lib/firmware will cure it
   and revert to scenario 1 above.

3. The user has an invalid si2157 firmware patch file in /lib/firmware,
   which looks "right" (to the file size check that's already been in
   the driver), but does not fit the si2157. I found that in this case,
   the si2157 will just operate with the original ROM firmware, i.e.
   also yield the same result as scenario 1 above.

I have tested all 3 scenarios on my Hauppauge WinTV-QuadHD, and the
result was a fully functional tuner in all cases. I haven't managed to
produce a scenario where things broke.

Could you sketch what risk you still see of things breaking/regressing
with my patch?

Best Regards,
-Robert Schlabbach

Re: [PATCH 2/2] media: si2157: Add optional firmware download

[Mauro Carvalho Chehab]

Em Wed, 8 Dec 2021 00:07:05 +0100
Robert Schlabbach <robert_s@gmx.net> escreveu:

> Von: "Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>
> > This patch seems too risky. While I don't know the specifics of this
> > specific chipset, usually the ROM is on a separate chip that may or
> > may not be present. It may even have an unusable or problematic
> > firmware, depending on when the firmware was flashed.  
> 
> Hi Mauro, thanks for your review. Could you help me understand what
> risk you mean?
> 
> Before this change _all_ users of this driver had to rely on the ROM
> firmware, because the driver simply never downloaded any firmware
> patches into it.
> 
> With my change, the following scenarios are possible:
> 
> 1. The user has no si2157 firmware patch file in /lib/firmware. That
>    will probably be the case for the vast majority of users, as this
>    file is not found e.g. in linux-firmware.git.
>    In this case the device will continue to work just as it did before,
>    no regressions, no improvements.
> 
> 2. The user has a valid si2157 firmware patch file in /lib/firmware,
>    which is downloaded into the si2157. Should the user experience any
>    issues with the updated firmware (which I think is rather unlikely,
>    as I would expect SiLabs to have revoked it otherwise), simply
>    deleting the firmware patch file from /lib/firmware will cure it
>    and revert to scenario 1 above.
> 
> 3. The user has an invalid si2157 firmware patch file in /lib/firmware,
>    which looks "right" (to the file size check that's already been in
>    the driver), but does not fit the si2157. I found that in this case,
>    the si2157 will just operate with the original ROM firmware, i.e.
>    also yield the same result as scenario 1 above.
> 
> I have tested all 3 scenarios on my Hauppauge WinTV-QuadHD, and the
> result was a fully functional tuner in all cases. I haven't managed to
> produce a scenario where things broke.
> 
> Could you sketch what risk you still see of things breaking/regressing
> with my patch?

The issue is that you tested it only on Hauppauge WinTV-QuadHD,
which seems to have an eeprom where the firmware is stored, based on
your report.

See, while the technical docs for this device are not publicity 
available, the block diagram for this device on its "advertising"
datasheet:
	https://media.digikey.com/pdf/Data%20Sheets/Silicon%20Laboratories%20PDFs/Si2157.pdf

Doesn't show any internal ROM/eeprom. So, it sounds to me that
either some external rom chip is needed or its firmware should
load via I2C.

While I'm not concerned about Hauppauge devices, there are a lot of
others manufacturers that won't add any optional eeproms, in order
to save a couple of bucks.

So, my main concern here is with regards to other devices that
are using si2157 driver. Among those:

- Some may have no eeproms;
- Some may have an eeprom with compatible firmware;
- Some may have an eeprom with a too old firmware.

The above scenarios don't have any relationship with the chip_id.
They actually depend on the device's release date and if the
manufacturer spent an extra money to have a device with an eeprom
and/or cared enough to update the firmware on its manufacturing
process. 

Also, if I remember well, with some versions of the firmware,
DVB-C won't work, due to incompatibility between the Linux driver
and the firmware version.

On other words, the only way to ensure that the device will
be in sane state and be fully supported by the driver is to
load a known-to work firmware.

---

Back to your patch...

Do you have access to all the technical datasheet and
application notes for all chips supported by this driver?

If you have, could you please describe why only SI2157_A30
is safe with regards to firmware?

If not, then checking for chip_id == SI2157_A30 makes no
sense.

The logic should, instead, do something similar to this:

	#define FIRMWARE_MIN_VERSION 0x123456 // FIXME: add something here
	unsigned int firmware_version;

	ret = request_firmware(&fw, fw_name, &client->dev);
 	if (ret) {
		/* Perhaps something else is needed before querying firmware version */

		/* query firmware version */
		memcpy(cmd.args, "\x11", 1);
		cmd.wlen = 1;
		cmd.rlen = 10;
		ret = si2157_cmd_execute(client, &cmd);
	        if (ret) {
			dev_err(&client->dev,
				"firmware file '%s' not found and no firmware at eeprom\n",
				fw_name);
		}

		firmware_version = cmd.args[6] << 16 + cmd.args[7] << 8 + cmd.args[8];

		if (firmware_version < FIRMWARE_MIN_VERSION) {
			dev_err(&client->dev,
				"firmware file '%s' not found and eeprom firmware version %c.%c.%c is too old\n",
				cmd.args[6], cmd.args[7], cmd.args[8], fw_name);
			goto err;
		}
			
		dev_err(&client->dev,
			"firmware file '%s' not found, using firmware version %c.%c.%c from EEPROM.\n",
			cmd.args[6], cmd.args[7], cmd.args[8], fw_name);
	}


Thanks,
Mauro

Aw: Re: [PATCH 2/2] media: si2157: Add optional firmware download

[Robert Schlabbach]

Von: "Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>

> See, while the technical docs for this device are not publicity
> available, the block diagram for this device on its "advertising"
> datasheet:
> https://media.digikey.com/pdf/Data%20Sheets/Silicon%20Laboratories%20PDFs/Si2157.pdf
> 
> Doesn't show any internal ROM/eeprom. So, it sounds to me that
> either some external rom chip is needed or its firmware should
> load via I2C.

I am very certain that the Silabs Si21xx tuners and demodulators
all have an embedded firmware ROM. Unfortunately, I cannot find a
public datasheet that explicitly mentions this, so I can only
provide "circumstantial evidence":

- There are very simple USB TV sticks out there e.g. with only a
  Cypress FX2LP on them and SiLabs tuner/demod. Those work in
  Linux without the Linux driver downloading a firmware onto the
  tuner. Where else would the tuner firmware then come from? The
  FX2LP firmware does not have such function, the demod does not
  do that either, and the datasheet you found also does not show
  that the tuner would be capable of downloading it from an
  attached EEPROM either.

- The last two digits of the part number are actually the ROM
  ROM firmware version (e.g. Si2157-A30 has firmware 3.0[.5],
  or Si2183-B60 has firmware 6.0[.2]).

- And one little hint "officialy" from SiLabs, in their driver
  code at:

  https://github.com/SiliconLabs/video_si21xx_superset/blob/master/SILABS_SUPERSET/TER/Si2157/Si2157_Commands.h

  Note that Si2157_PART_INFO_CMD_REPLY_struct has a field
  "rom_id" in it. So it must have some sort of ROM.

> So, my main concern here is with regards to other devices that
> are using si2157 driver. Among those:
> 
> - Some may have no eeproms;
> - Some may have an eeprom with compatible firmware;
> - Some may have an eeprom with a too old firmware.

I think it's very unlikely that ANY device out there actually has
an EEPROM with Si2157 firmware on it.

> On other words, the only way to ensure that the device will
> be in sane state and be fully supported by the driver is to
> load a known-to work firmware.

Ah, that's actually a different point, which I agree is valid:
The driver code must match the firmware API version running on
the tuner. So if a very different firmware was running on the
tuner, the Linux driver might not be compatible with it.

There are two ways to go about this:

1. Support only one specific firmware version in the driver
   and error out in init if a potentially required firmware
   file is not available.

2. Being more tolerant about this and only outputting a
   warning that the firmware running on the tuner does not
   match the version the driver was developed/tested against
   and might not work right, and that providing a firmware
   patch file might fix that.

I would prefer option 2 as it is more user-friendly.

> Do you have access to all the technical datasheet and
> application notes for all chips supported by this driver?

I wish. AFAIK, Antti developed the Linux drivers using
reverse engineering of the Windows drivers.

The situation is a bit better now, as SiLabs has now
published their own driver source code:

https://github.com/SiliconLabs/video_si21xx_superset

Ideally, someone would have a lot of spare time to
shuffle through that source code and e.g. correct some
incorrect command or parameter descriptions in the
Linux driver code...

> If you have, could you please describe why only SI2157_A30
> is safe with regards to firmware?
> 
> If not, then checking for chip_id == SI2157_A30 makes no
> sense.

The _existing_ logic is that if chip_id == SI2157_A30, no
firmware will ever be downloaded into the chip. My change
made it possible to _optionally_ download firmware into the
chip, but also proceeding without firmware, behaving
exactly as before. So it is "safe" with regards of ensuring
there are no regressions introduced in the Linux driver.

It is not "safe" with regards to _improving_ the existing
firmware handling, but that was not my goal. If you want to
expand the scope, you're welcome.

But I think what you are proposing is much more risky than
my patch, if only because you're touching far more code
lines.

> #define FIRMWARE_MIN_VERSION 0x123456 // FIXME: add something here

No, this will have to be far more complex:

1. The driver supports several Si21xx tuners (note that the
   SiLabs code has one driver per model), mostly having
   different firmware versions, so you will have to provide
   firmware versions for every tuner model supported by the
   driver.

2. You might also want to provide at least a "range" of
   supported/tested firmware versions, if not even a full
   list of firmware versions.

However, I consider that an almost impossible undertaking.
Where are you going to get that list of firmware versions
from? Do you plan to have many, many contributors filling
that list patch by patch?

I'm not sure the benefit would be worth the effort.

Best Regards,
-Robert Schlabbach

[PATCH 0/3] media: si2157: rework firmware load logic

[Mauro Carvalho Chehab]

As pointed bt Robert, if there's already a firmware for si2157 at
/lib/firmware, it is preferred to use it, instead of the default firmware.

Yet, when this is missed, the driver may still work. That's the case,
for instance, of the devices explicitly flagged with dont_load_firmware
(currently, Terratec Cinergy TC2).

So, rework the firmware logic in a way that it will emit a warning for
devices that are using the eeprom firmware and may not work well
with it.

Robert,

This was compile-tested only. Could you please test it on your device?

Mauro Carvalho Chehab (2):
  media: si2157: move firmware load to a separate function
  media: si2157: rework the firmware load logic

Robert Schlabbach (1):
  media: si2157: Add optional firmware download

 drivers/media/tuners/si2157.c | 120 +++++++++++++++++++++-------------
 1 file changed, 75 insertions(+), 45 deletions(-)

-- 
2.33.1

[PATCH 1/3] media: si2157: move firmware load to a separate function

[Mauro Carvalho Chehab]

Split the firmware load code from si2157_init, in order to
help to add further changes at the way firmware is handled on
this device.

No functional changes.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

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/cover.1638958050.git.mchehab+huawei@kernel.org/

 drivers/media/tuners/si2157.c | 98 ++++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 42 deletions(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 75ddf7ed1faf..481a5db7fb69 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -76,16 +76,63 @@ static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd *cmd)
 	return ret;
 }
 
-static int si2157_init(struct dvb_frontend *fe)
+static int si2157_load_firmware(struct dvb_frontend *fe,
+				const char *fw_name)
 {
 	struct i2c_client *client = fe->tuner_priv;
-	struct si2157_dev *dev = i2c_get_clientdata(client);
-	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-	int ret, len, remaining;
-	struct si2157_cmd cmd;
 	const struct firmware *fw;
-	const char *fw_name;
+	int ret, len, remaining;
+	struct si2157_cmd cmd;
+
+	/* request the firmware, this will block and timeout */
+	ret = request_firmware(&fw, fw_name, &client->dev);
+	if (ret)
+		return ret;
+
+	/* firmware should be n chunks of 17 bytes */
+	if (fw->size % 17 != 0) {
+		dev_err(&client->dev, "firmware file '%s' is invalid\n",
+			fw_name);
+		ret = -EINVAL;
+		goto err_release_firmware;
+	}
+
+	dev_info(&client->dev, "downloading firmware from file '%s'\n",
+		 fw_name);
+
+	for (remaining = fw->size; remaining > 0; remaining -= 17) {
+		len = fw->data[fw->size - remaining];
+		if (len > SI2157_ARGLEN) {
+			dev_err(&client->dev, "Bad firmware length\n");
+			ret = -EINVAL;
+			goto err_release_firmware;
+		}
+		memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
+		cmd.wlen = len;
+		cmd.rlen = 1;
+		ret = si2157_cmd_execute(client, &cmd);
+		if (ret) {
+			dev_err(&client->dev, "firmware download failed %d\n",
+					ret);
+			goto err_release_firmware;
+		}
+	}
+
+err_release_firmware:
+	release_firmware(fw);
+
+	return ret;
+}
+
+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);
 	unsigned int chip_id, xtal_trim;
+	struct si2157_cmd cmd;
+	const char *fw_name;
+	int ret;
 
 	dev_dbg(&client->dev, "\n");
 
@@ -181,45 +228,13 @@ static int si2157_init(struct dvb_frontend *fe)
 	if (fw_name == NULL)
 		goto skip_fw_download;
 
-	/* request the firmware, this will block and timeout */
-	ret = request_firmware(&fw, fw_name, &client->dev);
+	ret = si2157_load_firmware(fe, fw_name);
 	if (ret) {
 		dev_err(&client->dev, "firmware file '%s' not found\n",
-				fw_name);
-		goto err;
-	}
-
-	/* firmware should be n chunks of 17 bytes */
-	if (fw->size % 17 != 0) {
-		dev_err(&client->dev, "firmware file '%s' is invalid\n",
-				fw_name);
-		ret = -EINVAL;
-		goto err_release_firmware;
-	}
-
-	dev_info(&client->dev, "downloading firmware from file '%s'\n",
 			fw_name);
-
-	for (remaining = fw->size; remaining > 0; remaining -= 17) {
-		len = fw->data[fw->size - remaining];
-		if (len > SI2157_ARGLEN) {
-			dev_err(&client->dev, "Bad firmware length\n");
-			ret = -EINVAL;
-			goto err_release_firmware;
-		}
-		memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
-		cmd.wlen = len;
-		cmd.rlen = 1;
-		ret = si2157_cmd_execute(client, &cmd);
-		if (ret) {
-			dev_err(&client->dev, "firmware download failed %d\n",
-					ret);
-			goto err_release_firmware;
-		}
+		goto err;
 	}
 
-	release_firmware(fw);
-
 skip_fw_download:
 	/* reboot the tuner with new firmware? */
 	memcpy(cmd.args, "\x01\x01", 2);
@@ -270,8 +285,7 @@ static int si2157_init(struct dvb_frontend *fe)
 
 	dev->active = true;
 	return 0;
-err_release_firmware:
-	release_firmware(fw);
+
 err:
 	dev_dbg(&client->dev, "failed=%d\n", ret);
 	return ret;
-- 
2.33.1

Re: [PATCH 1/3] media: si2157: move firmware load to a separate function

[Robert Schlabbach]

> @@ -181,45 +228,13 @@  static int si2157_init(struct dvb_frontend *fe)
>  	if (fw_name == NULL)
>  		goto skip_fw_download;

You can invert the condition now and put the si2157_load_firmware() call
inside the if () block, and do away with the goto.

> -	/* request the firmware, this will block and timeout */
> -	ret = request_firmware(&fw, fw_name, &client->dev);
> +	ret = si2157_load_firmware(fe, fw_name);
>  	if (ret) {
>  		dev_err(&client->dev, "firmware file '%s' not found\n",

This will produce duplicate error messages, because the called function
will already output some error messages. You should move this line to
the extracted function as well, and reduce the code in the init function
to:

if (fw_name != null) {
        ret = si2157_load_firmware(fe, fw_name);
        if (ret)
                goto err;
}

Best Regards,
-Robert Schlabbach

Re: [PATCH 1/3] media: si2157: move firmware load to a separate function

[Mauro Carvalho Chehab]

Em Wed, 8 Dec 2021 17:40:56 +0100
Robert Schlabbach <robert_s@gmx.net> escreveu:

> > @@ -181,45 +228,13 @@  static int si2157_init(struct dvb_frontend *fe)
> >  	if (fw_name == NULL)
> >  		goto skip_fw_download;  
> 
> You can invert the condition now and put the si2157_load_firmware() call
> inside the if () block, and do away with the goto.

I know, but this would also require to move the dont_load_firmware check,
which also does the goto.

I did that on a first version of the patch, but ended reverting,
as I can't be 100% certain devices with dont_load_firmware:

	if ((le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_DEXATEK &&
	     le16_to_cpu(d->udev->descriptor.idProduct) == 0x0100) ||
	    (le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_TERRATEC &&
	     le16_to_cpu(d->udev->descriptor.idProduct) == USB_PID_TERRATEC_CINERGY_TC2_STICK))
		si2157_config.dont_load_firmware = true;

Would keep work and report the hardware type/review. 

> > -	/* request the firmware, this will block and timeout */
> > -	ret = request_firmware(&fw, fw_name, &client->dev);
> > +	ret = si2157_load_firmware(fe, fw_name);
> >  	if (ret) {
> >  		dev_err(&client->dev, "firmware file '%s' not found\n",  
> 
> This will produce duplicate error messages, because the called function
> will already output some error messages. You should move this line to
> the extracted function as well, and reduce the code in the init function
> to:
> 
> if (fw_name != null) {
>         ret = si2157_load_firmware(fe, fw_name);
>         if (ret)
>                 goto err;
> }

True, but I guess patch 3 fixes it.

On patch 1, my goal were just to place everything on a single routine
without any real changes.  Patch 3 does the optimization/cleanup.

Thanks,
Mauro

[PATCH 2/3] media: si2157: Add optional firmware download

[Mauro Carvalho Chehab]

From: Robert Schlabbach <robert_s@gmx.net>

The Si2157 (A30) is functional with the ROM firmware 3.0.5, but can also
be patched at runtime, e.g. to firmware 3.1.3. However, although a
firmware filename for its firmware patch exists, that has only been used
for the Si2177 (A30) so far (which indeed takes the binary identical
firmware patch).

Add support for downloading firmware patches into the Si2157 (A30), but
make it optional, so that initialization can succeed with and without a
firmware patch being available. Keep the use of request_firmware() for
this purpose rather than firmware_request_nowarn(), so that the warning
in the log makes users aware that it is possible to provide a firmware
for this tuner.

The firmware patch is probably also optional for other (if not all)
tuners supported by the driver, but since I do not have the others
available to test, they are kept mandatory for now to avoid regressions.

Signed-off-by: Robert Schlabbach <robert_s@gmx.net>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

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/cover.1638958050.git.mchehab+huawei@kernel.org/

 drivers/media/tuners/si2157.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 481a5db7fb69..ed28672c060d 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -130,6 +130,7 @@ static int si2157_init(struct dvb_frontend *fe)
 	struct i2c_client *client = fe->tuner_priv;
 	struct si2157_dev *dev = i2c_get_clientdata(client);
 	unsigned int chip_id, xtal_trim;
+	unsigned int fw_required;
 	struct si2157_cmd cmd;
 	const char *fw_name;
 	int ret;
@@ -198,6 +199,10 @@ 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:
@@ -206,10 +211,13 @@ static int si2157_init(struct dvb_frontend *fe)
 	case SI2141_A10:
 		fw_name = SI2141_A10_FIRMWARE;
 		break;
-	case SI2177_A30:
-		fw_name = SI2157_A30_FIRMWARE;
-		break;
 	case SI2157_A30:
+		fw_name = SI2157_A30_FIRMWARE;
+		fw_required = false;
+		break;
+	case SI2177_A30:
+		fw_name = SI2157_A30_FIRMWARE;
+		break;
 	case SI2147_A30:
 	case SI2146_A10:
 		fw_name = NULL;
@@ -230,6 +238,9 @@ static int si2157_init(struct dvb_frontend *fe)
 
 	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);
 		goto err;
-- 
2.33.1

Re: [PATCH 2/3] media: si2157: Add optional firmware download

[Robert Schlabbach]

> 	ret = si2157_load_firmware(fe, fw_name);
>  	if (ret) {
> +		if (!fw_required)
> +			goto skip_fw_download;
> +

In conjunction with my proposal for PATCH 1/3, this can be simplified to:

ret = si2157_load_firmware(fe, fw_name);
if (ret && fw_required)
        goto err;

Best Regards,
-Robert Schlabbach

Re: [PATCH 2/3] media: si2157: Add optional firmware download

[Mauro Carvalho Chehab]

Em Wed, 8 Dec 2021 17:45:43 +0100
Robert Schlabbach <robert_s@gmx.net> escreveu:

> > 	ret = si2157_load_firmware(fe, fw_name);
> >  	if (ret) {
> > +		if (!fw_required)
> > +			goto skip_fw_download;
> > +  
> 
> In conjunction with my proposal for PATCH 1/3, this can be simplified to:
> 
> ret = si2157_load_firmware(fe, fw_name);
> if (ret && fw_required)
>         goto err;

See the patch 3:

	ret = si2157_load_firmware(fe, 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);

Basically, it will do (about) the same thing you proposed, with one
important difference: It should only ignore errors loading the firmware
when the error is due to a non-existing firmware file. If the firmware
file is corrupted or can't be load for other reasons (ENOMEM, corrupted
file, etc), it will print the error code and bail out.

Thanks,
Mauro

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

[Mauro Carvalho Chehab]

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 <mchehab+huawei@kernel.org>
---

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/cover.1638958050.git.mchehab+huawei@kernel.org/

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

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;
 }
-- 
2.33.1

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

[Robert Schlabbach]

> 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

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

[Mauro Carvalho Chehab]

Em Wed, 8 Dec 2021 23:37:33 +0100
Robert Schlabbach <robert_s@gmx.net> 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

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

[Robert Schlabbach]

> 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

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

[Mauro Carvalho Chehab]

Em Thu, 9 Dec 2021 20:37:29 +0100
Robert Schlabbach <robert_s@gmx.net> 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