| Message ID | 20231004005729.3943515-1-jefferymiller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: elantech - fix fast_reconnect callback in ps2 mode | expand |
On 04.10.23 02:57, Jeffery Miller wrote: > Make `elantech_setup_ps2` set a compatible fast_reconnect pointer > when its ps2 mode is used. > > When an SMBus connection is attempted and fails `psmouse_smbus_init` > sets fast_reconnect to `psmouse_smbus_reconnect`. > `psmouse_smbus_reconnect` expects `psmouse->private` to be > `struct psmouse_smbus_dev` but `elantech_setup_ps2` replaces > it with its private data. This was causing an issue on resume > since psmouse_smbus_reconnect was being called while in ps2, not SMBus > mode. > > This was uncovered by commit 92e24e0e57f7 ("Input: psmouse - add delay when > deactivating for SMBus mode") Ahh, thx for investigating. This fixes things for me. Feel free to add: Tested-by: Thorsten Leemhuis <linux@leemhuis.info> > Closes: > Link:https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/ > Reported-by: Thorsten Leemhuis <linux@leemhuis.info> Side note: something went sideways here, it afaics should look something like this: """ > Reported-by: Thorsten Leemhuis <linux@leemhuis.info> > Closes: https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/ """ Thx again. Ciao, Thorsten
On Tue, Oct 03, 2023 at 07:57:24PM -0500, Jeffery Miller wrote: > Make `elantech_setup_ps2` set a compatible fast_reconnect pointer > when its ps2 mode is used. > > When an SMBus connection is attempted and fails `psmouse_smbus_init` > sets fast_reconnect to `psmouse_smbus_reconnect`. > `psmouse_smbus_reconnect` expects `psmouse->private` to be > `struct psmouse_smbus_dev` but `elantech_setup_ps2` replaces > it with its private data. This was causing an issue on resume > since psmouse_smbus_reconnect was being called while in ps2, not SMBus > mode. > > This was uncovered by commit 92e24e0e57f7 ("Input: psmouse - add delay when > deactivating for SMBus mode") > > Closes: > Link:https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/ > Reported-by: Thorsten Leemhuis <linux@leemhuis.info> > > Signed-off-by: Jeffery Miller <jefferymiller@google.com> > --- > > The other callbacks set in psmouse_smbus_init are already replaced. > Should fast_reconnect be set to `elantech_reconnect` instead? What commit id does this fix? Should it also have a cc: stable tag? thanks, greg k-h
On 04.10.23 08:19, Greg Kroah-Hartman wrote: > On Tue, Oct 03, 2023 at 07:57:24PM -0500, Jeffery Miller wrote: >> Make `elantech_setup_ps2` set a compatible fast_reconnect pointer >> when its ps2 mode is used. >> >> When an SMBus connection is attempted and fails `psmouse_smbus_init` >> sets fast_reconnect to `psmouse_smbus_reconnect`. >> `psmouse_smbus_reconnect` expects `psmouse->private` to be >> `struct psmouse_smbus_dev` but `elantech_setup_ps2` replaces >> it with its private data. This was causing an issue on resume >> since psmouse_smbus_reconnect was being called while in ps2, not SMBus >> mode. >> >> This was uncovered by commit 92e24e0e57f7 ("Input: psmouse - add delay when >> deactivating for SMBus mode") >> >> Closes: >> Link:https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/ >> Reported-by: Thorsten Leemhuis <linux@leemhuis.info> >> >> Signed-off-by: Jeffery Miller <jefferymiller@google.com> >> --- >> >> The other callbacks set in psmouse_smbus_init are already replaced. >> Should fast_reconnect be set to `elantech_reconnect` instead? > > What commit id does this fix? Good point, yes, it also needs this: Fixes: 92e24e0e57f72e ("Input: psmouse - add delay when deactivating for SMBus mode") > Should it also have a cc: stable tag? Not that I can see, as that commit was merged for 6.6 and not backported (no idea why Jeffery CCed the stable list, maybe I'm missing something) Ciao, Thorsten
Hi Jeffery, On Tue, Oct 03, 2023 at 07:57:24PM -0500, Jeffery Miller wrote: > Make `elantech_setup_ps2` set a compatible fast_reconnect pointer > when its ps2 mode is used. > > When an SMBus connection is attempted and fails `psmouse_smbus_init` > sets fast_reconnect to `psmouse_smbus_reconnect`. > `psmouse_smbus_reconnect` expects `psmouse->private` to be > `struct psmouse_smbus_dev` but `elantech_setup_ps2` replaces > it with its private data. This was causing an issue on resume > since psmouse_smbus_reconnect was being called while in ps2, not SMBus > mode. > > This was uncovered by commit 92e24e0e57f7 ("Input: psmouse - add delay when > deactivating for SMBus mode") Nice find, thank you. > > Closes: > Link:https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/ > Reported-by: Thorsten Leemhuis <linux@leemhuis.info> > > Signed-off-by: Jeffery Miller <jefferymiller@google.com> > --- > > The other callbacks set in psmouse_smbus_init are already replaced. > Should fast_reconnect be set to `elantech_reconnect` instead? No, doing PS/2 Elantech reinitialization is not a "fast" operation, as it takes a while to communicate with/query the device, so we should not be using elantech_reconnect() as a "fast" reconnect handler. In fact, now that I think about it more, we should rework the original patch that added the delay, so that we do not wait these 30 msec in the "fast" reconnect handler. It turns out your original approach was better, but we should not be using retries, but rather the existing reset_delay_ms already defined in rmi platform data. I would appreciate if you try the draft patch at the end of this email (to be applied after reverting your original one adding the delay in psmouse-smbus.c). > > > drivers/input/mouse/elantech.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c > index 2118b2075f43..4e38229404b4 100644 > --- a/drivers/input/mouse/elantech.c > +++ b/drivers/input/mouse/elantech.c > @@ -2114,6 +2114,7 @@ static int elantech_setup_ps2(struct psmouse *psmouse, > psmouse->protocol_handler = elantech_process_byte; > psmouse->disconnect = elantech_disconnect; > psmouse->reconnect = elantech_reconnect; > + psmouse->fast_reconnect = NULL; I think we need a similar change in synaptics.c as that one also can fall back to PS/2 mode. Thanks! --- diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c index ada299ec5bba..6ccc4a099b51 100644 --- a/drivers/input/mouse/synaptics.c +++ b/drivers/input/mouse/synaptics.c @@ -1752,6 +1752,7 @@ static int synaptics_create_intertouch(struct psmouse *psmouse, psmouse_matches_pnp_id(psmouse, topbuttonpad_pnp_ids) && !SYN_CAP_EXT_BUTTONS_STICK(info->ext_cap_10); const struct rmi_device_platform_data pdata = { + .reset_delay_ms = 30, .sensor_pdata = { .sensor_type = rmi_sensor_touchpad, .axis_align.flip_y = true, diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c index 7059a2762aeb..b0b099b5528a 100644 --- a/drivers/input/rmi4/rmi_smbus.c +++ b/drivers/input/rmi4/rmi_smbus.c @@ -235,12 +235,29 @@ static void rmi_smb_clear_state(struct rmi_smb_xport *rmi_smb) static int rmi_smb_enable_smbus_mode(struct rmi_smb_xport *rmi_smb) { - int retval; + struct i2c_client *client = rmi_smb->client; + int smbus_version; + + /* + * psmouse driver resets the controller, we only need to wait + * to give the firmware chance to fully reinitialize. + */ + if (rmi_smb->xport.pdata.reset_delay_ms) + msleep(rmi_smb->xport.pdata.reset_delay_ms); /* we need to get the smbus version to activate the touchpad */ - retval = rmi_smb_get_version(rmi_smb); - if (retval < 0) - return retval; + smbus_version = rmi_smb_get_version(rmi_smb); + if (smbus_version < 0) + return smbus_version; + + rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Smbus version is %d", + smbus_version); + + if (smbus_version != 2 && smbus_version != 3) { + dev_err(&client->dev, "Unrecognized SMB version %d\n", + smbus_version); + return -ENODEV; + } return 0; } @@ -253,11 +270,10 @@ static int rmi_smb_reset(struct rmi_transport_dev *xport, u16 reset_addr) rmi_smb_clear_state(rmi_smb); /* - * we do not call the actual reset command, it has to be handled in - * PS/2 or there will be races between PS/2 and SMBus. - * PS/2 should ensure that a psmouse_reset is called before - * intializing the device and after it has been removed to be in a known - * state. + * We do not call the actual reset command, it has to be handled in + * PS/2 or there will be races between PS/2 and SMBus. PS/2 should + * ensure that a psmouse_reset is called before initializing the + * device and after it has been removed to be in a known state. */ return rmi_smb_enable_smbus_mode(rmi_smb); } @@ -272,7 +288,6 @@ static int rmi_smb_probe(struct i2c_client *client) { struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev); struct rmi_smb_xport *rmi_smb; - int smbus_version; int error; if (!pdata) { @@ -311,18 +326,9 @@ static int rmi_smb_probe(struct i2c_client *client) rmi_smb->xport.proto_name = "smb"; rmi_smb->xport.ops = &rmi_smb_ops; - smbus_version = rmi_smb_get_version(rmi_smb); - if (smbus_version < 0) - return smbus_version; - - rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Smbus version is %d", - smbus_version); - - if (smbus_version != 2 && smbus_version != 3) { - dev_err(&client->dev, "Unrecognized SMB version %d\n", - smbus_version); - return -ENODEV; - } + error = rmi_smb_enable_smbus_mode(rmi_smb); + if (error) + return error; i2c_set_clientdata(client, rmi_smb);
Dmitry, On Wed, Oct 4, 2023 at 9:11 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > In fact, now that I think about it more, we should rework the original > patch that added the delay, so that we do not wait these 30 msec in the > "fast" reconnect handler. It turns out your original approach was > better, but we should not be using retries, but rather the existing > reset_delay_ms already defined in rmi platform data. I would appreciate > if you try the draft patch at the end of this email (to be applied after > reverting your original one adding the delay in psmouse-smbus.c). I tested the draft patch and it works. I did revert the previous delay patch while testing it. > > I think we need a similar change in synaptics.c as that one also can > fall back to PS/2 mode. > Ah, good point, yes it does appear this needs to be done as well. I have tested and will post an new version of the patch to include the fix in synaptics.c as well. Thanks, Jeff
On Wed, Oct 4, 2023 at 1:19 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > What commit id does this fix? Should it also have a cc: stable tag? > A crash was observed after commit 92e24e0e57f7 ("Input: psmouse - add delay when deactivating for SMBus mode") which is not in stable. I don't think so. The underlying issue was introduced in commit 8eb92e5c9133 ("Input: psmouse - add support for SMBus companions") but wasn't causing a noticed problem.
On 05.10.23 02:13, Jeffery Miller wrote: > > On Wed, Oct 4, 2023 at 9:11 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: >> >> In fact, now that I think about it more, we should rework the original >> patch that added the delay, so that we do not wait these 30 msec in the >> "fast" reconnect handler. It turns out your original approach was >> better, but we should not be using retries, but rather the existing >> reset_delay_ms already defined in rmi platform data. I would appreciate >> if you try the draft patch at the end of this email (to be applied after >> reverting your original one adding the delay in psmouse-smbus.c). > I tested the draft patch and it works. I did revert the previous delay > patch while testing it. > >> I think we need a similar change in synaptics.c as that one also can >> fall back to PS/2 mode. >> > Ah, good point, yes it does appear this needs to be done as well. > I have tested and will post an new version of the patch to include > the fix in synaptics.c as well. As I'm affected by this problem (and somebody else reported to me in private to be affected as well) and nothing afaics happened in the past few days a quick question: What's the way forward here now that -rc6 slowly comes into sight? Apply Jeff's patch to fix my problem? Revert the culprit and fix this properly up with Dmitry's and Jeff's patches in the next cycle? Something else? Ciao, Thorsten
Hi Thorsten, On Tue, Oct 10, 2023 at 09:08:23AM +0200, Thorsten Leemhuis wrote: > On 05.10.23 02:13, Jeffery Miller wrote: > > > > On Wed, Oct 4, 2023 at 9:11 AM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > >> > >> In fact, now that I think about it more, we should rework the original > >> patch that added the delay, so that we do not wait these 30 msec in the > >> "fast" reconnect handler. It turns out your original approach was > >> better, but we should not be using retries, but rather the existing > >> reset_delay_ms already defined in rmi platform data. I would appreciate > >> if you try the draft patch at the end of this email (to be applied after > >> reverting your original one adding the delay in psmouse-smbus.c). > > I tested the draft patch and it works. I did revert the previous delay > > patch while testing it. > > > >> I think we need a similar change in synaptics.c as that one also can > >> fall back to PS/2 mode. > >> > > Ah, good point, yes it does appear this needs to be done as well. > > I have tested and will post an new version of the patch to include > > the fix in synaptics.c as well. > > As I'm affected by this problem (and somebody else reported to me in > private to be affected as well) and nothing afaics happened in the past > few days a quick question: > > What's the way forward here now that -rc6 slowly comes into sight? Apply > Jeff's patch to fix my problem? Revert the culprit and fix this properly > up with Dmitry's and Jeff's patches in the next cycle? Something else? I will revert the original patch introducing the delay now that we argee there is a better way. In the mean time I will merge Jeff's patch to reset fast_reconnect handlers, as it is right thing to do anyway, and will get into shape the patch setting reset-delay in RMI code. Thanks.
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c index 2118b2075f43..4e38229404b4 100644 --- a/drivers/input/mouse/elantech.c +++ b/drivers/input/mouse/elantech.c @@ -2114,6 +2114,7 @@ static int elantech_setup_ps2(struct psmouse *psmouse, psmouse->protocol_handler = elantech_process_byte; psmouse->disconnect = elantech_disconnect; psmouse->reconnect = elantech_reconnect; + psmouse->fast_reconnect = NULL; psmouse->pktsize = info->hw_version > 1 ? 6 : 4; return 0;
Make `elantech_setup_ps2` set a compatible fast_reconnect pointer when its ps2 mode is used. When an SMBus connection is attempted and fails `psmouse_smbus_init` sets fast_reconnect to `psmouse_smbus_reconnect`. `psmouse_smbus_reconnect` expects `psmouse->private` to be `struct psmouse_smbus_dev` but `elantech_setup_ps2` replaces it with its private data. This was causing an issue on resume since psmouse_smbus_reconnect was being called while in ps2, not SMBus mode. This was uncovered by commit 92e24e0e57f7 ("Input: psmouse - add delay when deactivating for SMBus mode") Closes: Link:https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/ Reported-by: Thorsten Leemhuis <linux@leemhuis.info> Signed-off-by: Jeffery Miller <jefferymiller@google.com> --- The other callbacks set in psmouse_smbus_init are already replaced. Should fast_reconnect be set to `elantech_reconnect` instead? drivers/input/mouse/elantech.c | 1 + 1 file changed, 1 insertion(+)