On Mi, 2019-02-06 at 13:39 -0800, Ajay Gupta wrote:
> From: Ajay Gupta <ajayg@xxxxxxxxxx>
Hi,
>
> CCGx has two copies of the firmware in addition to the bootloader.
> If the device is running FW1, FW2 can be updated with the new version.
> Dual firmware mode allows the CCG device to stay in a PD contract and
> support USB PD and Type-C functionality while a firmware update is in
> progress.
>
> First we read the currently flashed firmware version of both
> primary and secondary firmware and then compare it with
> version of firmware file to determine if flashing is required.
I am wondering about the security implications of this.
You are persistently changing firmware of a device. It looks
to me like there ought to be a check for capable'(CFAP_SYS_RAWIO)
in the firmware update path.
> Command framework is added to support sending commands to CCGx
> controller. We wait for response after sending the command and then
> read the response from RAB_RESPONSE register.
>
> Below commands are supported,
> - ENTER_FLASHING
> - RESET
> - PDPORT_ENABLE
> - JUMP_TO_BOOT
> - FLASH_ROW_RW
> - VALIDATE_FW
>
> Command specific mutex lock is also added to sync between driver
> and user threads.
>
> PD port number information is added which is required while sending
> PD_PORT_ENABLE command
>
> Signed-off-by: Ajay Gupta <ajayg@xxxxxxxxxx>
[..]
> +struct ccg_cmd {
> + u16 reg;
> + u32 data;
> + int len;
> + int delay; /* ms delay for cmd timeout */
unsigned? A negative delay is an interesting concept.
[..]
> +static int ccg_cmd_reset(struct ucsi_ccg *uc, bool extra_delay)
> +{
> + struct ccg_cmd cmd;
> + u8 *p;
> + int ret;
> +
> + p = (u8 *)&cmd.data;
> + cmd.reg = CCGX_RAB_RESET_REQ;
> + p[0] = RESET_SIG;
> + p[1] = CMD_RESET_DEV;
> + cmd.len = 2;
> + cmd.delay = 2000 + (extra_delay ? 3000 : 0);
This is a maximum, right?
If a reset fails, this is very bad. So why not always allow
the maximum timeout?
> + mutex_lock(&uc->lock);
> +
> + set_bit(RESET_PENDING, &uc->flags);
> +
> + ret = ccg_send_command(uc, &cmd);
> + if (ret != RESET_COMPLETE)
> + goto err_clear_flag;
> +
> + ret = 0;
> +
> +err_clear_flag:
> + clear_bit(RESET_PENDING, &uc->flags);
> +
> + mutex_unlock(&uc->lock);
> +
> + return ret;
> +}
[..]
> +static int
> +ccg_cmd_write_flash_row(struct ucsi_ccg *uc, u16 row,
> + const void *data, u8 fcmd)
> +{
> + struct i2c_client *client = uc->client;
> + struct ccg_cmd cmd;
> + u8 buf[CCG4_ROW_SIZE + 2];
> + u8 *p;
> + int ret;
> +
> + /* Copy the data into the flash read/write memory. */
> + buf[0] = REG_FLASH_RW_MEM & 0xFF;
> + buf[1] = REG_FLASH_RW_MEM >> 8;
We have macros for converting endianness. Please use them. That is
a kind of generic issue with this patch.
> +
> + memcpy(buf + 2, data, CCG4_ROW_SIZE);
> +
> + mutex_lock(&uc->lock);
> +
> + ret = i2c_master_send(client, buf, CCG4_ROW_SIZE + 2);
> + if (ret != CCG4_ROW_SIZE + 2) {
> + dev_err(uc->dev, "REG_FLASH_RW_MEM write fail %d\n", ret);
> + return ret < 0 ? ret : -EIO;
> + }
> +
> + /* Use the FLASH_ROW_READ_WRITE register to trigger */
> + /* writing of data to the desired flash row */
> + p = (u8 *)&cmd.data;
> + cmd.reg = CCGX_RAB_FLASH_ROW_RW;
> + p[0] = FLASH_SIG;
> + p[1] = fcmd;
> + p[2] = row & 0xFF;
> + p[3] = row >> 8;
Again. The macros.
> + cmd.len = 4;
> + cmd.delay = 50;
> + if (fcmd == FLASH_FWCT_SIG_WR_CMD)
> + cmd.delay += 400;
> + if (row == 510)
> + cmd.delay += 220;
> + ret = ccg_send_command(uc, &cmd);
> +
> + mutex_unlock(&uc->lock);
> +
> + if (ret != CMD_SUCCESS) {
> + dev_err(uc->dev, "write flash row failed ret=%d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
