| Message ID | 1660293805-16053-1-git-send-email-marge.yang@synaptics.corp-partner.google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] Input: synaptics-rmi4 - filter incomplete relative packet. | expand |
Hi, On 8/12/22 10:43, margeyang wrote: > From: Marge Yang <marge.yang@synaptics.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@synaptics.corp-partner.google.com> > --- > drivers/input/rmi4/rmi_f03.c | 83 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 79 insertions(+), 4 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c > index c194b1664b10..56b3e1129b51 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; > }; > @@ -87,7 +96,7 @@ static int rmi_f03_pt_write(struct serio *id, unsigned char val) > __func__, error); > return error; > } > - Please keep the empty line you are removing here. > + f03->iwritecommandcounter++; > return 0; > } > > @@ -106,7 +115,8 @@ static int rmi_f03_initialize(struct f03_data *f03) > dev_err(dev, "Failed to read query register (%d).\n", error); > return error; > } > - Please keep the empty line you are removing here. (in general don't make any whitespace changes unrelated to your changes. So if your changes are only adding code, don't delete any whitespace) > + 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 +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); please align the start of "RMI_F03_RESET_STYK" with the '(' of "rmi_write(". > + 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,9 +324,58 @@ 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); > + Why the extra empty line here? please drop this. > + } 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); > + goto exit; You can just use break; here as you did before, please switch back to 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__); > + goto exit; You can just use break; here as you did before, please switch back to break; > + } else { > + if (f03->ipacketindex >= RELATIVE_PACKET_SIZE) > + f03->ipacketindex = 0; > + > + 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; > + } > + } > + } > + } > +exit: > + if (f03->iwritecommandcounter > 0) { > + f03->ipacketindex = 0; > + f03->iwritecommandcounter = f03->iwritecommandcounter - 1; > } > - Please keep the empty line you are removing here. > return IRQ_HANDLED; > } > Other then the few minor style issues above this looks good to me. Regards, Hans
Add Synaptics firmware member Hi Dmitry, Update Synaptics firmware's comment. [Wayne 08/15] To address the transaction error case in the middle would potential lead to the unexpected data transaction and latency between styk device and host driver. F$03 didn't really parse any data packet but simply works as bridge functionality to bypass the command and response packets between styk device and driver. Thanks Marge Yang -----Original Message----- From: Dmitry Torokhov <dmitry.torokhov@gmail.com> Sent: Saturday, August 13, 2022 3:28 AM To: margeyang <marge.yang@synaptics.corp-partner.google.com> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; hdegoede@redhat.com; benjamin.tissoires@redhat.com; Marge Yang <Marge.Yang@tw.synaptics.com>; Derek Cheng <derek.cheng@tw.synaptics.com>; Vincent Huang <Vincent.huang@tw.synaptics.com> Subject: Re: [PATCH V2] 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 Marge, On Fri, Aug 12, 2022 at 04:43:25PM +0800, margeyang wrote: > From: Marge Yang <marge.yang@synaptics.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. My understanding is that F03 is intended to be a pass-through mechanism for PS/2-compatible devices. In that spirit all protocol handling and validation should happen in psmouse driver that attaches to a serio port provided by F03. Historically we did not pay attention to parity and frame errors for PS/2 mice/touchpads (and for keyboards on x86) but we coudl add such code there. Do we actually observe this with RMI devices? Can RMI firmware handle this better instead? Thanks. -- Dmitry
Hi Hans, Thanks for your information. I will fix them on V3 patch. Thanks Marge Yang -----Original Message----- From: Hans de Goede <hdegoede@redhat.com> Sent: Friday, August 12, 2022 6:37 PM To: margeyang <marge.yang@synaptics.corp-partner.google.com>; dmitry.torokhov@gmail.com; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; benjamin.tissoires@redhat.com Cc: Marge Yang <Marge.Yang@tw.synaptics.com>; Derek Cheng <derek.cheng@tw.synaptics.com>; Vincent Huang <Vincent.huang@tw.synaptics.com> Subject: Re: [PATCH V2] 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/12/22 10:43, margeyang wrote: > From: Marge Yang <marge.yang@synaptics.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@synaptics.corp-partner.google.com> > --- > drivers/input/rmi4/rmi_f03.c | 83 > ++++++++++++++++++++++++++++++++++-- > 1 file changed, 79 insertions(+), 4 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_f03.c > b/drivers/input/rmi4/rmi_f03.c index c194b1664b10..56b3e1129b51 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; > }; > @@ -87,7 +96,7 @@ static int rmi_f03_pt_write(struct serio *id, unsigned char val) > __func__, error); > return error; > } > - Please keep the empty line you are removing here. [Marge 08/15] I will correct it on patch V3. > + f03->iwritecommandcounter++; > return 0; > } > > @@ -106,7 +115,8 @@ static int rmi_f03_initialize(struct f03_data *f03) > dev_err(dev, "Failed to read query register (%d).\n", error); > return error; > } > - Please keep the empty line you are removing here. (in general don't make any whitespace changes unrelated to your changes. So if your changes are only adding code, don't delete any whitespace) [Marge 08/15] I will correct it on patch V3. > + 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 > +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); please align the start of "RMI_F03_RESET_STYK" with the '(' of "rmi_write(". [Marge 08/15] I will correct it on patch V3. > + 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,9 +324,58 @@ 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); > + Why the extra empty line here? please drop this. [Marge 08/15] I will correct it on patch V3. > + } 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); > + goto exit; You can just use break; here as you did before, please switch back to break; [Marge 08/15] I will correct it on patch V3. > + } 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__); > + goto exit; You can just use break; here as you did before, please switch back to break; [Marge 08/15] I will correct it on patch V3. > + } else { > + if (f03->ipacketindex >= RELATIVE_PACKET_SIZE) > + f03->ipacketindex = 0; > + > + 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; > + } > + } > + } > + } > +exit: > + if (f03->iwritecommandcounter > 0) { > + f03->ipacketindex = 0; > + f03->iwritecommandcounter = f03->iwritecommandcounter - > +1; > } > - Please keep the empty line you are removing here. [Marge 08/15] I will correct it on patch V3. > return IRQ_HANDLED; > } > Other then the few minor style issues above this looks good to me. Regards, Hans
Hi Dmitry, May I know if you have any concerns about FW's feedback? Thanks Marge Yang -----Original Message----- From: Marge Yang Sent: Monday, August 15, 2022 2:50 PM To: Dmitry Torokhov <dmitry.torokhov@gmail.com>; margeyang <marge.yang@synaptics.corp-partner.google.com>; Wayne Chang <wayne.chang@tw.synaptics.com>; Marge Yang <Marge.Yang@tw.synaptics.com> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; hdegoede@redhat.com; benjamin.tissoires@redhat.com; Kevin Chu <kevin.chu@tw.synaptics.com>; Derek Cheng <derek.cheng@tw.synaptics.com>; Vincent Huang <Vincent.huang@tw.synaptics.com>; Carl Yang <Carl.Yang@synaptics.com>; Ian Lu <Ian.Lu@tw.synaptics.com>; Darren Kang <Darren.Kang@tw.synaptics.com> Subject: RE: [PATCH V2] Input: synaptics-rmi4 - filter incomplete relative packet. Add Synaptics firmware member Hi Dmitry, Update Synaptics firmware's comment. [Wayne 08/15] To address the transaction error case in the middle would potential lead to the unexpected data transaction and latency between styk device and host driver. F$03 didn't really parse any data packet but simply works as bridge functionality to bypass the command and response packets between styk device and driver. Thanks Marge Yang -----Original Message----- From: Dmitry Torokhov <dmitry.torokhov@gmail.com> Sent: Saturday, August 13, 2022 3:28 AM To: margeyang <marge.yang@synaptics.corp-partner.google.com> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; hdegoede@redhat.com; benjamin.tissoires@redhat.com; Marge Yang <Marge.Yang@tw.synaptics.com>; Derek Cheng <derek.cheng@tw.synaptics.com>; Vincent Huang <Vincent.huang@tw.synaptics.com> Subject: Re: [PATCH V2] 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 Marge, On Fri, Aug 12, 2022 at 04:43:25PM +0800, margeyang wrote: > From: Marge Yang <marge.yang@synaptics.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. My understanding is that F03 is intended to be a pass-through mechanism for PS/2-compatible devices. In that spirit all protocol handling and validation should happen in psmouse driver that attaches to a serio port provided by F03. Historically we did not pay attention to parity and frame errors for PS/2 mice/touchpads (and for keyboards on x86) but we coudl add such code there. Do we actually observe this with RMI devices? Can RMI firmware handle this better instead? Thanks. -- Dmitry
diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c index c194b1664b10..56b3e1129b51 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; }; @@ -87,7 +96,7 @@ static int rmi_f03_pt_write(struct serio *id, unsigned char val) __func__, error); return error; } - + f03->iwritecommandcounter++; return 0; } @@ -106,7 +115,8 @@ static int rmi_f03_initialize(struct f03_data *f03) dev_err(dev, "Failed to read query register (%d).\n", error); 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 +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; + } + f03->ipacketindex = 0; + break; + } + if (!(ob_status & RMI_F03_RX_DATA_OFB)) continue; @@ -298,9 +324,58 @@ 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); + goto exit; + } 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__); + goto exit; + } else { + if (f03->ipacketindex >= RELATIVE_PACKET_SIZE) + f03->ipacketindex = 0; + + 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; + } + } + } + } +exit: + if (f03->iwritecommandcounter > 0) { + f03->ipacketindex = 0; + f03->iwritecommandcounter = f03->iwritecommandcounter - 1; } - return IRQ_HANDLED; }