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

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

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