| 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: <41ca4d6b-e19c-c039-ed57-716106135e06@redhat.com>
Date: Thu, 11 Aug 2022 16:23:13 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: margeyang <marge.yang@...aptics.corp-partner.google.com>,
dmitry.torokhov@...il.com, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org, benjamin.tissoires@...hat.com
Cc: marge.yang@...synaptics.com, derek.cheng@...synaptcs.com,
vincent.huang@...synaptics.com
Subject: Re: [PATCH] Input: synaptics-rmi4 - filter incomplete relative
packet.
Hi,
On 8/8/22 09:44, margeyang wrote:
> From: Marge Yang <marge.yang@...aptics.corp-partner.google.com>
>
> RMI4 F03 supports the Stick function,
> it's designed to support relative packet.
> This patch supports the following case.
> When relative packet can't be reported completely,
> it may miss one byte or two byte.
> New Synaptics firmware will report PARITY error.
> When timeout error or parity error happens,
> RMI4 driver will sends 0xFE command and
> ask FW to Re-send stick packet again.
>
> Signed-off-by: Marge Yang<marge.yang@...aptics.corp-partner.google.com>
> ---
> drivers/input/rmi4/rmi_f03.c | 82 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 78 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c
> index c194b1664b10..57f03dfcb4ff 100644
> --- a/drivers/input/rmi4/rmi_f03.c
> +++ b/drivers/input/rmi4/rmi_f03.c
> @@ -23,8 +23,12 @@
> #define RMI_F03_BYTES_PER_DEVICE_SHIFT 4
> #define RMI_F03_QUEUE_LENGTH 0x0F
>
> +#define RMI_F03_RESET_STYK 0xFE
Please use tabs in front of the 0xFE to align it with the other values.
> +
> #define PSMOUSE_OOB_EXTRA_BTNS 0x01
>
> +#define RELATIVE_PACKET_SIZE 0x03
Just "3" please since this is a size (not a register value).
> +
> struct f03_data {
> struct rmi_function *fn;
>
> @@ -36,7 +40,8 @@ struct f03_data {
> u8 device_count;
> u8 rx_queue_length;
> };
> -
> +int iwritecommandcounter;
> +unsigned int ipacketindex;
Please do not use global variables like this, instead store these
e.g. inside struct f03_data.
> int rmi_f03_overwrite_button(struct rmi_function *fn, unsigned int button,
> int value)
> {
> @@ -87,7 +92,7 @@ static int rmi_f03_pt_write(struct serio *id, unsigned char val)
> __func__, error);
> return error;
> }
> -
> + iwritecommandcounter++;
> return 0;
> }
>
> @@ -197,10 +202,12 @@ static int rmi_f03_register_pt(struct f03_data *f03)
>
> static int rmi_f03_probe(struct rmi_function *fn)
> {
> +
> struct device *dev = &fn->dev;
> struct f03_data *f03;
> int error;
> -
> + iwritecommandcounter = 0;
> + ipacketindex = 0;
> f03 = devm_kzalloc(dev, sizeof(struct f03_data), GFP_KERNEL);
> if (!f03)
> return -ENOMEM;
If you put the 2 variables into the f03_data then there will be no need
to zero them.
> @@ -251,9 +258,12 @@ static irqreturn_t rmi_f03_attention(int irq, void *ctx)
> const u8 ob_len = f03->rx_queue_length * RMI_F03_OB_SIZE;
> u8 obs[RMI_F03_QUEUE_LENGTH * RMI_F03_OB_SIZE];
> u8 ob_status;
> + static u8 ob_dataArry[RELATIVE_PACKET_SIZE];
> u8 ob_data;
> unsigned int serio_flags;
> + static unsigned int serio_flagsArry[RELATIVE_PACKET_SIZE];
Please drop these 2 static arrays here and instead store the info in
the f03_data struct.
> int i;
> +
Unrelated whitespace change, please drop.
> int error;
>
> if (drvdata->attn_data.data) {
> @@ -284,6 +294,22 @@ static irqreturn_t rmi_f03_attention(int irq, void *ctx)
> ob_data = obs[i + RMI_F03_OB_DATA_OFFSET];
> serio_flags = 0;
>
> + if (ob_status & (RMI_F03_OB_FLAG_TIMEOUT | RMI_F03_OB_FLAG_PARITY)) {
> + // Send resend command to stick when timeout or parity error.
> + // Driver can receive the last stick packet.
> +
> + error = rmi_write(f03->fn->rmi_dev, f03->fn->fd.data_base_addr,
> + RMI_F03_RESET_STYK);
> + if (error) {
> + dev_err(&f03->fn->dev,
> + "%s: Failed to rmi_write to F03 TX register (%d).\n",
> + __func__, error);
> + return error;
> + }
> + ipacketindex = 0;
> + break;
> + }
> +
> if (!(ob_status & RMI_F03_RX_DATA_OFB))
> continue;
>
> @@ -298,9 +324,57 @@ static irqreturn_t rmi_f03_attention(int irq, void *ctx)
> serio_flags & SERIO_TIMEOUT ? 'Y' : 'N',
> serio_flags & SERIO_PARITY ? 'Y' : 'N');
>
> - serio_interrupt(f03->serio, ob_data, serio_flags);
> + if (iwritecommandcounter > 0) {
> + // Read Acknowledge Byte after writing the PS2 command.
> + // It is not trackpoint data.
> + serio_interrupt(f03->serio, ob_data, serio_flags);
> +
> + } else {
> + // The relative-mode PS/2 packet format is as follows:
> + //
> + // bit position position (as array of bytes)
> + // 7 6 5 4 3 2 1 0
> + // =================================+
> + // Yov Xov DY8 DX8 1 M R L | DATA[0]
> + // DX[7:0] | DATA[1]
> + // DY[7:0] | DATA[2]
> + // =================================+
> + // Yov: Y overflow
> + // Xov: X overflow
> + if ((ipacketindex == 0) && (ob_data & ((BIT(7)|BIT(6))))) {
> + dev_err(&f03->fn->dev,
> + "%s: X or Y is overflow. (%x)\n",
> + __func__, ob_data);
> + break;
> + } else if ((ipacketindex == 0) && !(ob_data & BIT(3))) {
> + dev_err(&f03->fn->dev,
> + "%s: New BIT 3 is not 1 for the first byte\n",
> + __func__);
Why no break; here like above ?
> + } else {
> + if (ipacketindex >= RELATIVE_PACKET_SIZE) {
> + ipacketindex = 0;
This means that you are skipping every 4th byte, since you only store
the ob_data + serio_flags the next cycle through the loop!
> + } else {
> + ob_dataArry[ipacketindex] = ob_data;
> + serio_flagsArry[ipacketindex] = serio_flags;
> + ipacketindex++;
> + }
> + if (ipacketindex == RELATIVE_PACKET_SIZE) {
> + serio_interrupt(f03->serio, ob_dataArry[0],
> + serio_flagsArry[0]);
> + serio_interrupt(f03->serio, ob_dataArry[1],
> + serio_flagsArry[1]);
> + serio_interrupt(f03->serio, ob_dataArry[2],
> + serio_flagsArry[2]);
> + ipacketindex = 0;
> + }
> + }
> + }
> }
>
> + if (iwritecommandcounter > 0) {
> + ipacketindex = 0;
> + iwritecommandcounter = iwritecommandcounter - 1;
> + }
You check iwritecommandcounter inside the:
for (i = 0; i < ob_len; i += RMI_F03_OB_SIZE) {
loop, I understand that you want to forward the entire PS/2 response
data and only decrement iwritecommandcounter once, but what if
the rmi_f03_attention() did not contain any OOB data at all ?
I believe that in that case iwritecommandcounter should not be
decremented ?
Regards,
Hans
Powered by blists - more mailing lists