Hi Hans,
Thanks for your information.
I will remove the following code on V4 patch.
> + if (f03->ipacketindex >= RELATIVE_PACKET_SIZE)
> + f03->ipacketindex = 0;
Thanks
Marge Yang
-----Original Message-----
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Sent: Monday, August 15, 2022 7:12 PM
To: margeyang <marge.yang@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>; dmitry.torokhov@xxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx
Cc: Marge Yang <Marge.Yang@xxxxxxxxxxxxxxxx>; Derek Cheng <derek.cheng@xxxxxxxxxxxxxxxx>; Vincent Huang <Vincent.huang@xxxxxxxxxxxxxxxx>
Subject: Re: [PATCH V3] Input: synaptics-rmi4 - filter incomplete relative packet.
CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
Hi,
On 8/15/22 12:27, margeyang wrote:
> From: Marge Yang <marge.yang@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/input/rmi4/rmi_f03.c | 77
> +++++++++++++++++++++++++++++++++++-
> 1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/rmi4/rmi_f03.c
> b/drivers/input/rmi4/rmi_f03.c index c194b1664b10..05076db83808 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
> +
> #define PSMOUSE_OOB_EXTRA_BTNS 0x01
>
> +#define RELATIVE_PACKET_SIZE 3
> +
> struct f03_data {
> struct rmi_function *fn;
>
> @@ -33,6 +37,11 @@ struct f03_data {
>
> unsigned int overwrite_buttons;
>
> + int iwritecommandcounter;
> + unsigned int ipacketindex;
> + unsigned int serio_flagsArry[RELATIVE_PACKET_SIZE];
> + u8 ob_dataArry[RELATIVE_PACKET_SIZE];
> +
> u8 device_count;
> u8 rx_queue_length;
> };
> @@ -88,6 +97,7 @@ static int rmi_f03_pt_write(struct serio *id, unsigned char val)
> return error;
> }
>
> + f03->iwritecommandcounter++;
> return 0;
> }
>
> @@ -107,6 +117,8 @@ static int rmi_f03_initialize(struct f03_data *f03)
> return error;
> }
>
> + f03->iwritecommandcounter = 0;
> + f03->ipacketindex = 0;
> f03->device_count = query1 & RMI_F03_DEVICE_COUNT;
> bytes_per_device = (query1 >> RMI_F03_BYTES_PER_DEVICE_SHIFT) &
> RMI_F03_BYTES_PER_DEVICE; @@ -284,6
> +296,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.
> + unsigned char val = RMI_F03_RESET_STYK;
> +
> + error = rmi_write(f03->fn->rmi_dev, f03->fn->fd.data_base_addr, val);
> + if (error) {
> + dev_err(&f03->fn->dev,
> + "%s: Failed to rmi_write to F03 TX register (%d).\n",
> + __func__, error);
> + return error;
> + }
> + f03->ipacketindex = 0;
> + break;
> + }
> +
> if (!(ob_status & RMI_F03_RX_DATA_OFB))
> continue;
>
> @@ -298,7 +326,54 @@ 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 (f03->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 ((f03->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 ((f03->ipacketindex == 0) && !(ob_data & BIT(3))) {
> + dev_err(&f03->fn->dev,
> + "%s: New BIT 3 is not 1 for the first byte\n",
> + __func__);
> + break;
> + }
> + if (f03->ipacketindex >= RELATIVE_PACKET_SIZE)
> + f03->ipacketindex = 0;
(f03->ipacketindex >= RELATIVE_PACKET_SIZE) is never true at this point, since it gets reset to 0 when it reaches RELATIVE_PACKET_SIZE below already.
So these 2 lines can be dropped.
Regards,
Hans
> +
> + f03->ob_dataArry[f03->ipacketindex] = ob_data;
> + f03->serio_flagsArry[f03->ipacketindex] = serio_flags;
> + f03->ipacketindex++;
> +
> + if (f03->ipacketindex == RELATIVE_PACKET_SIZE) {
> + serio_interrupt(f03->serio, f03->ob_dataArry[0],
> + f03->serio_flagsArry[0]);
> + serio_interrupt(f03->serio, f03->ob_dataArry[1],
> + f03->serio_flagsArry[1]);
> + serio_interrupt(f03->serio, f03->ob_dataArry[2],
> + f03->serio_flagsArry[2]);
> + f03->ipacketindex = 0;
> + }
> + }
> + }
> + if (f03->iwritecommandcounter > 0) {
> + f03->ipacketindex = 0;
> + f03->iwritecommandcounter = f03->iwritecommandcounter -
> + 1;
> }
>
> return IRQ_HANDLED;
