| Message ID | YFgUxG/TljMuVeQ3@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 81c7c0a350bfe9306ad9fb10356534ede8f4ab31 |
| Headers | show |
| Series | Input: serio - make write method mandatory | expand |
On Sun, Mar 21, 2021 at 08:53:40PM -0700, Dmitry Torokhov wrote: > Given that all serio drivers except one implement write() method > let's make it mandatory to avoid testing for its presence whenever > we attempt to use it. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/serio/ams_delta_serio.c | 6 ++++++ > drivers/input/serio/serio.c | 5 +++++ > include/linux/serio.h | 5 +---- > 3 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/input/serio/ams_delta_serio.c b/drivers/input/serio/ams_delta_serio.c > index 1c0be299f179..a1c314897951 100644 > --- a/drivers/input/serio/ams_delta_serio.c > +++ b/drivers/input/serio/ams_delta_serio.c > @@ -89,6 +89,11 @@ static irqreturn_t ams_delta_serio_interrupt(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static int ams_delta_serio_write(struct serio *serio, u8 data) > +{ > + return -EINVAL; > +} > + > static int ams_delta_serio_open(struct serio *serio) > { > struct ams_delta_serio *priv = serio->port_data; > @@ -157,6 +162,7 @@ static int ams_delta_serio_init(struct platform_device *pdev) > priv->serio = serio; > > serio->id.type = SERIO_8042; > + serio->write = ams_delta_serio_write; > serio->open = ams_delta_serio_open; > serio->close = ams_delta_serio_close; > strlcpy(serio->name, "AMS DELTA keyboard adapter", sizeof(serio->name)); > diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c > index 29f491082926..8d229a11bb6b 100644 > --- a/drivers/input/serio/serio.c > +++ b/drivers/input/serio/serio.c > @@ -694,6 +694,11 @@ EXPORT_SYMBOL(serio_reconnect); > */ > void __serio_register_port(struct serio *serio, struct module *owner) > { > + if (!serio->write) { > + pr_err("%s: refusing to register %s without write method\n", > + __func__, serio->name); > + return; > + } > serio_init_port(serio); > serio_queue_event(serio, owner, SERIO_REGISTER_PORT); > } > diff --git a/include/linux/serio.h b/include/linux/serio.h > index 6c27d413da92..075f1b8d76fa 100644 > --- a/include/linux/serio.h > +++ b/include/linux/serio.h > @@ -121,10 +121,7 @@ void serio_unregister_driver(struct serio_driver *drv); > > static inline int serio_write(struct serio *serio, unsigned char data) > { > - if (serio->write) > - return serio->write(serio, data); > - else > - return -1; > + return serio->write(serio, data); > } > > static inline void serio_drv_write_wakeup(struct serio *serio) > -- > 2.31.0.rc2.261.g7f71774620-goog > > > -- > Dmitry This patch as commit 81c7c0a350bf ("Input: serio - make write method mandatory") in -next breaks input for my Hyper-V VM, which prevents me from logging in. I attempted to do something like the following (-1 or -EINVAL) which should be equivalent but it does not resolve the issue. Cheers, Nathan diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c index 1a7b72a9016d..d3eee2d4c327 100644 --- a/drivers/input/serio/hyperv-keyboard.c +++ b/drivers/input/serio/hyperv-keyboard.c @@ -311,6 +311,11 @@ static void hv_kbd_stop(struct serio *serio) spin_unlock_irqrestore(&kbd_dev->lock, flags); } +static int hv_kbd_write(struct serio *serio, u8 data) +{ + return -1; +} + static int hv_kbd_probe(struct hv_device *hv_dev, const struct hv_vmbus_device_id *dev_id) { @@ -341,6 +346,7 @@ static int hv_kbd_probe(struct hv_device *hv_dev, hv_serio->start = hv_kbd_start; hv_serio->stop = hv_kbd_stop; + hv_serio->write = hv_kbd_write; error = vmbus_open(hv_dev->channel, KBD_VSC_SEND_RING_BUFFER_SIZE,
From: Nathan Chancellor <nathan@kernel.org> Sent: Tuesday, July 20, 2021 6:56 PM > > On Sun, Mar 21, 2021 at 08:53:40PM -0700, Dmitry Torokhov wrote: > > Given that all serio drivers except one implement write() method > > let's make it mandatory to avoid testing for its presence whenever > > we attempt to use it. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/input/serio/ams_delta_serio.c | 6 ++++++ > > drivers/input/serio/serio.c | 5 +++++ > > include/linux/serio.h | 5 +---- > > 3 files changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/input/serio/ams_delta_serio.c b/drivers/input/serio/ams_delta_serio.c > > index 1c0be299f179..a1c314897951 100644 > > --- a/drivers/input/serio/ams_delta_serio.c > > +++ b/drivers/input/serio/ams_delta_serio.c > > @@ -89,6 +89,11 @@ static irqreturn_t ams_delta_serio_interrupt(int irq, void *dev_id) > > return IRQ_HANDLED; > > } > > > > +static int ams_delta_serio_write(struct serio *serio, u8 data) > > +{ > > + return -EINVAL; > > +} > > + > > static int ams_delta_serio_open(struct serio *serio) > > { > > struct ams_delta_serio *priv = serio->port_data; > > @@ -157,6 +162,7 @@ static int ams_delta_serio_init(struct platform_device *pdev) > > priv->serio = serio; > > > > serio->id.type = SERIO_8042; > > + serio->write = ams_delta_serio_write; > > serio->open = ams_delta_serio_open; > > serio->close = ams_delta_serio_close; > > strlcpy(serio->name, "AMS DELTA keyboard adapter", sizeof(serio->name)); > > diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c > > index 29f491082926..8d229a11bb6b 100644 > > --- a/drivers/input/serio/serio.c > > +++ b/drivers/input/serio/serio.c > > @@ -694,6 +694,11 @@ EXPORT_SYMBOL(serio_reconnect); > > */ > > void __serio_register_port(struct serio *serio, struct module *owner) > > { > > + if (!serio->write) { > > + pr_err("%s: refusing to register %s without write method\n", > > + __func__, serio->name); > > + return; > > + } > > serio_init_port(serio); > > serio_queue_event(serio, owner, SERIO_REGISTER_PORT); > > } > > diff --git a/include/linux/serio.h b/include/linux/serio.h > > index 6c27d413da92..075f1b8d76fa 100644 > > --- a/include/linux/serio.h > > +++ b/include/linux/serio.h > > @@ -121,10 +121,7 @@ void serio_unregister_driver(struct serio_driver *drv); > > > > static inline int serio_write(struct serio *serio, unsigned char data) > > { > > - if (serio->write) > > - return serio->write(serio, data); > > - else > > - return -1; > > + return serio->write(serio, data); > > } > > > > static inline void serio_drv_write_wakeup(struct serio *serio) > > -- > > 2.31.0.rc2.261.g7f71774620-goog > > > > > > -- > > Dmitry > > This patch as commit 81c7c0a350bf ("Input: serio - make write method > mandatory") in -next breaks input for my Hyper-V VM, which prevents me > from logging in. I attempted to do something like the following (-1 or > -EINVAL) which should be equivalent but it does not resolve the issue. > > Cheers, > Nathan > > diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c > index 1a7b72a9016d..d3eee2d4c327 100644 > --- a/drivers/input/serio/hyperv-keyboard.c > +++ b/drivers/input/serio/hyperv-keyboard.c > @@ -311,6 +311,11 @@ static void hv_kbd_stop(struct serio *serio) > spin_unlock_irqrestore(&kbd_dev->lock, flags); > } > > +static int hv_kbd_write(struct serio *serio, u8 data) > +{ > + return -1; > +} > + > static int hv_kbd_probe(struct hv_device *hv_dev, > const struct hv_vmbus_device_id *dev_id) > { > @@ -341,6 +346,7 @@ static int hv_kbd_probe(struct hv_device *hv_dev, > > hv_serio->start = hv_kbd_start; > hv_serio->stop = hv_kbd_stop; > + hv_serio->write = hv_kbd_write; > > error = vmbus_open(hv_dev->channel, > KBD_VSC_SEND_RING_BUFFER_SIZE, I'm seeing the same problem. I've added the code to hyperv-keyboard.c that Nathan proposed, and that solves the immediate problem in that the "refusing to register" message no longer occurs. But there's now a different problem in that this error is output whenever a key is typed on the Hyper-V synthetic keyboard: [ 11.576716] atkbd serio0: keyboard reset failed on d34b2567-b9b6-42b9-8778-0a4ec0b The Hyper-V keyboard driver depends on the AT Keyboard driver, and there's code in atkbd.c that checks for the existence of the serio->write function. I haven't debugged all the details, but apparently hyperv-keyboard.c depends on atkbd.c finding that function as NULL in order to work properly. See atkbd_connect(). These messages are output during boot when the two drivers are working properly together: [ 2.672693] hv_vmbus: registering driver hyperv_keyboard [ 2.700587] input: AT Translated Set 2 keyboard as /devices/LNXSYSTM:00/LNXSYBUS:00/ACPI0004:00/VMBUS:00/d34b2567-b9b6-42b9-8778-0a4ec0b955bf/serio0/input/input1 I'm not seeing the second message when running the latest linux-next. Michael
On Wed, Jul 21, 2021 at 03:19:36AM +0000, Michael Kelley wrote: > From: Nathan Chancellor <nathan@kernel.org> Sent: Tuesday, July 20, 2021 6:56 PM > > > > On Sun, Mar 21, 2021 at 08:53:40PM -0700, Dmitry Torokhov wrote: > > > Given that all serio drivers except one implement write() method > > > let's make it mandatory to avoid testing for its presence whenever > > > we attempt to use it. > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > --- > > > drivers/input/serio/ams_delta_serio.c | 6 ++++++ > > > drivers/input/serio/serio.c | 5 +++++ > > > include/linux/serio.h | 5 +---- > > > 3 files changed, 12 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/input/serio/ams_delta_serio.c b/drivers/input/serio/ams_delta_serio.c > > > index 1c0be299f179..a1c314897951 100644 > > > --- a/drivers/input/serio/ams_delta_serio.c > > > +++ b/drivers/input/serio/ams_delta_serio.c > > > @@ -89,6 +89,11 @@ static irqreturn_t ams_delta_serio_interrupt(int irq, void *dev_id) > > > return IRQ_HANDLED; > > > } > > > > > > +static int ams_delta_serio_write(struct serio *serio, u8 data) > > > +{ > > > + return -EINVAL; > > > +} > > > + > > > static int ams_delta_serio_open(struct serio *serio) > > > { > > > struct ams_delta_serio *priv = serio->port_data; > > > @@ -157,6 +162,7 @@ static int ams_delta_serio_init(struct platform_device *pdev) > > > priv->serio = serio; > > > > > > serio->id.type = SERIO_8042; > > > + serio->write = ams_delta_serio_write; > > > serio->open = ams_delta_serio_open; > > > serio->close = ams_delta_serio_close; > > > strlcpy(serio->name, "AMS DELTA keyboard adapter", sizeof(serio->name)); > > > diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c > > > index 29f491082926..8d229a11bb6b 100644 > > > --- a/drivers/input/serio/serio.c > > > +++ b/drivers/input/serio/serio.c > > > @@ -694,6 +694,11 @@ EXPORT_SYMBOL(serio_reconnect); > > > */ > > > void __serio_register_port(struct serio *serio, struct module *owner) > > > { > > > + if (!serio->write) { > > > + pr_err("%s: refusing to register %s without write method\n", > > > + __func__, serio->name); > > > + return; > > > + } > > > serio_init_port(serio); > > > serio_queue_event(serio, owner, SERIO_REGISTER_PORT); > > > } > > > diff --git a/include/linux/serio.h b/include/linux/serio.h > > > index 6c27d413da92..075f1b8d76fa 100644 > > > --- a/include/linux/serio.h > > > +++ b/include/linux/serio.h > > > @@ -121,10 +121,7 @@ void serio_unregister_driver(struct serio_driver *drv); > > > > > > static inline int serio_write(struct serio *serio, unsigned char data) > > > { > > > - if (serio->write) > > > - return serio->write(serio, data); > > > - else > > > - return -1; > > > + return serio->write(serio, data); > > > } > > > > > > static inline void serio_drv_write_wakeup(struct serio *serio) > > > -- > > > 2.31.0.rc2.261.g7f71774620-goog > > > > > > > > > -- > > > Dmitry > > > > This patch as commit 81c7c0a350bf ("Input: serio - make write method > > mandatory") in -next breaks input for my Hyper-V VM, which prevents me > > from logging in. I attempted to do something like the following (-1 or > > -EINVAL) which should be equivalent but it does not resolve the issue. > > > > Cheers, > > Nathan > > > > diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c > > index 1a7b72a9016d..d3eee2d4c327 100644 > > --- a/drivers/input/serio/hyperv-keyboard.c > > +++ b/drivers/input/serio/hyperv-keyboard.c > > @@ -311,6 +311,11 @@ static void hv_kbd_stop(struct serio *serio) > > spin_unlock_irqrestore(&kbd_dev->lock, flags); > > } > > > > +static int hv_kbd_write(struct serio *serio, u8 data) > > +{ > > + return -1; > > +} > > + > > static int hv_kbd_probe(struct hv_device *hv_dev, > > const struct hv_vmbus_device_id *dev_id) > > { > > @@ -341,6 +346,7 @@ static int hv_kbd_probe(struct hv_device *hv_dev, > > > > hv_serio->start = hv_kbd_start; > > hv_serio->stop = hv_kbd_stop; > > + hv_serio->write = hv_kbd_write; > > > > error = vmbus_open(hv_dev->channel, > > KBD_VSC_SEND_RING_BUFFER_SIZE, > > I'm seeing the same problem. I've added the code to hyperv-keyboard.c that Nathan > proposed, and that solves the immediate problem in that the "refusing to register" > message no longer occurs. > > But there's now a different problem in that this error is output whenever a key > is typed on the Hyper-V synthetic keyboard: > > [ 11.576716] atkbd serio0: keyboard reset failed on d34b2567-b9b6-42b9-8778-0a4ec0b > > The Hyper-V keyboard driver depends on the AT Keyboard driver, and there's code in > atkbd.c that checks for the existence of the serio->write function. I haven't debugged all > the details, but apparently hyperv-keyboard.c depends on atkbd.c finding that function > as NULL in order to work properly. See atkbd_connect(). These messages are output > during boot when the two drivers are working properly together: > > [ 2.672693] hv_vmbus: registering driver hyperv_keyboard > [ 2.700587] input: AT Translated Set 2 keyboard as /devices/LNXSYSTM:00/LNXSYBUS:00/ACPI0004:00/VMBUS:00/d34b2567-b9b6-42b9-8778-0a4ec0b955bf/serio0/input/input1 > > I'm not seeing the second message when running the latest linux-next. Yeah, the patch is busted as several drivers actually check for the presence of serio->write() to adjust their behavior. I will revert the patch. Thanks!
diff --git a/drivers/input/serio/ams_delta_serio.c b/drivers/input/serio/ams_delta_serio.c index 1c0be299f179..a1c314897951 100644 --- a/drivers/input/serio/ams_delta_serio.c +++ b/drivers/input/serio/ams_delta_serio.c @@ -89,6 +89,11 @@ static irqreturn_t ams_delta_serio_interrupt(int irq, void *dev_id) return IRQ_HANDLED; } +static int ams_delta_serio_write(struct serio *serio, u8 data) +{ + return -EINVAL; +} + static int ams_delta_serio_open(struct serio *serio) { struct ams_delta_serio *priv = serio->port_data; @@ -157,6 +162,7 @@ static int ams_delta_serio_init(struct platform_device *pdev) priv->serio = serio; serio->id.type = SERIO_8042; + serio->write = ams_delta_serio_write; serio->open = ams_delta_serio_open; serio->close = ams_delta_serio_close; strlcpy(serio->name, "AMS DELTA keyboard adapter", sizeof(serio->name)); diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c index 29f491082926..8d229a11bb6b 100644 --- a/drivers/input/serio/serio.c +++ b/drivers/input/serio/serio.c @@ -694,6 +694,11 @@ EXPORT_SYMBOL(serio_reconnect); */ void __serio_register_port(struct serio *serio, struct module *owner) { + if (!serio->write) { + pr_err("%s: refusing to register %s without write method\n", + __func__, serio->name); + return; + } serio_init_port(serio); serio_queue_event(serio, owner, SERIO_REGISTER_PORT); } diff --git a/include/linux/serio.h b/include/linux/serio.h index 6c27d413da92..075f1b8d76fa 100644 --- a/include/linux/serio.h +++ b/include/linux/serio.h @@ -121,10 +121,7 @@ void serio_unregister_driver(struct serio_driver *drv); static inline int serio_write(struct serio *serio, unsigned char data) { - if (serio->write) - return serio->write(serio, data); - else - return -1; + return serio->write(serio, data); } static inline void serio_drv_write_wakeup(struct serio *serio)
Given that all serio drivers except one implement write() method let's make it mandatory to avoid testing for its presence whenever we attempt to use it. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/serio/ams_delta_serio.c | 6 ++++++ drivers/input/serio/serio.c | 5 +++++ include/linux/serio.h | 5 +---- 3 files changed, 12 insertions(+), 4 deletions(-)