Em Wed, 8 Dec 2021 00:07:05 +0100 Robert Schlabbach <robert_s@xxxxxxx> escreveu: > Von: "Mauro Carvalho Chehab" <mchehab+huawei@xxxxxxxxxx> > > 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
