| 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: Tue, 6 Jun 2017 14:04:00 +0200
From: Maxime Ripard <maxime.ripard@...e-electrons.com>
To: Mylene Josserand <mylene.josserand@...e-electrons.com>
Cc: dmitry.torokhov@...il.com, fery@...ress.com, robh+dt@...nel.org,
mark.rutland@....com, linux-kernel@...r.kernel.org,
linux-input@...r.kernel.org, devicetree@...r.kernel.org,
thomas.petazzoni@...e-electrons.com
Subject: Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5
touchscreen
On Tue, Jun 06, 2017 at 10:03:48AM +0200, Mylene Josserand wrote:
> > > +static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code)
> > > +{
> > > + u16 size, crc;
> > > + u8 status, offset;
> > > + int command_code;
> > > +
> > > + size = get_unaligned_le16(&ts->response_buf[0]);
> > > +
> > > + if (!size)
> > > + return 0;
> > > +
> > > + offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET];
> > > +
> > > + if (offset == HID_BL_RESPONSE_REPORT_ID) {
> > > + if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) {
> > > + dev_err(ts->dev, "%s: HID output response, wrong SOP\n",
> > > + __func__);
> > > + return -EPROTO;
> > > + }
> > > +
> > > + if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) {
> > > + dev_err(ts->dev, "%s: HID output response, wrong EOP\n",
> > > + __func__);
> > > + return -EPROTO;
> > > + }
> > > +
> > > + crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7);
> > > + if (ts->response_buf[size - 3] != LOW_BYTE(crc)
> > > + || ts->response_buf[size - 2] != HI_BYTE(crc)) {
> > > + dev_err(ts->dev, "%s: HID output response, wrong CRC 0x%X\n",
> > > + __func__, crc);
> > > + return -EPROTO;
> > > + }
> > > +
> > > + status = ts->response_buf[5];
> > > + if (status) {
> > > + dev_err(ts->dev, "%s: HID output response, ERROR:%d\n",
> > > + __func__, status);
> > > + return -EPROTO;
> > > + }
> > > + }
> > > +
> > > + if (offset == HID_APP_RESPONSE_REPORT_ID) {
> > > + command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET]
> > > + & HID_OUTPUT_RESPONSE_CMD_MASK;
> > > + if (command_code != code) {
> > > + dev_err(ts->dev,
> > > + "%s: HID output response, wrong command_code:%X\n",
> > > + __func__, command_code);
> > > + return -EPROTO;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts)
> > > +{
> > > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > > + int i;
> > > + int num_btns = 0;
> > > + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET]
> > > + & HID_SYSINFO_BTN_MASK;
> > > +
> > > + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) {
> > > + if (btns & (1 << i))
> > > + num_btns++;
> > > + }
> > > + si->num_btns = num_btns;
> > > +}
> > > +
> > > +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts)
> > > +{
> > > + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data;
> > > + struct cyttsp5_sensing_conf_data_dev *scd_dev =
> > > + (struct cyttsp5_sensing_conf_data_dev *)
> > > + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET];
> > > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > > +
> > > + cyttsp5_si_get_btn_data(ts);
> > > +
> > > + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle;
> > > + scd->res_x = get_unaligned_le16(&scd_dev->res_x);
> > > + scd->res_y = get_unaligned_le16(&scd_dev->res_y);
> > > + scd->max_z = get_unaligned_le16(&scd_dev->max_z);
> > > + scd->len_x = get_unaligned_le16(&scd_dev->len_x);
> > > + scd->len_y = get_unaligned_le16(&scd_dev->len_y);
> > > +
> > > + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE,
> > > + GFP_KERNEL);
> > > + if (!si->xy_data)
> > > + return -ENOMEM;
> > > +
> > > + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE, GFP_KERNEL);
> > > + if (!si->xy_mode) {
> > > + devm_kfree(ts->dev, si->xy_data);
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts)
> > > +{
> > > + int rc, t;
> > > + u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE];
> > > +
> > > + mutex_lock(&ts->system_lock);
> > > + ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1;
> > > + mutex_unlock(&ts->system_lock);
> > > +
> > > + /* HI bytes of Output register address */
> > > + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
> > > + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
> > > + cmd[2] = HID_APP_OUTPUT_REPORT_ID;
> > > + cmd[3] = 0x0; /* Reserved */
> > > + cmd[4] = HID_OUTPUT_GET_SYSINFO;
> > > +
> > > + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
> > > + HID_OUTPUT_GET_SYSINFO_SIZE);
> > > + if (rc) {
> > > + dev_err(ts->dev, "%s: Failed to write command %d",
> > > + __func__, rc);
> > > + goto error;
> > > + }
> > > +
> > > + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0),
> > > + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT));
> > > + if (IS_TMO(t)) {
> > > + dev_err(ts->dev, "%s: HID output cmd execution timed out\n",
> > > + __func__);
> > > + rc = -ETIME;
> > > + goto error;
> > > + }
> > > +
> > > + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO);
> > > + goto exit;
> > > +
> > > +error:
> > > + mutex_lock(&ts->system_lock);
> > > + ts->hid_cmd_state = 0;
> > > + mutex_unlock(&ts->system_lock);
> > > + return rc;
> > > +exit:
> > > + rc = cyttsp5_get_sysinfo_regs(ts);
> > > +
> > > + return rc;
> > > +}
> > > +
> > > +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
> > > +{
> > > + int rc, t;
> > > + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP];
> > > + u16 crc;
> > > +
> > > + mutex_lock(&ts->system_lock);
> > > + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1;
> > > + mutex_unlock(&ts->system_lock);
> > > +
> > > + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> > > + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> > > + cmd[2] = HID_BL_OUTPUT_REPORT_ID;
> > > + cmd[3] = 0x0; /* Reserved */
> > > + cmd[4] = HID_OUTPUT_BL_SOP;
> > > + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP;
> > > + cmd[6] = 0x0; /* Low bytes of data */
> > > + cmd[7] = 0x0; /* Hi bytes of data */
> > > + crc = cyttsp5_compute_crc(&cmd[4], 4);
> > > + cmd[8] = LOW_BYTE(crc);
> > > + cmd[9] = HI_BYTE(crc);
> > > + cmd[10] = HID_OUTPUT_BL_EOP;
> > > +
> > > + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
> > > + HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> >
> > It seems like you'll send HID_OUTPUT_BL_LAUNCH_APP_SIZE twice here,
> > once as setup in cyttsp5_write, and once in the buffer you want to
> > send. Is that on purpose?
>
> I am not sure to see the second time it is sent. What do you mean by "as
> setup in cyttsp5_write"? Anyway, the size is needed in the buffer of the
> frame.
Nevermind, I confused the HID_OUTPUT_BL_LAUNCH_APP_SIZE with the
register, my bad.
> > > + /* Call platform init function */
> > > + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > + if (IS_ERR(ts->reset_gpio)) {
> > > + rc = PTR_ERR(ts->reset_gpio);
> > > + dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> > > + return rc;
> > > + }
> > > +
> > > + /* Need a delay to have device up */
> > > + msleep(20);
> >
> > In the case where the device has already been out of reset, this means
> > that this alone is not sufficient to reset it and bring it out of
> > reset, which also means that you do not have any guarantee on the
> > current state of the device. I'm not sure how it would impact the
> > proper operations though.
>
> Okay. A reset frame can be sent to perform a "software
> reset". Should I add it on startup to be in a "known behavior"?
I guess you'd be safer just getting the GPIO with the low level by
default, and then changing to high. That way you know that you went
through a reset state, before bringing up the device.
> > > + ts->input = input_allocate_device();
> > > + if (!ts->input) {
> > > + dev_err(dev, "%s: Error, failed to allocate input device\n",
> > > + __func__);
> > > + rc = -ENODEV;
> > > + goto error_startup;
> > > + }
> >
> > In error_startup, you never uninitialize the device. Given your
> > comment in cyttsp5_startup, this means that you might end up in a
> > situation where your driver probes again with the device initialized
> > and no longer in a bootloader mode, which is likely to cause some
> > error.
> >
> > Putting the call to cyttsp5_startup later will make you less likely to
> > fail (especially because of the DT parsing), and putting the device
> > back into reset will probably address the issue once and for all.
>
> Hm, I am not sure to understand what you want to say, here.
> Could you explain me more what you have in mind?
I meant to say that there was still an issue with the reset here, and
moving the DT parsing code further would ease the reset operation.
> Notice that the DT parsing uses a sysinfo's variable (si->num_btns)
> which is retrieved in the startup function (thanks to
> get_sysinfo). So, currently, it is not possible to move the startup
> function after the DT parsing.
Can't that be made the other way around? You parse the number of
buttons in the DT, and check the consistency with the hardware?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)
Powered by blists - more mailing lists