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

Commit Message

Robert Schlabbach Dec. 1, 2021, 9:10 p.m. UTC

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 <[e-mail]>
---
 drivers/media/tuners/si2157.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

--
2.17.1

Comments

Mauro Carvalho Chehab Dec. 6, 2021, 2 p.m. UTC | #1

Hi Robert,

Em Wed, 1 Dec 2021 22:10:58 +0100
Robert Schlabbach <[e-mail]> 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 <[e-mail]>
> ---
>  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

Robert Schlabbach Dec. 7, 2021, 11:07 p.m. UTC | #2

Von: "Mauro Carvalho Chehab" <[e-mail]>
> 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

Mauro Carvalho Chehab Dec. 8, 2021, 8:52 a.m. UTC | #3

Em Wed, 8 Dec 2021 00:07:05 +0100
Robert Schlabbach <[e-mail]> escreveu:

> Von: "Mauro Carvalho Chehab" <[e-mail]>
> > 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

Robert Schlabbach Dec. 8, 2021, 3:42 p.m. UTC | #4

Von: "Mauro Carvalho Chehab" <[e-mail]>

> 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

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;