| 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: <925cc416-e8e0-4a68-addb-8a7d11d3895c@linaro.org>
Date: Mon, 23 Oct 2023 16:04:35 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: linux-input@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Bastien Nocera <hadess@...ess.net>,
Hans de Goede <hdegoede@...hat.com>,
Henrik Rydberg <rydberg@...math.org>,
Jeff LaBundy <jeff@...undy.com>, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 2/4] Input: add core support for Goodix Berlin
Touchscreen IC
Hi Dmitry,
On 23/10/2023 06:37, Dmitry Torokhov wrote:
> Hi Neil,
>
> On Sat, Oct 21, 2023 at 01:09:24PM +0200, Neil Armstrong wrote:
>> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
>> +{
>> + u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
>
> You probably already saw the kernel test robot message, I think we
> should allocate this buffer in the heap (and free it once its no longer
> needed).
Indeed I haven't received it on 9 patch submitting, anyway moved it to head.
>
>> + __le16 length_raw;
>> + u16 length;
>> + int error;
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
>> + &length_raw, sizeof(length_raw));
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info length, %d\n", error);
>> + return error;
>> + }
>> +
>> + length = le16_to_cpu(length_raw);
>> + if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
>> + dev_info(cd->dev, "invalid ic info length %d\n", length);
>> + return -EINVAL;
>> + }
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
>> + afe_data, length);
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info data, %d\n", error);
>> + return error;
>> + }
>> +
>> + /* check whether the data is valid (ex. bus default values) */
>> + if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
>
> This cast is not needed.
>
>> + dev_err(cd->dev, "fw info data invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
>
> This cast is not needed either.
>
>> + dev_info(cd->dev, "fw info checksum error\n");
>> + return -EINVAL;
>> + }
>> +
>> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
>> + if (error)
>> + return error;
>> +
>> + /* check some key info */
>> + if (!cd->touch_data_addr) {
>> + dev_err(cd->dev, "touch_data_addr is null\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
>> + const void *buf, int touch_num)
>> +{
>> + const struct goodix_berlin_touch_data *touch_data = buf;
>> + int i;
>> +
>> + /* Check for data validity */
>> + for (i = 0; i < touch_num; i++) {
>> + unsigned int id;
>> +
>> + id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
>> +
>> + if (id >= GOODIX_BERLIN_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid finger id %d\n", id);
>> + return;
>
> Is it important to abort entire packet if one if the slots has incorrect
> data? Can we simply skip over these contacts?
Indeed, merged the for loops and simply skip the invalid finger id.
>
>> + }
>> + }
>> +
>> + /* Report finger touches */
>> + for (i = 0; i < touch_num; i++) {
>> + input_mt_slot(cd->input_dev,
>> + FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
>> + touch_data[i].id));
>> + input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
>> +
>> + touchscreen_report_pos(cd->input_dev, &cd->props,
>> + __le16_to_cpu(touch_data[i].x),
>> + __le16_to_cpu(touch_data[i].y),
>> + true);
>> + input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
>> + __le16_to_cpu(touch_data[i].w));
>> + }
>> +
>> + input_mt_sync_frame(cd->input_dev);
>> + input_sync(cd->input_dev);
>> +}
>> +
>> +static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
>> + const void *pre_buf, u32 pre_buf_len)
>> +{
>> + static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
>
> No, please no static data. The drivers should be ready to handle more
> than one device on a system. If the buffer is large-ish put it into
> goodix_berlin_core.
This is a typo from vendor kernel, I didn't want a static buffer here...
The buffer is only 26 bytes, it's small enough to stay here in non-static.
>
>
>> + u8 point_type, touch_num;
>> + int error;
>> +
>> + /* copy pre-data to buffer */
>> + memcpy(buffer, pre_buf, pre_buf_len);
>> +
>> + touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
>> + buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
>> +
>> + if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
>> + return;
>> + }
>> +
>> + if (touch_num) {
>> + /* read more data if more than 2 touch events */
>> + if (unlikely(touch_num > 2)) {
>> + error = regmap_raw_read(cd->regmap,
>> + cd->touch_data_addr + pre_buf_len,
>> + &buffer[pre_buf_len],
>> + (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
>> + if (error) {
>> + dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
>> + error);
>> + return;
>> + }
>> + }
>> +
>> + point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
>> + buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
>> +
>> + if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
>> + point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
>> + dev_warn_once(cd->dev, "Stylus event type not handled\n");
>> + return;
>> + }
>> +
>> + if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
>> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
>> + dev_err(cd->dev, "touch data checksum error, data: %*ph\n",
>> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
>> + &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
>> + return;
>> + }
>> + }
>> +
>> + goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
>> + touch_num);
>> +}
>> +
>> +static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
>> +{
>> + gpiod_set_value(cd->reset_gpio, 1);
>> + usleep_range(2000, 2100);
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
>
> The reset line handling is optional, we should skip waits if there is
> no GPIO defined for the board.
Ack, instead I only call this if gpio is valid.
>
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
>> +{
>> + struct goodix_berlin_core *cd = data;
>> + u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
>> + u8 event_status;
>> + int error;
>> +
>> + /* First, read buffer with space for 2 touch events */
>> + error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
>> + GOODIX_BERLIN_IRQ_READ_LEN(2));
>> + if (error) {
>> + dev_err_ratelimited(cd->dev, "failed get event head data, %d\n", error);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
>> + return IRQ_HANDLED;
>> +
>> + if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
>> + dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
>> + GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
>> +
>> + if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
>> + goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
>> +
>> + if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
>> + switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
>> + case GOODIX_BERLIN_REQUEST_CODE_RESET:
>> + goodix_berlin_request_handle_reset(cd);
>> + break;
>> +
>> + default:
>> + dev_warn(cd->dev, "unsupported request code 0x%x\n",
>> + buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
>> + }
>> + }
>> +
>> + /* Clear up status field */
>> + regmap_write(cd->regmap, cd->touch_data_addr, 0);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
>> + const struct input_id *id)
>> +{
>> + struct input_dev *input_dev;
>> + int error;
>> +
>> + input_dev = devm_input_allocate_device(cd->dev);
>> + if (!input_dev)
>> + return -ENOMEM;
>> +
>> + cd->input_dev = input_dev;
>> + input_set_drvdata(input_dev, cd);
>> +
>> + input_dev->name = "Goodix Berlin Capacitive TouchScreen";
>> + input_dev->phys = "input/ts";
>> +
>> + input_dev->id = *id;
>> +
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>> +
>> + touchscreen_parse_properties(cd->input_dev, true, &cd->props);
>> +
>> + error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
>> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
>> + if (error)
>> + return error;
>> +
>> + return input_register_device(cd->input_dev);
>
> Please in functions with multiple possible failure paths use format
>
> error = op(...);
> if (error)
> return error;
>
> return 0;
Ack, will check for this in the whole patchset.
Thanks for review,
Neil
>
> Thanks.
>
Powered by blists - more mailing lists