| 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
| ||
|
Message-ID: <5f24b14b-75b0-52d5-eb0a-da17f3c45742@gmail.com>
Date: Tue, 10 Dec 2019 04:34:33 +0300
From: Dmitry Osipenko <digetx@...il.com>
To: Michał Mirosław <mirq-linux@...e.qmqm.pl>,
linux-input@...r.kernel.org
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] input: elants: refactor elants_i2c_execute_command()
10.12.2019 03:19, Michał Mirosław пишет:
> Apply some DRY-ing to elants_i2c_execute_command() callers.
>
> Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> ---
> drivers/input/touchscreen/elants_i2c.c | 182 +++++++++++++------------
> 1 file changed, 93 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/input/touchscreen/elants_i2c.c b/drivers/input/touchscreen/elants_i2c.c
> index 2e6c9aa60496..27aca3971da5 100644
> --- a/drivers/input/touchscreen/elants_i2c.c
> +++ b/drivers/input/touchscreen/elants_i2c.c
> @@ -201,7 +201,8 @@ static int elants_i2c_read(struct i2c_client *client, void *data, size_t size)
>
> static int elants_i2c_execute_command(struct i2c_client *client,
> const u8 *cmd, size_t cmd_size,
> - u8 *resp, size_t resp_size)
> + u8 *resp, size_t resp_size,
> + int retries, const char *cmd_name)
> {
> struct i2c_msg msgs[2];
> int ret;
> @@ -217,30 +218,55 @@ static int elants_i2c_execute_command(struct i2c_client *client,
> break;
>
> default:
> - dev_err(&client->dev, "%s: invalid command %*ph\n",
> - __func__, (int)cmd_size, cmd);
> + dev_err(&client->dev, "(%s): invalid command: %*ph\n",
> + cmd_name, (int)cmd_size, cmd);
> return -EINVAL;
> }
>
> - msgs[0].addr = client->addr;
> - msgs[0].flags = client->flags & I2C_M_TEN;
> - msgs[0].len = cmd_size;
> - msgs[0].buf = (u8 *)cmd;
> + for (;;) {
> + msgs[0].addr = client->addr;
> + msgs[0].flags = client->flags & I2C_M_TEN;
> + msgs[0].len = cmd_size;
> + msgs[0].buf = (u8 *)cmd;
>
> - msgs[1].addr = client->addr;
> - msgs[1].flags = client->flags & I2C_M_TEN;
> - msgs[1].flags |= I2C_M_RD;
> - msgs[1].len = resp_size;
> - msgs[1].buf = resp;
> + msgs[1].addr = client->addr;
> + msgs[1].flags = client->flags & I2C_M_TEN;
> + msgs[1].flags |= I2C_M_RD;
> + msgs[1].len = resp_size;
> + msgs[1].buf = resp;
>
> - ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> - if (ret < 0)
> - return ret;
> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (ret < 0) {
> + if (--retries > 0) {
> + dev_dbg(&client->dev,
> + "(%s) I2C transfer failed: %d (retrying)\n",
> + cmd_name, ret);
> + continue;
> + }
>
> - if (ret != ARRAY_SIZE(msgs) || resp[FW_HDR_TYPE] != expected_response)
> - return -EIO;
> + dev_err(&client->dev,
> + "(%s) I2C transfer failed: %d\n",
> + cmd_name, ret);
> + return ret;
> + }
>
> - return 0;
> + if (ret != ARRAY_SIZE(msgs) ||
> + resp[FW_HDR_TYPE] != expected_response) {
> + if (--retries > 0) {
> + dev_dbg(&client->dev,
> + "(%s) unexpected response: %*ph (retrying)\n",
> + cmd_name, ret, resp);
> + continue;
> + }
> +
> + dev_err(&client->dev,
> + "(%s) unexpected response: %*ph\n",
> + cmd_name, ret, resp);
> + return -EIO;
> + }
> +
> + return --retries;
> + }
> }
>
> static int elants_i2c_calibrate(struct elants_data *ts)
> @@ -313,27 +339,20 @@ static u16 elants_i2c_parse_version(u8 *buf)
> static int elants_i2c_query_hw_version(struct elants_data *ts)
> {
> struct i2c_client *client = ts->client;
> - int error, retry_cnt;
> + int retry_cnt = MAX_RETRIES;
> const u8 cmd[] = { CMD_HEADER_READ, E_ELAN_INFO_FW_ID, 0x00, 0x01 };
> u8 resp[HEADER_SIZE];
>
> - for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
> - error = elants_i2c_execute_command(client, cmd, sizeof(cmd),
> - resp, sizeof(resp));
> - if (!error) {
> - ts->hw_version = elants_i2c_parse_version(resp);
> - if (ts->hw_version != 0xffff)
> - return 0;
> - }
> + while (retry_cnt) {
> + retry_cnt = elants_i2c_execute_command(client, cmd, sizeof(cmd),
> + resp, sizeof(resp),
> + retry_cnt, "read fw id");
> + if (retry_cnt < 0)
> + return retry_cnt;
>
> - dev_dbg(&client->dev, "read fw id error=%d, buf=%*phC\n",
> - error, (int)sizeof(resp), resp);
> - }
> -
> - if (error) {
> - dev_err(&client->dev,
> - "Failed to read fw id: %d\n", error);
> - return error;
> + ts->hw_version = elants_i2c_parse_version(resp);
> + if (ts->hw_version != 0xffff)
> + return 0;
> }
>
> dev_err(&client->dev, "Invalid fw id: %#04x\n", ts->hw_version);
> @@ -344,26 +363,28 @@ static int elants_i2c_query_hw_version(struct elants_data *ts)
> static int elants_i2c_query_fw_version(struct elants_data *ts)
> {
> struct i2c_client *client = ts->client;
> - int error, retry_cnt;
> + int retry_cnt = MAX_RETRIES;
> const u8 cmd[] = { CMD_HEADER_READ, E_ELAN_INFO_FW_VER, 0x00, 0x01 };
> u8 resp[HEADER_SIZE];
>
> - for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
> - error = elants_i2c_execute_command(client, cmd, sizeof(cmd),
> - resp, sizeof(resp));
> - if (!error) {
> - ts->fw_version = elants_i2c_parse_version(resp);
> - if (ts->fw_version != 0x0000 &&
> - ts->fw_version != 0xffff)
> - return 0;
> - }
> + while (retry_cnt) {
> + retry_cnt = elants_i2c_execute_command(client, cmd,
> + sizeof(cmd),
> + resp, sizeof(resp),
> + retry_cnt,
> + "read fw version");
> + if (retry_cnt < 0)
> + return retry_cnt;
>
> - dev_dbg(&client->dev, "read fw version error=%d, buf=%*phC\n",
> - error, (int)sizeof(resp), resp);
> + ts->fw_version = elants_i2c_parse_version(resp);
> + if (ts->fw_version != 0x0000 && ts->fw_version != 0xffff)
> + return 0;
> +
> + dev_dbg(&client->dev, "(read fw version) resp %*phC\n",
> + (int)sizeof(resp), resp);
> }
>
> - dev_err(&client->dev,
> - "Failed to read fw version or fw version is invalid\n");
> + dev_err(&client->dev, "Invalid fw ver: %#04x\n", ts->fw_version);
>
> return -EINVAL;
> }
> @@ -371,25 +392,20 @@ static int elants_i2c_query_fw_version(struct elants_data *ts)
> static int elants_i2c_query_test_version(struct elants_data *ts)
> {
> struct i2c_client *client = ts->client;
> - int error, retry_cnt;
> + int error;
> u16 version;
> const u8 cmd[] = { CMD_HEADER_READ, E_ELAN_INFO_TEST_VER, 0x00, 0x01 };
> u8 resp[HEADER_SIZE];
>
> - for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
> - error = elants_i2c_execute_command(client, cmd, sizeof(cmd),
> - resp, sizeof(resp));
> - if (!error) {
> - version = elants_i2c_parse_version(resp);
> - ts->test_version = version >> 8;
> - ts->solution_version = version & 0xff;
> + error = elants_i2c_execute_command(client, cmd, sizeof(cmd),
> + resp, sizeof(resp), MAX_RETRIES,
> + "read test version");
> + if (error >= 0) {
> + version = elants_i2c_parse_version(resp);
> + ts->test_version = version >> 8;
> + ts->solution_version = version & 0xff;
>
> - return 0;
> - }
> -
> - dev_dbg(&client->dev,
> - "read test version error rc=%d, buf=%*phC\n",
> - error, (int)sizeof(resp), resp);
> + return 0;
> }
>
> dev_err(&client->dev, "Failed to read test version\n");
> @@ -406,13 +422,10 @@ static int elants_i2c_query_bc_version(struct elants_data *ts)
> int error;
>
> error = elants_i2c_execute_command(client, cmd, sizeof(cmd),
> - resp, sizeof(resp));
> - if (error) {
> - dev_err(&client->dev,
> - "read BC version error=%d, buf=%*phC\n",
> - error, (int)sizeof(resp), resp);
> + resp, sizeof(resp), 1,
> + "read BC version");
> + if (error)
> return error;
> - }
>
> version = elants_i2c_parse_version(resp);
> ts->bc_version = version >> 8;
> @@ -444,12 +457,10 @@ static int elants_i2c_query_ts_info(struct elants_data *ts)
> error = elants_i2c_execute_command(client,
> get_resolution_cmd,
> sizeof(get_resolution_cmd),
> - resp, sizeof(resp));
> - if (error) {
> - dev_err(&client->dev, "get resolution command failed: %d\n",
> - error);
> + resp, sizeof(resp), 1,
> + "get resolution");
> + if (error)
> return error;
> - }
>
> rows = resp[2] + resp[6] + resp[10];
> cols = resp[3] + resp[7] + resp[11];
> @@ -457,36 +468,29 @@ static int elants_i2c_query_ts_info(struct elants_data *ts)
> /* Process mm_to_pixel information */
> error = elants_i2c_execute_command(client,
> get_osr_cmd, sizeof(get_osr_cmd),
> - resp, sizeof(resp));
> - if (error) {
> - dev_err(&client->dev, "get osr command failed: %d\n",
> - error);
> + resp, sizeof(resp), 1, "get osr");
> + if (error)
> return error;
> - }
>
> osr = resp[3];
>
> error = elants_i2c_execute_command(client,
> get_physical_scan_cmd,
> sizeof(get_physical_scan_cmd),
> - resp, sizeof(resp));
> - if (error) {
> - dev_err(&client->dev, "get physical scan command failed: %d\n",
> - error);
> + resp, sizeof(resp), 1,
> + "get physical scan");
> + if (error)
> return error;
> - }
>
> phy_x = get_unaligned_be16(&resp[2]);
>
> error = elants_i2c_execute_command(client,
> get_physical_drive_cmd,
> sizeof(get_physical_drive_cmd),
> - resp, sizeof(resp));
> - if (error) {
> - dev_err(&client->dev, "get physical drive command failed: %d\n",
> - error);
> + resp, sizeof(resp), 1,
> + "get physical drive");
> + if (error)
> return error;
> - }
>
> phy_y = get_unaligned_be16(&resp[2]);
>
>
Is this really needed? Could you please try to set IRQ_TYPE_EDGE_FALLING
in the device-tree for the interrupt type? I don't think think that
downstream kernel is correct and I've seen weird I2C problems when
IRQ_TYPE_LEVEL_LOW is used.
Powered by blists - more mailing lists