Date:	Thu, 28 Jul 2016 17:27:13 +0200
From:	Enric Balletbo Serra <eballetbo@...il.com>
To:	Martyn Welch <martyn.welch@...labora.co.uk>
Cc:	Enric Balletbo i Serra <enric.balletbo@...labora.com>,
	linux-watchdog@...r.kernel.org,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Wim Van Sebroeck <wim@...ana.be>,
	Guenter Roeck <linux@...ck-us.net>,
	Martyn Welch <martyn.welch@...labora.com>
Subject: Re: [PATCH v3] watchdog: ziirave_wdt: Add support to upload the firmware.

Hi Martyn,


2016-07-28 16:36 GMT+02:00 Martyn Welch <martyn.welch@...labora.co.uk>:
>
>
> On 28/07/16 11:37, 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@...labora.com>
>> ---
>>
>> Changes since v2: (requested by Guenter)
>>   - Remove ihex parsing, convert the file into binary before passing it to
>> the
>>     kernel (ihex2fw <src.HEX> <dst.fw>).
>>   - New binary firmware parsed using the ihex.h helpers.
>>   - Protect firmware upgrade with a mutex.
>>   - Remove a noisy message.
>>   - Remove some unneded memset.
>>   - Remove some unnecessary ()
>>
>> Changes since v1: (requested by Martyn)
>>   - Remove two defines that are not used.
>>   - Fix typo in documentation count -> length
>>
>>   drivers/watchdog/ziirave_wdt.c | 363
>> +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 363 insertions(+)
>>
>> diff --git a/drivers/watchdog/ziirave_wdt.c
>> b/drivers/watchdog/ziirave_wdt.c
>> index fa1efef..81c8545 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/delay.h>
>>   #include <linux/i2c.h>
>> +#include <linux/ihex.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_wdt.fw"
>> +
>>   static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL,
>> NULL,
>>                                   "host request", NULL, "illegal
>> configuration",
>>                                   "illegal instruction", "illegal trap",
>> @@ -50,12 +55,32 @@ 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;
>>   };
>>
>>   struct ziirave_wdt_data {
>> +       struct mutex sysfs_mutex;
>
>
> I don't think this is the best name for the mutex given that it's only used
> with firmware upload. "firmware_mutex"?ffir
>

Oh, this reminds me that I missed something while rebasing. The idea
was block the sysfs attributes access while a firmware upgrade is in
progress, this is why I named sysfs_mutex. My bad, I'll fix this in
next revision.

>
>>         struct watchdog_device wdd;
>>         struct ziirave_wdt_rev bootloader_rev;
>>         struct ziirave_wdt_rev firmware_rev;
>> @@ -146,6 +171,288 @@ 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;
>> +}
>> +
>> +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];
>> +
>> +       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);
>> +
>> +       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);
>> +}
>> +
>> +/*
>> + * ziirave_firm_write_pkt() - Build and write a firmware packet
>> + *
>> + * 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.
>> + */
>> +static int ziirave_firm_write_pkt(struct watchdog_device *wdd,
>> +                                 const struct ihex_binrec *rec)
>> +{
>> +       struct i2c_client *client = to_i2c_client(wdd->parent);
>> +       u8 i, checksum = 0, packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE];
>> +       int ret;
>> +       u16 addr;
>> +
>> +       memset(packet, 0, ARRAY_SIZE(packet));
>> +
>> +       /* Packet length */
>> +       packet[0] = (u8)be16_to_cpu(rec->len);
>> +       /* Packet address */
>> +       addr = (be32_to_cpu(rec->addr) & 0xffff) >> 1;
>> +       packet[1] = (u8)(addr & 0xff);
>> +       packet[2] = (u8)((addr & 0xff00) >> 8);
>> +
>> +       /* Packet data */
>> +       if (be16_to_cpu(rec->len) > ZIIRAVE_FIRM_PKT_DATA_SIZE)
>> +               return -EMSGSIZE;
>> +       memcpy(packet + 3, rec->data, be16_to_cpu(rec->len));
>> +
>> +       /* Packet checksum */
>> +       for (i = 0; i < ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1; i++)
>> +               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",
>> +                     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);
>> +       const struct ihex_binrec *rec;
>> +       int i, ret;
>> +       u8 data[ZIIRAVE_FIRM_PKT_DATA_SIZE];
>> +       u16 addr;
>> +
>> +       for (rec = (void *)fw->data; rec; rec = ihex_next_binrec(rec)) {
>> +               /* Zero length marks end of records */
>> +               if (!be16_to_cpu(rec->len))
>> +                       break;
>> +
>> +               addr = (be32_to_cpu(rec->addr) & 0xffff) >> 1;
>> +               if (addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
>> +                   addr > ZIIRAVE_FIRM_FLASH_MEMORY_END)
>> +                       continue;
>> +
>> +               ret = ziirave_firm_set_read_addr(wdd, 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, rec->data, be16_to_cpu(rec->len))) {
>> +                       dev_err(&client->dev,
>> +                               "Firmware mismatch at address 0x%04x\n",
>> 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);
>> +       int ret, words_till_page_break;
>> +       const struct ihex_binrec *rec;
>> +       struct ihex_binrec *rec_new;
>> +
>> +       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);
>> +
>> +       for (rec = (void *)fw->data; rec; rec = ihex_next_binrec(rec)) {
>> +               /* Zero length marks end of records */
>> +               if (!be16_to_cpu(rec->len))
>> +                       break;
>> +
>> +               /* Check max data size */
>> +               if (be16_to_cpu(rec->len) > ZIIRAVE_FIRM_PKT_DATA_SIZE) {
>> +                       dev_err(&client->dev, "Firmware packet too long
>> (%d)\n",
>> +                               be16_to_cpu(rec->len));
>> +                       return -EMSGSIZE;
>> +               }
>> +
>> +               /* Calculate words till page break */
>> +               words_till_page_break = (64 - ((be32_to_cpu(rec->addr) >>
>> 1) &
>> +                                        0x3f));
>> +               if ((be16_to_cpu(rec->len) >> 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.
>> +                        */
>> +                       rec_new = kzalloc(sizeof(struct ihex_binrec) +
>> +                                         (words_till_page_break << 1),
>> +                                         GFP_KERNEL);
>> +                       if (!rec_new)
>> +                               return -ENOMEM;
>> +
>> +                       rec_new->len = cpu_to_be16(words_till_page_break
>> << 1);
>> +                       rec_new->addr = rec->addr;
>> +                       memcpy(rec_new->data, rec->data,
>> +                              be16_to_cpu(rec_new->len));
>> +
>> +                       ret = ziirave_firm_write_pkt(wdd, rec_new);
>> +                       kfree(rec_new);
>> +                       if (ret)
>> +                               return ret;
>> +
>> +                       /* Create a packet with the second block of data
>> */
>> +                       rec_new = kzalloc(sizeof(struct ihex_binrec) +
>> +                                         be16_to_cpu(rec->len) -
>> +                                         (words_till_page_break << 1),
>> +                                         GFP_KERNEL);
>> +                       if (!rec_new)
>> +                               return -ENOMEM;
>> +
>> +                       /* Remaining bytes */
>> +                       rec_new->len = rec->len -
>> +                                      cpu_to_be16(words_till_page_break
>> << 1);
>> +
>> +                       rec_new->addr = cpu_to_be32(be32_to_cpu(rec->addr)
>> +
>> +                                       (words_till_page_break << 1));
>> +
>> +                       memcpy(rec_new->data,
>> +                              rec->data + (words_till_page_break << 1),
>> +                              be16_to_cpu(rec_new->len));
>> +
>> +                       ret = ziirave_firm_write_pkt(wdd, rec_new);
>> +                       kfree(rec_new);
>> +                       if (ret)
>> +                               return ret;
>> +               } else {
>> +                       ret = ziirave_firm_write_pkt(wdd, rec);
>> +                       if (ret)
>> +                               return ret;
>> +               }
>> +       }
>> +
>> +       /* For end of download, the length field will be set to 0 */
>> +       rec_new = kzalloc(sizeof(struct ihex_binrec) + 1, GFP_KERNEL);
>> +       if (!rec_new)
>> +               return -ENOMEM;
>> +
>> +       ret = ziirave_firm_write_pkt(wdd, rec_new);
>> +       kfree(rec_new);
>> +       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;
>> +
>> +       /* 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 +508,64 @@ 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)
>> +{
>
>
>
> Am I right in thinking firmware upgrade will get triggered whatever is
> written to it? Wouldn't it be safer to only start the upgrade if a specific
> string is written to the sysfs entry? (Performing the upgrade is basically
> going to disable the watchdog is it not?)
>

Yes you're right. And yes, trigger a firmware upgrade whatever is
written to a file doesn't look very safe. Looking at other drivers
that has this upgrade mechanism seems they also trigger the upgrade
just writting something to the file. For example see
drivers/input/touchscreen/atmel_mxt_ts.c. So doesn't seem to be
something standard in the kernel to handle this.

Maybe just check for a string key will make the firmware upgrade more safer.

Before implementing it I'd like to hear other opinions from Guenter
and other people. Thoughts?

Enric
>
>> +       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;
>> +
>> +       err = mutex_lock_interruptible(&w_priv->sysfs_mutex);
>> +       if (err)
>> +               return err;
>> +
>> +       err = request_ihex_firmware(&fw, ZIIRAVE_FW_NAME, dev);
>> +       if (err) {
>> +               dev_err(&client->dev, "Failed to request ihex
>> firmware\n");
>> +               goto unlock_sysfs;
>> +       }
>> +
>> +       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);
>> +unlock_sysfs:
>> +       mutex_unlock(&w_priv->sysfs_mutex);
>> +
>> +       return err ? err : count;
>> +}
>> +
>> +static DEVICE_ATTR(update_firmware, 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_firmware.attr,
>>         NULL
>>   };
>>   ATTRIBUTE_GROUPS(ziirave_wdt);
>> @@ -252,6 +613,8 @@ static int ziirave_wdt_probe(struct i2c_client
>> *client,
>>         if (!w_priv)
>>                 return -ENOMEM;
>>
>> +       mutex_init(&w_priv->sysfs_mutex);
>> +
>>         w_priv->wdd.info = &ziirave_wdt_info;
>>         w_priv->wdd.ops = &ziirave_wdt_ops;
>>         w_priv->wdd.min_timeout = ZIIRAVE_TIMEOUT_MIN;
>>
>

Powered by blists - more mailing lists