Dmitry Torokhov wrote: > On Wed, Aug 28, 2024 at 12:15:24PM -0700, Dmitry Torokhov wrote: >> On Tue, Aug 27, 2024 at 10:46:11PM -0400, Woody Suwalski wrote: >>> Dmitry Torokhov wrote: >>>> Hi Woody, >>>> >>>> On Tue, Aug 27, 2024 at 07:44:12PM -0400, Woody Suwalski wrote: >>>>> Kernel 6.11 rcN on Dell XPS 15Z:  touch pad has stopped working after the >>>>> patch >>>>> >>>>> commit 8bccf667f62a2351fd0b2a2fe5ba90806702c048 >>>>> Author: Dmitry Torokhov >>>>> Date:   Fri Jun 28 15:47:25 2024 -0700 >>>>> >>>>>     Input: cypress_ps2 - report timeouts when reading command status >>>>> >>>>> It seems that the first communication is with an invalid packet of 3 NULLs, >>>>> and that >>>>> status failure used to be ignored. With the above patch it is now returning >>>>> an error and >>>>> that results in a dead touch pad. >>>>> >>>>> The fix is to stop flagging an error for 3-byte null packets, just keep >>>>> ignoring them as before. >>>>> [    2.338016] [    T591] err: Command 0x00 response data (0x): 00 00 00 >>>>> [    2.338032] [    T591] ok: Command 0x00 response data (0x): 33 cc a2 >>>>> ... >>>>> [    2.770029] [    T591] ok: Command 0x00 response data (0x): 33 cc a2 >>>>> [    2.998030] [    T591] ok: Command 0x11 response data (0x): 01 00 64 >>>> Could you please send me logs with i8042.debug=1 kernel command line >>>> option please? I wonder if we need to wake up the controller... >>>> >>>> Thanks. >>>> >>> Sure, the dmesg log is attached (for the failing scenario) >> Thank you. >> >>> [ 0.000000] [ T0] Linux version 6.9.0-pingu+ (root@DellXPS15Z) (gcc (Debian 14.2.0-1) 14.2.0, GNU ld (GNU Binutils for Debian) 2.43.1) #24 SMP PREEMPT_DYNAMIC Tue Aug 27 22:33:33 EDT 2024 >> This is not 6.11, did you patch 6.9 with cypress patches from 6.11? No, this is just inside git bisect tree. Whatever name git has chosen :-) >> >> Anyway, the patch you posted does not make sense. You are doing the >> following check: >> >> + if (!(pktsize == 3 && param[0] == 0 && param[1] == 0 )) { >> + rc = -ETIMEDOUT; >> + goto out; >> + } >> >> trying to ignore "all zeroes" response from the device, however at this >> point param array does not contain data from the device, it always >> contains all zeroes because of memset() a few lines above. So in effect >> you always skipping reporting timeout. I was just blindly following the trace output I have got during debugging. First line had 00 00 00. And the workaround did work... >> However I think cypress is busted in general as it looks like it times >> out all the commands, because it tries to issue them outside of libp2s, >> and so noone is actually wakes it up when we get enough response from >> the device. To prove this could you please try applying this: >> >> diff --git a/drivers/input/mouse/cypress_ps2.c b/drivers/input/mouse/cypress_ps2.c >> index b3c34ebcc4ef..8c0c7100aa4d 100644 >> --- a/drivers/input/mouse/cypress_ps2.c >> +++ b/drivers/input/mouse/cypress_ps2.c >> @@ -115,8 +115,9 @@ static int cypress_ps2_read_cmd_status(struct psmouse *psmouse, >> if (!wait_event_timeout(ps2dev->wait, >> psmouse->pktcnt >= pktsize, >> msecs_to_jiffies(CYTP_CMD_TIMEOUT))) { >> - rc = -ETIMEDOUT; >> - goto out; >> +// rc = -ETIMEDOUT; >> +// goto out; >> + pr_err("XXX looks like timeout\n"); >> } >> >> memcpy(param, psmouse->packet, pktsize); >> >> and let me know if you see "XXX looks like timeout" multiple times >> during initialization (essentially for each extended command issued by >> the cypress driver)? >> >> Thanks! That is more or less the first test I did during debugging. As a result there is a lot of error output, but the touchpad does work. See dmesg.cypress.timeout.txt > And I wonder if this fixes it properly: > > diff --git a/drivers/input/mouse/cypress_ps2.c b/drivers/input/mouse/cypress_ps2.c > index b3c34ebcc4ef..9446657a5f35 100644 > --- a/drivers/input/mouse/cypress_ps2.c > +++ b/drivers/input/mouse/cypress_ps2.c > @@ -91,48 +91,6 @@ static int cypress_ps2_ext_cmd(struct psmouse *psmouse, u8 prefix, u8 nibble) > return rc; > } > > -static int cypress_ps2_read_cmd_status(struct psmouse *psmouse, > - u8 cmd, u8 *param) > -{ > - struct ps2dev *ps2dev = &psmouse->ps2dev; > - enum psmouse_state old_state; > - int pktsize; > - int rc; > - > - ps2_begin_command(ps2dev); > - > - old_state = psmouse->state; > - psmouse->state = PSMOUSE_CMD_MODE; > - psmouse->pktcnt = 0; > - > - pktsize = (cmd == CYTP_CMD_READ_TP_METRICS) ? 8 : 3; > - memset(param, 0, pktsize); > - > - rc = cypress_ps2_sendbyte(psmouse, PSMOUSE_CMD_GETINFO & 0xff); > - if (rc) > - goto out; > - > - if (!wait_event_timeout(ps2dev->wait, > - psmouse->pktcnt >= pktsize, > - msecs_to_jiffies(CYTP_CMD_TIMEOUT))) { > - rc = -ETIMEDOUT; > - goto out; > - } > - > - memcpy(param, psmouse->packet, pktsize); > - > - psmouse_dbg(psmouse, "Command 0x%02x response data (0x): %*ph\n", > - cmd, pktsize, param); > - > -out: > - psmouse->state = old_state; > - psmouse->pktcnt = 0; > - > - ps2_end_command(ps2dev); > - > - return rc; > -} > - > static bool cypress_verify_cmd_state(struct psmouse *psmouse, u8 cmd, u8* param) > { > bool rate_match = false; > @@ -166,6 +124,8 @@ static bool cypress_verify_cmd_state(struct psmouse *psmouse, u8 cmd, u8* param) > static int cypress_send_ext_cmd(struct psmouse *psmouse, u8 cmd, u8 *param) > { > u8 cmd_prefix = PSMOUSE_CMD_SETRES & 0xff; > + unsigned int resp_size = cmd == CYTP_CMD_READ_TP_METRICS ? 8 : 3; > + unsigned int ps2_cmd = (PSMOUSE_CMD_GETINFO & 0xff) | (resp_size << 8); > int tries = CYTP_PS2_CMD_TRIES; > int error; > > @@ -179,10 +139,18 @@ static int cypress_send_ext_cmd(struct psmouse *psmouse, u8 cmd, u8 *param) > cypress_ps2_ext_cmd(psmouse, cmd_prefix, DECODE_CMD_BB(cmd)); > cypress_ps2_ext_cmd(psmouse, cmd_prefix, DECODE_CMD_AA(cmd)); > > - error = cypress_ps2_read_cmd_status(psmouse, cmd, param); > - if (!error && cypress_verify_cmd_state(psmouse, cmd, param)) > - return 0; > + error = ps2_command(&psmouse->ps2dev, param, ps2_cmd); > + if (error) { > + psmouse_dbg(psmouse, "Command 0x%02x failed: %d\n", > + cmd, error); > + } else { > + psmouse_dbg(psmouse, > + "Command 0x%02x response data (0x): %*ph\n", > + cmd, resp_size, param); > > + if (cypress_verify_cmd_state(psmouse, cmd, param)) > + return 0; > + } > } while (--tries > 0); > > return -EIO; > > Thanks. > Thanks, the new algorithm seems to be working ok. See the trace in dmesg.cypress.new.txt. The patch looks good. I would suggest using brackets to remove ambiguity in     unsigned int resp_size = (cmd == CYTP_CMD_READ_TP_METRICS) ? 8 : 3; Tested_by: Woody Suwalski