Re: [PATCH v2] watchdog: ziirave_wdt: Add support to upload the firmware.

[Enric Balletbo Serra]

Hi Guenter,

Many thanks for the review, I'm agree with your comments so I'll apply
the changes in next version that hopefully I'll send soon. Just some
questions below.

2016-07-21 5:42 GMT+02:00 Guenter Roeck <linux@xxxxxxxxxxxx>:
> On 07/20/2016 03:31 AM, Enric Balletbo i Serra wrote:
>>
>> This patch adds and entry to the sysfs to start firmware upload process
>> on the specified device with the requested firmware.
>>
>> The uploading of the firmware needs only to happen once per firmware
>> upgrade, as the firmware is stored in persistent storage. If the
>> firmware upload or the firmware verification fails then we print and
>> error message and exit.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>> ---
>>
>> Changes since v1: (requested by Martyn)
>>   - Remove two defines that are not used.
>>   - Fix typo in documentation count -> length
>>
>>   drivers/watchdog/ziirave_wdt.c | 421
>> +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 421 insertions(+)
>>
>> diff --git a/drivers/watchdog/ziirave_wdt.c
>> b/drivers/watchdog/ziirave_wdt.c
>> index fa1efef..1a37577 100644
>> --- a/drivers/watchdog/ziirave_wdt.c
>> +++ b/drivers/watchdog/ziirave_wdt.c
>> @@ -18,7 +18,10 @@
>>    * GNU General Public License for more details.
>>    */
>>
>> +#include <linux/ctype.h>
>> +#include <linux/delay.h>
>>   #include <linux/i2c.h>
>> +#include <linux/firmware.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/slab.h>
>> @@ -36,6 +39,8 @@
>>   #define ZIIRAVE_STATE_OFF     0x1
>>   #define ZIIRAVE_STATE_ON      0x2
>>
>> +#define ZIIRAVE_FW_NAME                "ziirave_fw.hex"
>> +
>>   static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL,
>> NULL,
>>                                   "host request", NULL, "illegal
>> configuration",
>>                                   "illegal instruction", "illegal trap",
>> @@ -50,6 +55,25 @@ static char *ziirave_reasons[] = {"power cycle", "hw
>> watchdog", NULL, NULL,
>>   #define ZIIRAVE_WDT_PING              0x9
>>   #define ZIIRAVE_WDT_RESET_DURATION    0xa
>>
>> +#define ZIIRAVE_FIRM_PKT_TOTAL_SIZE    20
>> +#define ZIIRAVE_FIRM_PKT_DATA_SIZE     16
>> +#define ZIIRAVE_FIRM_FLASH_MEMORY_START        0x1600
>> +#define ZIIRAVE_FIRM_FLASH_MEMORY_END  0x2bbf
>> +
>> +/* Received and ready for next Download packet. */
>> +#define ZIIRAVE_FIRM_DOWNLOAD_ACK      1
>> +/* Currently writing to flash. Retry Download status in a moment! */
>> +#define ZIIRAVE_FIRM_DOWNLOAD_BUSY     2
>> +
>> +/* Firmware commands */
>> +#define ZIIRAVE_CMD_DOWNLOAD_START             0x10
>> +#define ZIIRAVE_CMD_DOWNLOAD_END               0x11
>> +#define ZIIRAVE_CMD_DOWNLOAD_SET_READ_ADDR     0x12
>> +#define ZIIRAVE_CMD_DOWNLOAD_READ_BYTE         0x13
>> +#define ZIIRAVE_CMD_RESET_PROCESSOR            0x0b
>> +#define ZIIRAVE_CMD_JUMP_TO_BOOTLOADER         0x0c
>> +#define ZIIRAVE_CMD_DOWNLOAD_PACKET            0x0e
>> +
>>   struct ziirave_wdt_rev {
>>         unsigned char major;
>>         unsigned char minor;
>> @@ -62,6 +86,23 @@ struct ziirave_wdt_data {
>>         int reset_reason;
>>   };
>>
>> +/*
>> + * A packet to send to the firmware is composed by following bytes:
>> + *     Length | Addr0 | Addr1 | Data0 .. Data15 | Checksum |
>> + * Where,
>> + *     Length: A data byte containing the length of the data.
>> + *     Addr0: Low byte of the address.
>> + *     Addr1: High byte of the address.
>> + *     Data0 .. Data15: Array of 16 bytes of data.
>> + *     Checksum: Checksum byte to verify data integrity.
>> + */
>> +struct ziirave_fw_pkt_t {
>> +       u8 length;
>> +       u16 addr;
>> +       u8 data[ZIIRAVE_FIRM_PKT_DATA_SIZE];
>> +       u8 checksum;
>> +} __packed;
>> +
>>   static int wdt_timeout;
>>   module_param(wdt_timeout, int, 0);
>>   MODULE_PARM_DESC(wdt_timeout, "Watchdog timeout in seconds");
>> @@ -146,6 +187,339 @@ static unsigned int ziirave_wdt_get_timeleft(struct
>> watchdog_device *wdd)
>>         return ret;
>>   }
>>
>> +static int ziirave_firm_wait_for_ack(struct watchdog_device *wdd)
>> +{
>> +       struct i2c_client *client = to_i2c_client(wdd->parent);
>> +       int ret;
>> +
>> +       do {
>> +               usleep_range(5000, 5100);
>> +               ret = i2c_smbus_read_byte(client);
>> +               if (ret < 0) {
>> +                       dev_err(&client->dev, "Failed to read byte\n");
>> +                       return ret;
>> +               }
>> +       } while (ret == ZIIRAVE_FIRM_DOWNLOAD_BUSY);
>> +
>> +       return ret == ZIIRAVE_FIRM_DOWNLOAD_ACK ? 0 : -EIO;
>> +}
>> +
>> +/*
>> + * Parse Microchip format Hex file (an extended Intel Hex file) to
>> extract
>> + * address and data.
>> + */
>> +static int ziirave_firm_parse_hex_line(unsigned char *fw_data,
>> +                                      struct ziirave_fw_pkt_t *fw_pkt,
>> +                                      bool *addr_has_changed)
>> +{
>> +       int count = 0;
>> +       unsigned char *src, dst;
>> +
>> +       if (*fw_data++ != ':')
>> +               return -EFAULT;
>
>
> Why is this a bad address ? Something like -EINVAL might be more
> appropriate.
>
>> +
>> +       /* locate end of line */
>> +       for (src = fw_data; *src != '\n'; src += 2) {
>
>
> What if there is no newline at the end ? What if fw_data is corrupt and
> contains nonsense ?
>
> To keep going until a non-hex character is found is not really robust.
>
>
>> +               /*
>> +                * Convert the ascii hexadecimal string to its binary
>> +                * representation
>> +                */
>> +               if (hex2bin(&dst, src, 1))
>> +                       return -EINVAL;
>> +
>> +               /* Parse line to split addr / data */
>> +               switch (count) {
>> +               case 0:
>> +                       fw_pkt->length = dst;
>> +                       break;
>> +               case 1:
>> +                       fw_pkt->addr = dst << 8;
>> +                       break;
>> +               case 2:
>> +                       fw_pkt->addr |= dst;
>> +                       fw_pkt->addr >>= 1;
>> +                       break;
>> +               case 3:
>> +                       /* check if data is an address */
>> +                       if (dst == 0x04)
>> +                               *addr_has_changed = true;
>> +                       else
>> +                               *addr_has_changed = false;
>> +                       break;
>> +               case  4:
>> +               case  5:
>> +                       if (!*addr_has_changed)
>> +                               fw_pkt->data[(count - 4)] = dst;
>
>
> Really unnecessary ( ).
>
>> +                       break;
>> +               default:
>> +                       fw_pkt->data[(count - 4)] = dst;
>> +                       break;
>
>
> I am really not a friend of passing an intel hex file to the kernel.
> It is quite easy to construct a file which messes up the kernel, unless
> a lot of care is put into its parsing, as evidenced here:
> Construct a file which results in count >= 16 and see what happens.
>
> You'll have to make this way more robust to make it acceptable.
> You might want to consider converting the file into a binary file
> _before_ passing it to the kernel.
>

Ok, got it, sounds good I'll do this in next version.

> Thanks,
> Guenter
>
>> +               }
>> +               count++;
>> +       }
>> +
>> +       /* return read value + ':' + '\n' */
>> +       return (count * 2) + 2;
>> +}
>> +
>> +static int ziirave_firm_set_read_addr(struct watchdog_device *wdd, u16
>> addr)
>> +{
>> +       struct i2c_client *client = to_i2c_client(wdd->parent);
>> +       u8 address[2];
>> +
>> +       memset(address, 0, sizeof(address));
>
>
> you are filling in address in the next two instructions. This is
> unnecessary.
>
>
>> +       address[0] = addr & 0xff;
>> +       address[1] = (addr >> 8) & 0xff;
>> +
>> +       return i2c_smbus_write_block_data(client,
>> +
>> ZIIRAVE_CMD_DOWNLOAD_SET_READ_ADDR,
>> +                                         ARRAY_SIZE(address), address);
>> +}
>> +
>> +static int ziirave_firm_write_block_data(struct watchdog_device *wdd,
>> +                                        u8 command, u8 length, const u8
>> *data,
>> +                                        bool wait_for_ack)
>> +{
>> +       struct i2c_client *client = to_i2c_client(wdd->parent);
>> +       int ret;
>> +
>> +       ret = i2c_smbus_write_block_data(client, command, length, data);
>> +
>> +       if (ret) {
>> +               dev_err(&client->dev,
>> +                       "Failed to send command 0x%02x: %d\n", command,
>> ret);
>> +               return ret;
>> +       }
>> +
>> +       if (wait_for_ack) {
>> +               ret = ziirave_firm_wait_for_ack(wdd);
>> +               if (ret)
>> +                       dev_err(&client->dev,
>> +                               "Failed waiting ACK from command 0x%02x:
>> %d\n",
>> +                               command, ret);
>
>
> This is really quite noisy. Since the error is reported to user space,
> I would suggest to either drop the messages or make them debug messages.
>

If it's ok I would prefer transform to a debug message.

>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static int ziirave_firm_write_byte(struct watchdog_device *wdd, u8
>> command,
>> +                                  u8 byte, bool wait_for_ack)
>> +{
>> +       return ziirave_firm_write_block_data(wdd, command, 1, &byte,
>> +                                            wait_for_ack);
>> +}
>> +
>> +static int ziirave_firm_write_pkt(struct watchdog_device *wdd,
>> +                                 struct ziirave_fw_pkt_t *fw_pkt)
>> +{
>> +       struct i2c_client *client = to_i2c_client(wdd->parent);
>> +       u8 i, checksum = 0, packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE];
>> +       int ret;
>> +
>> +       memset(&packet, 0, sizeof(packet));
>
>
> & is unnecessary here, and is this really necessary ?
>
>> +
>> +       packet[0] = fw_pkt->length;
>> +       packet[1] = (u8)(fw_pkt->addr & 0xff);
>> +       packet[2] = (u8)((fw_pkt->addr & 0xff00) >> 8);
>> +
>> +       /* Copy packet data */
>> +       memcpy(packet + 3, fw_pkt->data, fw_pkt->length);
>
>
> length is unchecked when written and may be up to 255.
>
>> +
>> +       /* Compute checksum */
>> +       for (i = 0; i < (ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1); i++)
>
>
> Unnecessary ( ).
>
>
>> +               checksum += packet[i];
>> +       packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1] = checksum;
>> +
>> +       ret = ziirave_firm_write_block_data(wdd,
>> ZIIRAVE_CMD_DOWNLOAD_PACKET,
>> +                                           ARRAY_SIZE(packet), packet,
>> true);
>> +       if (ret)
>> +               dev_err(&client->dev,
>> +                     "Failed to write firmware packet at address 0x%04x:
>> %d\n",
>> +                     fw_pkt->addr, ret);
>> +
>> +       return ret;
>> +}
>> +
>> +static int ziirave_firm_verify(struct watchdog_device *wdd,
>> +                              const struct firmware *fw)
>> +{
>> +       struct i2c_client *client = to_i2c_client(wdd->parent);
>> +       int i, ret, read_bytes = 0, total_read_bytes = 0;
>> +       bool addr_has_changed;
>> +       struct ziirave_fw_pkt_t fw_pkt;
>> +       u8 data[ZIIRAVE_FIRM_PKT_DATA_SIZE];
>> +
>> +       while (total_read_bytes < fw->size) {
>> +               read_bytes = ziirave_firm_parse_hex_line(
>> +                                       (u8 *)(fw->data +
>> total_read_bytes),
>> +                                       &fw_pkt, &addr_has_changed);
>> +
>
> Error returns are ignored, in effect reducing total_read_bytes.
>
>> +               /* Detect the end of file */
>> +               total_read_bytes += read_bytes;
>> +               if (total_read_bytes == fw->size)
>> +                       break;
>> +
>
> ziirave_firm_parse_hex_line() doesn't know anything about the size of the
> file.
> Thus, total_read_bytes can become larger than fw->size. Also, doesn't this
> ignore
> the last line ?
>
>> +               if (addr_has_changed)
>> +                       continue;
>> +
>> +               if (fw_pkt.addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
>> +                   fw_pkt.addr > ZIIRAVE_FIRM_FLASH_MEMORY_END)
>> +                       continue;
>> +
>
> What if addr >= ZIIRAVE_FIRM_FLASH_MEMORY_END - ARRAY_SIZE(data) ?
> Wouldn't this read beyond the end of the firmware ?
>
>
>> +               ret = ziirave_firm_set_read_addr(wdd, fw_pkt.addr);
>> +               if (ret) {
>> +                       dev_err(&client->dev,
>> +                               "Failed to send SET_READ_ADDR command:
>> %d\n",
>> +                               ret);
>> +                       return ret;
>> +               }
>> +
>> +               for (i = 0; i < ARRAY_SIZE(data); i++) {
>> +                       ret = i2c_smbus_read_byte_data(client,
>> +
>> ZIIRAVE_CMD_DOWNLOAD_READ_BYTE);
>> +                       if (ret < 0) {
>> +                               dev_err(&client->dev,
>> +                                       "Failed to READ DATA: %d\n", ret);
>> +                               return ret;
>> +                       }
>> +                       data[i] = (u8)ret;
>> +               }
>> +
>> +               if (memcmp(data, fw_pkt.data, fw_pkt.length)) {
>> +                       dev_err(&client->dev,
>> +                               "Firmware mismatch at address 0x%04x\n",
>> +                               fw_pkt.addr);
>> +                       return -EINVAL;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int ziirave_firm_upload(struct watchdog_device *wdd,
>> +                              const struct firmware *fw)
>> +{
>> +       struct i2c_client *client = to_i2c_client(wdd->parent);
>> +       struct ziirave_fw_pkt_t fw_pkt;
>> +       struct ziirave_fw_pkt_t fw_pkt_new;
>> +       int ret, total_read_bytes = 0, words_till_page_break;
>> +       bool addr_has_changed;
>> +
>> +       ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_JUMP_TO_BOOTLOADER,
>> 1,
>> +                                     false);
>> +       if (ret)
>> +               return ret;
>> +
>> +       msleep(500);
>> +
>> +       ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_START, 1,
>> true);
>> +       if (ret)
>> +               return ret;
>> +
>> +       msleep(500);
>> +
>> +       while (total_read_bytes < fw->size) {
>> +               int read_bytes = 0;
>> +
>> +               read_bytes = ziirave_firm_parse_hex_line(
>> +                                       (u8 *)(fw->data +
>> total_read_bytes),
>> +                                       &fw_pkt, &addr_has_changed);
>> +
>> +               if (read_bytes <= 0) {
>> +                       dev_err(&client->dev,
>> +                               "Failed to parse HEX file\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               /* Detect the end of file */
>> +               total_read_bytes += read_bytes;
>> +               if (total_read_bytes == fw->size) {
>
>
> What if it is larger due to a bad firmware file ?
>
>> +                       /*
>> +                        * For end of download, the length field will be
>> set
>> +                        * to 0
>> +                        */
>
>
> And if the file is not semantically correct and doesn't have a length field
> of 0 at the end ?
>
>
>> +                       memset(&fw_pkt, 0, sizeof(fw_pkt));
>> +                       ret = ziirave_firm_write_pkt(wdd, &fw_pkt);
>> +                       if (ret) {
>> +                               dev_err(&client->dev,
>> +                                       "Failed to send EMPTY packet:
>> %d\n",
>> +                                       ret);
>> +                               return ret;
>> +                       }
>> +
>> +                       /* This sleep seems to be required */
>> +                       msleep(20);
>> +
>> +                       /* Start firmware verification */
>> +                       ret = ziirave_firm_verify(wdd, fw);
>> +                       if (ret) {
>> +                               dev_err(&client->dev,
>> +                                       "Failed to verify firmware: %d\n",
>> ret);
>> +                               return ret;
>> +                       }
>> +
>> +                       /* End download operation */
>> +                       ret = ziirave_firm_write_byte(wdd,
>> +
>> ZIIRAVE_CMD_DOWNLOAD_END,
>> +                                                     1, false);
>> +                       if (ret)
>> +                               return ret;
>> +                       break;
>> +               }
>> +
>> +               if (addr_has_changed)
>> +                       continue;
>> +
>> +               /* Calculate words till page break */
>> +               words_till_page_break = (64 - (fw_pkt.addr & 0x3f));
>> +               if ((fw_pkt.length >> 1) > words_till_page_break) {
>> +                       /*
>> +                        * Data in passes page boundary, so we need to
>> split in
>> +                        * two blocks of data. Create a packet with the
>> first
>> +                        * block of data.
>> +                        */
>> +                       memset(&fw_pkt_new, 0, sizeof(fw_pkt_new));
>> +
>> +                       fw_pkt_new.length = words_till_page_break << 1;
>> +                       fw_pkt_new.addr = fw_pkt.addr;
>> +                       memcpy(fw_pkt_new.data, fw_pkt.data,
>> fw_pkt_new.length);
>> +
>> +                       ret = ziirave_firm_write_pkt(wdd, &fw_pkt_new);
>> +                       if (ret)
>> +                               return ret;
>> +
>> +                       /* Create a packet with the second block of data
>> */
>> +                       memset(&fw_pkt_new, 0, sizeof(fw_pkt_new));
>> +
>> +                       /* Remaining bytes */
>> +                       fw_pkt_new.length =
>> +                               fw_pkt.length - (words_till_page_break <<
>> 1);
>> +                       fw_pkt_new.addr = fw_pkt.addr +
>> words_till_page_break;
>> +                       memcpy(fw_pkt_new.data,
>> +                              fw_pkt.data + (words_till_page_break << 1),
>> +                              fw_pkt_new.length);
>> +
>> +                       ret = ziirave_firm_write_pkt(wdd, &fw_pkt_new);
>> +                       if (ret)
>> +                               return ret;
>> +               } else {
>> +                       ret = ziirave_firm_write_pkt(wdd, &fw_pkt);
>> +                       if (ret)
>> +                               return ret;
>> +               }
>> +       }
>> +
>> +       /* Reset the processor */
>> +       ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_RESET_PROCESSOR, 1,
>> +                                     false);
>> +       if (ret)
>> +               return ret;
>> +
>> +       msleep(500);
>> +
>> +       return 0;
>> +}
>> +
>>   static const struct watchdog_info ziirave_wdt_info = {
>>         .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
>> WDIOF_KEEPALIVEPING,
>>         .identity = "Zodiac RAVE Watchdog",
>> @@ -201,10 +575,57 @@ static ssize_t ziirave_wdt_sysfs_show_reason(struct
>> device *dev,
>>   static DEVICE_ATTR(reset_reason, S_IRUGO, ziirave_wdt_sysfs_show_reason,
>>                    NULL);
>>
>> +static ssize_t ziirave_wdt_sysfs_store_firm(struct device *dev,
>> +                                           struct device_attribute *attr,
>> +                                           const char *buf, size_t count)
>> +{
>> +       struct i2c_client *client = to_i2c_client(dev->parent);
>> +       struct ziirave_wdt_data *w_priv = i2c_get_clientdata(client);
>> +       const struct firmware *fw;
>> +       int err;
>> +
>
>
> What happens if this file is opened twice and written in parallel ?
> What if any
> other attribute is accessed while the firmware download is ongoing ?
>

Good point. So you think I need to protect all with mutexes or there
is another way? From a quick look on other drivers i didn't see this
kind of protection (something to look at in the future)

>
>> +       err = request_firmware(&fw, ZIIRAVE_FW_NAME, dev);
>> +       if (err) {
>> +               dev_err(&client->dev, "Unable to open firmware %s: %d\n",
>> +                       ZIIRAVE_FW_NAME, err);
>> +               return err;
>> +       }
>> +
>> +       err = ziirave_firm_upload(&w_priv->wdd, fw);
>> +       if (err) {
>> +               dev_err(&client->dev, "The firmware update failed: %d\n",
>> err);
>> +               goto release_firmware;
>> +       }
>> +
>> +       /* Update firmware version */
>> +       err = ziirave_wdt_revision(client, &w_priv->firmware_rev,
>> +                                  ZIIRAVE_WDT_FIRM_VER_MAJOR);
>> +       if (err) {
>> +               dev_err(&client->dev, "Failed to read firmware version:
>> %d\n",
>> +                       err);
>> +               goto release_firmware;
>> +       }
>> +
>> +       dev_info(&client->dev, "Firmware updated to version
>> 02.%02u.%02u\n",
>> +                w_priv->firmware_rev.major, w_priv->firmware_rev.minor);
>> +
>> +       /* Restore the watchdog timeout */
>> +       err = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout);
>> +       if (err)
>> +               dev_err(&client->dev, "Failed to set timeout: %d\n", err);
>> +
>> +release_firmware:
>> +       release_firmware(fw);
>> +       return err ? err : count;
>> +}
>> +
>> +static DEVICE_ATTR(update_fw, S_IWUSR, NULL,
>> ziirave_wdt_sysfs_store_firm);
>> +
>>   static struct attribute *ziirave_wdt_attrs[] = {
>>         &dev_attr_firmware_version.attr,
>>         &dev_attr_bootloader_version.attr,
>>         &dev_attr_reset_reason.attr,
>> +       &dev_attr_update_fw.attr,
>
>
> We have firmware_version, so update_firmware seems appropriate.
>
>>         NULL
>>   };
>>   ATTRIBUTE_GROUPS(ziirave_wdt);
>>
>

Regards,
Enric