| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 13 Oct 2011 20:03:20 +0200
From: "Henrik Rydberg" <rydberg@...omail.se>
To: Javier Martinez Canillas <martinez.javier@...il.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Mohan Pallaka <mpallaka@...eaurora.org>,
Kevin McNeely <kev@...ress.com>, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/3] Input: cyttsp - Cypress TTSP capacitive
multi-touch screen support
Hi Javier,
some more comments on the code. Presumable Dmitry will have additional
ones later on. Thank you for your patience.
> +/* Slots management */
> +#define CY_MAX_FINGER 4
> +#define CY_MAX_ID 15
I realize the firmware reports ids in the range [1..14], but it is not
visible from the code. Allowing all 16 values makes correctness
obvious.
> +static int cyttsp_load_bl_regs(struct cyttsp *ts)
> +{
> + int retval;
> +
> + memset(&(ts->bl_data), 0, sizeof(struct cyttsp_bootloader_data));
> +
> + ts->bl_data.bl_status = 0x10;
> +
> + retval = ttsp_read_block_data(ts, CY_REG_BASE,
> + sizeof(ts->bl_data), &(ts->bl_data));
> +
> + return retval;
> +}
Looks like you missed the removal of retval here.
> +
> +static int cyttsp_bl_app_valid(struct cyttsp *ts)
> +{
> + int retval;
> +
> + retval = cyttsp_load_bl_regs(ts);
> +
> + if (retval < 0) {
> + retval = -ENODEV;
> + goto done;
> + }
> +
> + if (GET_BOOTLOADERMODE(ts->bl_data.bl_status)) {
> + if (IS_VALID_APP(ts->bl_data.bl_status)) {
> + dev_dbg(ts->dev, "%s: App found; normal boot\n",
> + __func__);
> + return 0;
> + } else {
> + dev_dbg(ts->dev, "%s: NO APP; load firmware!!\n",
> + __func__);
> + return -ENODEV;
> + }
> + }
> +
> + if (GET_HSTMODE(ts->bl_data.bl_file) == CY_OPERATE_MODE) {
> + if (!(IS_OPERATIONAL_ERR(ts->bl_data.bl_status))) {
> + dev_dbg(ts->dev, "%s: Operational\n",
> + __func__);
> + return 1;
> + } else {
> + dev_dbg(ts->dev, "%s: Operational failure\n",
> + __func__);
> + return -ENODEV;
> + }
> + }
> +
> + dev_dbg(ts->dev, "%s: Non-Operational failure\n",
> + __func__);
> + retval = -ENODEV;
> +done:
> + return retval;
> +
> +}
Are the debug paths still necessary? There are a lot of them.
> +static int cyttsp_exit_bl_mode(struct cyttsp *ts)
> +{
> + int retval;
> + int tries;
> + u8 bl_cmd[sizeof(bl_command)];
> +
> + memcpy(bl_cmd, bl_command, sizeof(bl_command));
> + if (ts->platform_data->bl_keys)
> + memcpy(&bl_cmd[sizeof(bl_command) - CY_NUM_BL_KEYS],
> + ts->platform_data->bl_keys, sizeof(bl_command));
> +
> + dev_dbg(ts->dev,
> + "%s: bl_cmd= "
> + "%02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X\n",
> + __func__, bl_cmd[0], bl_cmd[1], bl_cmd[2],
> + bl_cmd[3], bl_cmd[4], bl_cmd[5], bl_cmd[6],
> + bl_cmd[7], bl_cmd[8], bl_cmd[9], bl_cmd[10]);
> +
> + retval = ttsp_write_block_data(ts, CY_REG_BASE,
> + sizeof(bl_cmd), (void *)bl_cmd);
> + if (retval < 0)
> + return retval;
> +
> + /* wait for TTSP Device to complete switch to Operational mode */
> + tries = 0;
> + do {
> + msleep(CY_DELAY_DFLT);
> + retval = cyttsp_load_bl_regs(ts);
> + } while (!((retval == 0) &&
> + !GET_BOOTLOADERMODE(ts->bl_data.bl_status)) &&
> + (tries++ < CY_DELAY_MAX));
> +
> + dev_dbg(ts->dev, "%s: check bl ready tries=%d ret=%d stat=%02X\n",
> + __func__, tries, retval, ts->bl_data.bl_status);
> +
> + if (retval < 0)
> + return retval;
> + else if (GET_BOOTLOADERMODE(ts->bl_data.bl_status))
> + return -ENODEV;
> + else
> + return 0;
> +}
Regardless of if it can ever happen, programatically the (retval > 0)
&& GET_BOOTLOADERMODE(ts->bl_data.bl_status) case is slipping through here.
> +static int cyttsp_set_operational_mode(struct cyttsp *ts)
> +{
> + struct cyttsp_xydata xy_data;
> + int retval;
> + int tries;
> + u8 cmd = CY_OPERATE_MODE;
> +
> + retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
> +
> + if (retval < 0)
> + return retval;
> +
> + /* wait for TTSP Device to complete switch to Operational mode */
> + tries = 0;
> + do {
> + retval = ttsp_read_block_data(ts, CY_REG_BASE,
> + sizeof(xy_data), &(xy_data));
> + } while (!((retval == 0) &&
> + (xy_data.act_dist == CY_ACT_DIST_DFLT)) &&
> + (tries++ < CY_DELAY_MAX));
> +
> + dev_dbg(ts->dev, "%s: check op ready tries=%d ret=%d dist=%02X\n",
> + __func__, tries, retval, xy_data.act_dist);
> +
> + return retval;
> +}
So (xy_data.act_dist == CY_ACT_DIST_DFL) here is not an error?
> +static int cyttsp_set_sysinfo_mode(struct cyttsp *ts)
> +{
> + int retval;
> + int tries;
> + u8 cmd = CY_SYSINFO_MODE;
> +
> + memset(&(ts->sysinfo_data), 0, sizeof(struct cyttsp_sysinfo_data));
> +
> + /* switch to sysinfo mode */
> + retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
> + if (retval < 0)
> + return retval;
> +
> + /* read sysinfo registers */
> + tries = 0;
> + do {
> + msleep(CY_DELAY_DFLT);
> + retval = ttsp_read_block_data(ts, CY_REG_BASE,
> + sizeof(ts->sysinfo_data), &(ts->sysinfo_data));
> + } while (!((retval == 0) &&
> + !((ts->sysinfo_data.tts_verh == 0) &&
> + (ts->sysinfo_data.tts_verl == 0))) &&
> + (tries++ < CY_DELAY_MAX));
Expands to (retval || !ts->sysinfo_data.tts_verh &&
!ts->sysinfo_data.tts_verl) && (tries++ < CY_DELAY_MAX), which seems a
bit simpler to me. Also, &ts->sysinfo_data could be a pointer here.
Coding verh+verl as a 16-bit value?
Are the timeouted (tries >= CY_DELAY_MAX) cases not errors here either?
<snip>
> +static int cyttsp_soft_reset(struct cyttsp *ts)
> +{
> + int retval;
> + u8 cmd = CY_SOFT_RESET_MODE;
> + long wait_jiffies = msecs_to_jiffies(CY_DELAY_DFLT * CY_DELAY_MAX);
> + /* wait for interrupt to set ready completion */
> + INIT_COMPLETION(ts->bl_ready);
> +
> + retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
> + if (retval < 0)
> + return retval;
> +
> + retval = wait_for_completion_timeout(&ts->bl_ready, wait_jiffies);
> +
> + if (retval > 0)
> + retval = 0;
Seems this one slipped.
> +
> + return retval;
> +}
> +
> +static int cyttsp_hndshk(struct cyttsp *ts, u8 hst_mode)
> +{
> + int retval;
> + u8 cmd;
> +
> + cmd = hst_mode & CY_HNDSHK_BIT ?
> + hst_mode ^ CY_HNDSHK_BIT :
> + hst_mode | CY_HNDSHK_BIT;
Whoa, please redo this one correctly.
> +
> + retval = ttsp_write_block_data(ts, CY_REG_BASE,
> + sizeof(cmd), (u8 *)&cmd);
> +
> + return retval;
> +}
> +int cyttsp_resume(void *handle)
> +{
> + struct cyttsp *ts = handle;
> + int retval = 0;
> + struct cyttsp_xydata xydata;
> +
> + if (ts) {
Neater if returning on (!ts) here instead.
> + if (ts->platform_data->use_sleep && (ts->power_state !=
> + CY_ACTIVE_STATE)) {
> + if (ts->platform_data->wakeup) {
> + retval = ts->platform_data->wakeup();
> + if (retval < 0)
> + dev_dbg(ts->dev, "%s: Error, wakeup failed!\n",
> + __func__);
> + } else {
> + dev_dbg(ts->dev, "%s: Error, wakeup not implemented "
> + "(check board file).\n", __func__);
> + retval = -ENOSYS;
> + }
> + if (!(retval < 0)) {
> + retval = ttsp_read_block_data(ts, CY_REG_BASE,
> + sizeof(xydata),
> + &xydata);
> + if (!(retval < 0) &&
> + !GET_HSTMODE(xydata.hst_mode))
> + ts->power_state = CY_ACTIVE_STATE;
> + }
> + }
> + dev_dbg(ts->dev, "%s: Wake Up %s\n", __func__,
> + (retval < 0) ? "FAIL" : "PASS");
> + }
> + return retval;
> +}
> +EXPORT_SYMBOL_GPL(cyttsp_resume);
Thanks.
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists