* [PATCH v2] watchdog: ziirave_wdt: Add support to upload the firmware.
@ 2016-07-20 10:31 Enric Balletbo i Serra
2016-07-21 3:42 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Enric Balletbo i Serra @ 2016-07-20 10:31 UTC (permalink / raw)
To: linux-watchdog, linux-kernel
Cc: Wim Van Sebroeck, Guenter Roeck, Martyn Welch
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@collabora.com>
---
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;
+
+ /* locate end of line */
+ for (src = fw_data; *src != '\n'; src += 2) {
+ /*
+ * 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;
+ break;
+ default:
+ fw_pkt->data[(count - 4)] = dst;
+ break;
+ }
+ 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));
+ 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);
+ }
+
+ 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));
+
+ 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);
+
+ /* Compute 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",
+ 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);
+
+ /* Detect the end of file */
+ total_read_bytes += read_bytes;
+ if (total_read_bytes == fw->size)
+ break;
+
+ if (addr_has_changed)
+ continue;
+
+ if (fw_pkt.addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
+ fw_pkt.addr > ZIIRAVE_FIRM_FLASH_MEMORY_END)
+ continue;
+
+ 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) {
+ /*
+ * For end of download, the length field will be set
+ * to 0
+ */
+ 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;
+
+ 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,
NULL
};
ATTRIBUTE_GROUPS(ziirave_wdt);
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] watchdog: ziirave_wdt: Add support to upload the firmware.
2016-07-20 10:31 [PATCH v2] watchdog: ziirave_wdt: Add support to upload the firmware Enric Balletbo i Serra
@ 2016-07-21 3:42 ` Guenter Roeck
2016-07-21 8:59 ` Enric Balletbo Serra
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2016-07-21 3:42 UTC (permalink / raw)
To: Enric Balletbo i Serra, linux-watchdog, linux-kernel
Cc: Wim Van Sebroeck, Martyn Welch
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@collabora.com>
> ---
>
> 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.
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.
> + }
> +
> + 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 ?
> + 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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] watchdog: ziirave_wdt: Add support to upload the firmware.
2016-07-21 3:42 ` Guenter Roeck
@ 2016-07-21 8:59 ` Enric Balletbo Serra
2016-07-21 9:09 ` Enric Balletbo Serra
2016-07-21 13:24 ` Guenter Roeck
0 siblings, 2 replies; 6+ messages in thread
From: Enric Balletbo Serra @ 2016-07-21 8:59 UTC (permalink / raw)
To: Guenter Roeck
Cc: Enric Balletbo i Serra, linux-watchdog, linux-kernel,
Wim Van Sebroeck, Martyn Welch
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@roeck-us.net>:
> 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@collabora.com>
>> ---
>>
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] watchdog: ziirave_wdt: Add support to upload the firmware.
2016-07-21 8:59 ` Enric Balletbo Serra
@ 2016-07-21 9:09 ` Enric Balletbo Serra
2016-07-21 13:24 ` Guenter Roeck
1 sibling, 0 replies; 6+ messages in thread
From: Enric Balletbo Serra @ 2016-07-21 9:09 UTC (permalink / raw)
To: Guenter Roeck
Cc: Enric Balletbo i Serra, linux-watchdog, linux-kernel,
Wim Van Sebroeck, Martyn Welch
Correction
2016-07-21 10:59 GMT+02:00 Enric Balletbo Serra <eballetbo@gmail.com>:
> 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@roeck-us.net>:
>> 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@collabora.com>
>>> ---
>>>
>>> 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)
>
Just found other drivers using a mutex, so forget my last sentence.
>>
>>> + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] watchdog: ziirave_wdt: Add support to upload the firmware.
2016-07-21 8:59 ` Enric Balletbo Serra
2016-07-21 9:09 ` Enric Balletbo Serra
@ 2016-07-21 13:24 ` Guenter Roeck
2016-07-21 13:40 ` Enric Balletbo Serra
1 sibling, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2016-07-21 13:24 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Enric Balletbo i Serra, linux-watchdog, linux-kernel,
Wim Van Sebroeck, Martyn Welch
On 07/21/2016 01:59 AM, Enric Balletbo Serra wrote:
[ ... ]
>>>
>>> +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
Yes, you have to protect any possible damaging parallel access with mutexes.
> 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)
>
Really ? Can you point me to some examples ? This would be a bug.
You'll have to fix the to get your patch accepted. Pointing to other
examples with the same or a similar bug is not an argument.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] watchdog: ziirave_wdt: Add support to upload the firmware.
2016-07-21 13:24 ` Guenter Roeck
@ 2016-07-21 13:40 ` Enric Balletbo Serra
0 siblings, 0 replies; 6+ messages in thread
From: Enric Balletbo Serra @ 2016-07-21 13:40 UTC (permalink / raw)
To: Guenter Roeck
Cc: Enric Balletbo i Serra, linux-watchdog, linux-kernel,
Wim Van Sebroeck, Martyn Welch
2016-07-21 15:24 GMT+02:00 Guenter Roeck <linux@roeck-us.net>:
> On 07/21/2016 01:59 AM, Enric Balletbo Serra wrote:
>
> [ ... ]
>
>>>>
>>>> +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
>
>
> Yes, you have to protect any possible damaging parallel access with mutexes.
>
>> 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)
>>
>
> Really ? Can you point me to some examples ? This would be a bug.
>
> You'll have to fix the to get your patch accepted. Pointing to other
> examples with the same or a similar bug is not an argument.
>
Yeah I know, I didn't want to justify me, I didn't express well if
this was your impression, I just wanted to point that there are some
drivers in other subsystems that I think that have this problem, I
only did a quick look though (is some upstreaming work that I can add
to my TODO list)
Already fixed in next v3 :)
> Thanks,
> Guenter
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-21 13:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-20 10:31 [PATCH v2] watchdog: ziirave_wdt: Add support to upload the firmware Enric Balletbo i Serra
2016-07-21 3:42 ` Guenter Roeck
2016-07-21 8:59 ` Enric Balletbo Serra
2016-07-21 9:09 ` Enric Balletbo Serra
2016-07-21 13:24 ` Guenter Roeck
2016-07-21 13:40 ` Enric Balletbo Serra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.