On Mi, 2019-02-06 at 13:39 -0800, Ajay Gupta wrote: > From: Ajay Gupta <[email protected]>
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 <[email protected]> [..] > +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; > +}
