diff mbox series

Input: serio - make write method mandatory

Message ID YFgUxG/TljMuVeQ3@google.com (mailing list archive)
State Accepted
Commit 81c7c0a350bfe9306ad9fb10356534ede8f4ab31
Headers show
Series Input: serio - make write method mandatory | expand

Commit Message

Dmitry Torokhov March 22, 2021, 3:53 a.m. UTC
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(-)

Comments

Nathan Chancellor July 21, 2021, 1:55 a.m. UTC | #1
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,
Michael Kelley (LINUX) July 21, 2021, 3:19 a.m. UTC | #2
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
Dmitry Torokhov July 21, 2021, 4:11 a.m. UTC | #3
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 mbox series

Patch

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)