From: Stephen Chandler Paul <[email protected]>
A big problem with the current i8042 debugging option is that it outputs
data going to and from the keyboard by default. As a result, many dmesg
logs uploaded by users will unintentionally contain sensitive information
such as their password, as such it's probably a good idea not to output
data coming from the keyboard unless specifically enabled by the user.
Signed-off-by: Stephen Chandler Paul <[email protected]>
Reviewed-by: Benjamin Tissoires <[email protected]>
---
Documentation/kernel-parameters.txt | 7 +++++++
drivers/input/serio/i8042.c | 25 +++++++++++++++++++++----
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ae44749..9e00234 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1304,6 +1304,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
<bus_id>,<clkrate>
i8042.debug [HW] Toggle i8042 debug mode
+ i8042.debug_kbd [HW] Enable printing of interrupt data from the KBD port
+ As a side effect, this option will mask some of the
+ interrupts sent back from the keyboard during the
+ initialization of the KBD port on the i8042, if you
+ need to see this, you will need to enable this
+ option.
+ (disabled by default)
i8042.direct [HW] Put keyboard port into non-translated mode
i8042.dumbkbd [HW] Pretend that controller can only read data from
keyboard and cannot control its state
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index cb5ece7..084fc05 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -88,6 +88,23 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
static bool i8042_debug;
module_param_named(debug, i8042_debug, bool, 0600);
MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
+
+static bool i8042_debug_kbd;
+module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600);
+MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or off");
+
+#define str_dbg(str, data, format, args...) \
+ do { \
+ if (!i8042_debug) \
+ break; \
+ \
+ if (str & I8042_STR_AUXDATA || i8042_debug_kbd) \
+ dbg("%02x " format, data, ##args); \
+ else \
+ dbg("** " format, ##args); \
+ } while (0)
+#else
+#define str_dbg(str, data, format, args...) do { } while (0)
#endif
static bool i8042_bypass_aux_irq_test;
@@ -528,10 +545,10 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
port = &i8042_ports[port_no];
serio = port->exists ? port->serio : NULL;
- dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n",
- data, port_no, irq,
- dfl & SERIO_PARITY ? ", bad parity" : "",
- dfl & SERIO_TIMEOUT ? ", timeout" : "");
+ str_dbg(str, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
+ port_no, irq,
+ dfl & SERIO_PARITY ? ", bad parity" : "",
+ dfl & SERIO_TIMEOUT ? ", timeout" : "");
filtered = i8042_filter(data, str, serio);
--
2.4.3
On Thu, 2015-06-25 at 15:25 -0400, [email protected] wrote:
> From: Stephen Chandler Paul <[email protected]>
>
> A big problem with the current i8042 debugging option is that it outputs
> data going to and from the keyboard by default. As a result, many dmesg
> logs uploaded by users will unintentionally contain sensitive information
> such as their password, as such it's probably a good idea not to output
> data coming from the keyboard unless specifically enabled by the user.
Seems sensible. Coupla trivial bits:
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
[]
> @@ -88,6 +88,23 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
> static bool i8042_debug;
> module_param_named(debug, i8042_debug, bool, 0600);
> MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
> +
> +static bool i8042_debug_kbd;
> +module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600);
> +MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or off");
It's unclear this depends on i8042_debug.
Maybe add something like " (enabling requires i8042_debug=1)"
> +#define str_dbg(str, data, format, args...) \
> + do { \
> + if (!i8042_debug) \
> + break; \
> + \
> + if (str & I8042_STR_AUXDATA || i8042_debug_kbd) \
> + dbg("%02x " format, data, ##args); \
> + else \
> + dbg("** " format, ##args); \
> + } while (0)
> +#else
> +#define str_dbg(str, data, format, args...) do { } while (0)
> #endif
This should probably go into i8042.h
where the dbg macro is #defined
On Thu, Jun 25, 2015 at 03:25:10PM -0400, [email protected] wrote:
> From: Stephen Chandler Paul <[email protected]>
>
> A big problem with the current i8042 debugging option is that it outputs
> data going to and from the keyboard by default. As a result, many dmesg
> logs uploaded by users will unintentionally contain sensitive information
> such as their password, as such it's probably a good idea not to output
> data coming from the keyboard unless specifically enabled by the user.
>
> Signed-off-by: Stephen Chandler Paul <[email protected]>
> Reviewed-by: Benjamin Tissoires <[email protected]>
> ---
> Documentation/kernel-parameters.txt | 7 +++++++
> drivers/input/serio/i8042.c | 25 +++++++++++++++++++++----
> 2 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index ae44749..9e00234 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1304,6 +1304,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> <bus_id>,<clkrate>
>
> i8042.debug [HW] Toggle i8042 debug mode
> + i8042.debug_kbd [HW] Enable printing of interrupt data from the KBD port
> + As a side effect, this option will mask some of the
> + interrupts sent back from the keyboard during the
> + initialization of the KBD port on the i8042, if you
> + need to see this, you will need to enable this
> + option.
Hmm, can we maybe use the bus notifier and react to
BUS_NOTIFY_BOUND_DRIVER/BUS_NOTIFY_UNBIND_DRIVER to decide if we want to
see keyboard data stream?
Thanks.
--
Dmitry
On Thu, 2015-06-25 at 13:32 -0700, Dmitry Torokhov wrote:
> On Thu, Jun 25, 2015 at 03:25:10PM -0400, [email protected] wrote:
> > From: Stephen Chandler Paul <[email protected]>
> >
> > A big problem with the current i8042 debugging option is that it
> > outputs
> > data going to and from the keyboard by default. As a result, many
> > dmesg
> > logs uploaded by users will unintentionally contain sensitive
> > information
> > such as their password, as such it's probably a good idea not to
> > output
> > data coming from the keyboard unless specifically enabled by the
> > user.
> >
> > Signed-off-by: Stephen Chandler Paul <[email protected]>
> > Reviewed-by: Benjamin Tissoires <[email protected]>
> > ---
> > Documentation/kernel-parameters.txt | 7 +++++++
> > drivers/input/serio/i8042.c | 25 +++++++++++++++++++++----
> > 2 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/kernel-parameters.txt
> > b/Documentation/kernel-parameters.txt
> > index ae44749..9e00234 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -1304,6 +1304,13 @@ bytes respectively. Such letter suffixes can
> > also be entirely omitted.
> > <bus_id>,<clkrate>
> >
> > i8042.debug [HW] Toggle i8042 debug mode
> > + i8042.debug_kbd [HW] Enable printing of interrupt data
> > from the KBD port
> > + As a side effect, this option will
> > mask some of the
> > + interrupts sent back from the
> > keyboard during the
> > + initialization of the KBD port on the
> > i8042, if you
> > + need to see this, you will need to
> > enable this
> > + option.
>
> Hmm, can we maybe use the bus notifier and react to
> BUS_NOTIFY_BOUND_DRIVER/BUS_NOTIFY_UNBIND_DRIVER to decide if we want
> to
> see keyboard data stream?
Out of curiosity, are there devices that aren't keyboards that actually
make use of the KBD port? It was my understanding keyboards used the
KBD port, and everything else uses the AUX port.
Regardless, I'm looking into doing this as we speak.
Cheers,
Stephen Chandler Paul
>
> Thanks.
>
On Thu, Jun 25, 2015 at 05:31:25PM -0400, Stephen Chandler Paul wrote:
> On Thu, 2015-06-25 at 13:32 -0700, Dmitry Torokhov wrote:
> > On Thu, Jun 25, 2015 at 03:25:10PM -0400, [email protected] wrote:
> > > From: Stephen Chandler Paul <[email protected]>
> > >
> > > A big problem with the current i8042 debugging option is that it
> > > outputs
> > > data going to and from the keyboard by default. As a result, many
> > > dmesg
> > > logs uploaded by users will unintentionally contain sensitive
> > > information
> > > such as their password, as such it's probably a good idea not to
> > > output
> > > data coming from the keyboard unless specifically enabled by the
> > > user.
> > >
> > > Signed-off-by: Stephen Chandler Paul <[email protected]>
> > > Reviewed-by: Benjamin Tissoires <[email protected]>
> > > ---
> > > Documentation/kernel-parameters.txt | 7 +++++++
> > > drivers/input/serio/i8042.c | 25 +++++++++++++++++++++----
> > > 2 files changed, 28 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/kernel-parameters.txt
> > > b/Documentation/kernel-parameters.txt
> > > index ae44749..9e00234 100644
> > > --- a/Documentation/kernel-parameters.txt
> > > +++ b/Documentation/kernel-parameters.txt
> > > @@ -1304,6 +1304,13 @@ bytes respectively. Such letter suffixes can
> > > also be entirely omitted.
> > > <bus_id>,<clkrate>
> > >
> > > i8042.debug [HW] Toggle i8042 debug mode
> > > + i8042.debug_kbd [HW] Enable printing of interrupt data
> > > from the KBD port
> > > + As a side effect, this option will
> > > mask some of the
> > > + interrupts sent back from the
> > > keyboard during the
> > > + initialization of the KBD port on the
> > > i8042, if you
> > > + need to see this, you will need to
> > > enable this
> > > + option.
> >
> > Hmm, can we maybe use the bus notifier and react to
> > BUS_NOTIFY_BOUND_DRIVER/BUS_NOTIFY_UNBIND_DRIVER to decide if we want
> > to
> > see keyboard data stream?
> Out of curiosity, are there devices that aren't keyboards that actually
> make use of the KBD port? It was my understanding keyboards used the
> KBD port, and everything else uses the AUX port.
On desktops if you plug keyboard into mouse port and mouse into keyboard
port there is a chance they will work (depends on the BIOS). But I was
not talking about supporting that necessarily, but use bus notifiers to
allow seeing KBC port stream until the driver is fully bound and when it
is to be unbound from the KBC port (whatever driver that might be).
Thanks.
--
Dmitry
On Thu, 2015-06-25 at 14:35 -0700, Dmitry Torokhov wrote:
> On Thu, Jun 25, 2015 at 05:31:25PM -0400, Stephen Chandler Paul
> wrote:
> > On Thu, 2015-06-25 at 13:32 -0700, Dmitry Torokhov wrote:
> > > On Thu, Jun 25, 2015 at 03:25:10PM -0400, [email protected] wrote:
> > > > From: Stephen Chandler Paul <[email protected]>
> > > >
> > > > A big problem with the current i8042 debugging option is that
> > > > it
> > > > outputs
> > > > data going to and from the keyboard by default. As a result,
> > > > many
> > > > dmesg
> > > > logs uploaded by users will unintentionally contain sensitive
> > > > information
> > > > such as their password, as such it's probably a good idea not
> > > > to
> > > > output
> > > > data coming from the keyboard unless specifically enabled by
> > > > the
> > > > user.
> > > >
> > > > Signed-off-by: Stephen Chandler Paul <[email protected]>
> > > > Reviewed-by: Benjamin Tissoires <[email protected]>
> > > > ---
> > > > Documentation/kernel-parameters.txt | 7 +++++++
> > > > drivers/input/serio/i8042.c | 25 +++++++++++++++++++++
> > > > ----
> > > > 2 files changed, 28 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/kernel-parameters.txt
> > > > b/Documentation/kernel-parameters.txt
> > > > index ae44749..9e00234 100644
> > > > --- a/Documentation/kernel-parameters.txt
> > > > +++ b/Documentation/kernel-parameters.txt
> > > > @@ -1304,6 +1304,13 @@ bytes respectively. Such letter suffixes
> > > > can
> > > > also be entirely omitted.
> > > > <bus_id>,<clkrate>
> > > >
> > > > i8042.debug [HW] Toggle i8042 debug mode
> > > > + i8042.debug_kbd [HW] Enable printing of interrupt data
> > > >
> > > > from the KBD port
> > > > + As a side effect, this option
> > > > will
> > > > mask some of the
> > > > + interrupts sent back from the
> > > > keyboard during the
> > > > + initialization of the KBD port on
> > > > the
> > > > i8042, if you
> > > > + need to see this, you will need
> > > > to
> > > > enable this
> > > > + option.
> > >
> > > Hmm, can we maybe use the bus notifier and react to
> > > BUS_NOTIFY_BOUND_DRIVER/BUS_NOTIFY_UNBIND_DRIVER to decide if we
> > > want
> > > to
> > > see keyboard data stream?
> > Out of curiosity, are there devices that aren't keyboards that
> > actually
> > make use of the KBD port? It was my understanding keyboards used
> > the
> > KBD port, and everything else uses the AUX port.
>
> On desktops if you plug keyboard into mouse port and mouse into
> keyboard
> port there is a chance they will work (depends on the BIOS). But I
> was
> not talking about supporting that necessarily, but use bus notifiers
> to
> allow seeing KBC port stream until the driver is fully bound and when
> it
> is to be unbound from the KBC port (whatever driver that might be).
Alright, I'm following you now. This is definitely doable, we don't
need it for our use case (this is mostly just to stop people from
accidentally giving their passwords to us), but I'll be happy to add it
and get back to you with the new version of the patch when I'm
finished.
Cheers,
Stephen Chandler Paul
>
> Thanks.
>
From: Stephen Chandler Paul <[email protected]>
A big problem with the current i8042 debugging option is that it outputs
data going to and from the keyboard by default. As a result, many dmesg
logs uploaded by users will unintentionally contain sensitive information
such as their password, as such it's probably a good idea not to output
data coming from the keyboard unless specifically enabled by the user.
Signed-off-by: Stephen Chandler Paul <[email protected]>
---
Changes
* Moved declaration of str_dbg() (now filter_dbg()) to i8042.h with the
rest of the debugging functions
* Make note of the dependency on i8042.debug=1 in the documentation and in the
kernel option description
* Use a bus notifier so that we only filter interrupts coming from the
keyboard once an input driver has been bound to a device on the port
Documentation/kernel-parameters.txt | 3 ++
drivers/input/serio/i8042.c | 60 +++++++++++++++++++++++++++++++++----
drivers/input/serio/i8042.h | 13 ++++++++
3 files changed, 70 insertions(+), 6 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ae44749..a9d2b19 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1304,6 +1304,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
<bus_id>,<clkrate>
i8042.debug [HW] Toggle i8042 debug mode
+ i8042.debug_kbd [HW] Enable printing of interrupt data from the KBD port
+ (disabled by default, requires that i8042.debug=1
+ be enabled)
i8042.direct [HW] Put keyboard port into non-translated mode
i8042.dumbkbd [HW] Pretend that controller can only read data from
keyboard and cannot control its state
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index cb5ece7..97e5c93 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -88,6 +88,10 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
static bool i8042_debug;
module_param_named(debug, i8042_debug, bool, 0600);
MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
+
+static bool i8042_debug_kbd;
+module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600);
+MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or off (requires i8042.debug=1)");
#endif
static bool i8042_bypass_aux_irq_test;
@@ -116,6 +120,9 @@ struct i8042_port {
struct serio *serio;
int irq;
bool exists;
+#ifdef DEBUG
+ bool filter_dbg;
+#endif
signed char mux;
};
@@ -133,6 +140,9 @@ static bool i8042_kbd_irq_registered;
static bool i8042_aux_irq_registered;
static unsigned char i8042_suppress_kbd_ack;
static struct platform_device *i8042_platform_device;
+#ifdef DEBUG
+static struct notifier_block i8042_kbd_bind_notifier_block;
+#endif
static irqreturn_t i8042_interrupt(int irq, void *dev_id);
static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
@@ -528,10 +538,10 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
port = &i8042_ports[port_no];
serio = port->exists ? port->serio : NULL;
- dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n",
- data, port_no, irq,
- dfl & SERIO_PARITY ? ", bad parity" : "",
- dfl & SERIO_TIMEOUT ? ", timeout" : "");
+ filter_dbg(port->filter_dbg, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
+ port_no, irq,
+ dfl & SERIO_PARITY ? ", bad parity" : "",
+ dfl & SERIO_TIMEOUT ? ", timeout" : "");
filtered = i8042_filter(data, str, serio);
@@ -1329,6 +1339,13 @@ static void __init i8042_register_ports(void)
i8042_ports[i].irq);
serio_register_port(serio);
device_set_wakeup_capable(&serio->dev, true);
+#ifdef DEBUG
+ if (i8042_ports[i].irq == I8042_KBD_IRQ) {
+ bus_register_notifier(
+ serio->dev.bus,
+ &i8042_kbd_bind_notifier_block);
+ }
+#endif
}
}
}
@@ -1338,8 +1355,17 @@ static void i8042_unregister_ports(void)
int i;
for (i = 0; i < I8042_NUM_PORTS; i++) {
- if (i8042_ports[i].serio) {
- serio_unregister_port(i8042_ports[i].serio);
+ struct serio *serio = i8042_ports[i].serio;
+
+ if (serio) {
+#ifdef DEBUG
+ if (i8042_ports[i].irq == I8042_KBD_IRQ) {
+ bus_unregister_notifier(
+ serio->dev.bus,
+ &i8042_kbd_bind_notifier_block);
+ }
+#endif
+ serio_unregister_port(serio);
i8042_ports[i].serio = NULL;
}
}
@@ -1438,6 +1464,22 @@ static int __init i8042_setup_kbd(void)
return error;
}
+#ifdef DEBUG
+static int i8042_kbd_bind_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ struct i8042_port *port = to_serio_port(dev)->port_data;
+
+ if (action == BUS_NOTIFY_BOUND_DRIVER)
+ port->filter_dbg = true;
+ else if (action == BUS_NOTIFY_UNBOUND_DRIVER)
+ port->filter_dbg = false;
+
+ return 0;
+}
+#endif
+
static int __init i8042_probe(struct platform_device *dev)
{
int error;
@@ -1507,6 +1549,12 @@ static struct platform_driver i8042_driver = {
.shutdown = i8042_shutdown,
};
+#ifdef DEBUG
+static struct notifier_block i8042_kbd_bind_notifier_block = {
+ .notifier_call = i8042_kbd_bind_notifier,
+};
+#endif
+
static int __init i8042_init(void)
{
struct platform_device *pdev;
diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
index fc080be..a198f0d 100644
--- a/drivers/input/serio/i8042.h
+++ b/drivers/input/serio/i8042.h
@@ -73,6 +73,17 @@ static unsigned long i8042_start_time;
printk(KERN_DEBUG KBUILD_MODNAME ": [%d] " format, \
(int) (jiffies - i8042_start_time), ##arg); \
} while (0)
+
+#define filter_dbg(filter, data, format, args...) \
+ do { \
+ if (!i8042_debug) \
+ break; \
+ \
+ if (!filter || i8042_debug_kbd) \
+ dbg("%02x " format, data, ##args); \
+ else \
+ dbg("** " format, ##args); \
+ } while (0)
#else
#define dbg_init() do { } while (0)
#define dbg(format, arg...) \
@@ -80,6 +91,8 @@ static unsigned long i8042_start_time;
if (0) \
printk(KERN_DEBUG pr_fmt(format), ##arg); \
} while (0)
+
+#define filter_dbg(filter, data, format, args...) do { } while (0)
#endif
#endif /* _I8042_H */
--
2.4.3
From: Stephen Chandler Paul <[email protected]>
A big problem with the current i8042 debugging option is that it outputs
data going to and from the keyboard by default. As a result, many dmesg
logs uploaded by users will unintentionally contain sensitive information
such as their password, as such it's probably a good idea not to output
data coming from the keyboard unless specifically enabled by the user.
Signed-off-by: Stephen Chandler Paul <[email protected]>
---
That patch was not working at all like I thought it was, the results on the
machine I tested it on were very misleading so I apologize for any trouble that
may have caused! I've done more thorough testing of this, and it should work
perfectly on any machine.
Changes
* Bus structs are shared between devices, whoops. Now when the notifier goes
off we check two things:
* That the device actually belongs to the i8042 platform driver. This is
something that's difficult to reproduce, since devices on the i8042 bus
are usually handled by the plaform driver. If they don't happen to be
however (which currently I've only seen with ps2emu, a kernel module I'm
working on to replay PS/2 devices in the kernel), then we make the false
assumption the port_data variable points to an i8042_port struct, and
very likely crash the machine or worse.
* That the IRQ of the port is equivalent to that of the KBD port, so that
we only mask the interrupts coming from the keyboard.
Documentation/kernel-parameters.txt | 3 ++
drivers/input/serio/i8042.c | 64 +++++++++++++++++++++++++++++++++----
drivers/input/serio/i8042.h | 13 ++++++++
3 files changed, 74 insertions(+), 6 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ae44749..a9d2b19 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1304,6 +1304,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
<bus_id>,<clkrate>
i8042.debug [HW] Toggle i8042 debug mode
+ i8042.debug_kbd [HW] Enable printing of interrupt data from the KBD port
+ (disabled by default, requires that i8042.debug=1
+ be enabled)
i8042.direct [HW] Put keyboard port into non-translated mode
i8042.dumbkbd [HW] Pretend that controller can only read data from
keyboard and cannot control its state
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index cb5ece7..0e17bdd 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -88,6 +88,10 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
static bool i8042_debug;
module_param_named(debug, i8042_debug, bool, 0600);
MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
+
+static bool i8042_debug_kbd;
+module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600);
+MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or off (requires i8042.debug=1)");
#endif
static bool i8042_bypass_aux_irq_test;
@@ -116,6 +120,9 @@ struct i8042_port {
struct serio *serio;
int irq;
bool exists;
+#ifdef DEBUG
+ bool filter_dbg;
+#endif
signed char mux;
};
@@ -133,6 +140,9 @@ static bool i8042_kbd_irq_registered;
static bool i8042_aux_irq_registered;
static unsigned char i8042_suppress_kbd_ack;
static struct platform_device *i8042_platform_device;
+#ifdef DEBUG
+static struct notifier_block i8042_kbd_bind_notifier_block;
+#endif
static irqreturn_t i8042_interrupt(int irq, void *dev_id);
static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
@@ -528,10 +538,10 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
port = &i8042_ports[port_no];
serio = port->exists ? port->serio : NULL;
- dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n",
- data, port_no, irq,
- dfl & SERIO_PARITY ? ", bad parity" : "",
- dfl & SERIO_TIMEOUT ? ", timeout" : "");
+ filter_dbg(port->filter_dbg, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
+ port_no, irq,
+ dfl & SERIO_PARITY ? ", bad parity" : "",
+ dfl & SERIO_TIMEOUT ? ", timeout" : "");
filtered = i8042_filter(data, str, serio);
@@ -1329,6 +1339,13 @@ static void __init i8042_register_ports(void)
i8042_ports[i].irq);
serio_register_port(serio);
device_set_wakeup_capable(&serio->dev, true);
+#ifdef DEBUG
+ if (i == I8042_KBD_PORT_NO) {
+ bus_register_notifier(
+ serio->dev.bus,
+ &i8042_kbd_bind_notifier_block);
+ }
+#endif
}
}
}
@@ -1338,8 +1355,17 @@ static void i8042_unregister_ports(void)
int i;
for (i = 0; i < I8042_NUM_PORTS; i++) {
- if (i8042_ports[i].serio) {
- serio_unregister_port(i8042_ports[i].serio);
+ struct serio *serio = i8042_ports[i].serio;
+
+ if (serio) {
+#ifdef DEBUG
+ if (i == I8042_KBD_PORT_NO) {
+ bus_unregister_notifier(
+ serio->dev.bus,
+ &i8042_kbd_bind_notifier_block);
+ }
+#endif
+ serio_unregister_port(serio);
i8042_ports[i].serio = NULL;
}
}
@@ -1438,6 +1464,26 @@ static int __init i8042_setup_kbd(void)
return error;
}
+#ifdef DEBUG
+static int i8042_kbd_bind_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ struct i8042_port *port = to_serio_port(dev)->port_data;
+
+ if (dev->parent != &i8042_platform_device->dev ||
+ port->irq != I8042_KBD_IRQ)
+ return 0;
+
+ if (action == BUS_NOTIFY_BOUND_DRIVER)
+ port->filter_dbg = true;
+ else if (action == BUS_NOTIFY_UNBOUND_DRIVER)
+ port->filter_dbg = false;
+
+ return 0;
+}
+#endif
+
static int __init i8042_probe(struct platform_device *dev)
{
int error;
@@ -1507,6 +1553,12 @@ static struct platform_driver i8042_driver = {
.shutdown = i8042_shutdown,
};
+#ifdef DEBUG
+static struct notifier_block i8042_kbd_bind_notifier_block = {
+ .notifier_call = i8042_kbd_bind_notifier,
+};
+#endif
+
static int __init i8042_init(void)
{
struct platform_device *pdev;
diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
index fc080be..a198f0d 100644
--- a/drivers/input/serio/i8042.h
+++ b/drivers/input/serio/i8042.h
@@ -73,6 +73,17 @@ static unsigned long i8042_start_time;
printk(KERN_DEBUG KBUILD_MODNAME ": [%d] " format, \
(int) (jiffies - i8042_start_time), ##arg); \
} while (0)
+
+#define filter_dbg(filter, data, format, args...) \
+ do { \
+ if (!i8042_debug) \
+ break; \
+ \
+ if (!filter || i8042_debug_kbd) \
+ dbg("%02x " format, data, ##args); \
+ else \
+ dbg("** " format, ##args); \
+ } while (0)
#else
#define dbg_init() do { } while (0)
#define dbg(format, arg...) \
@@ -80,6 +91,8 @@ static unsigned long i8042_start_time;
if (0) \
printk(KERN_DEBUG pr_fmt(format), ##arg); \
} while (0)
+
+#define filter_dbg(filter, data, format, args...) do { } while (0)
#endif
#endif /* _I8042_H */
--
2.4.3
Hi Stephen,
On Fri, Jun 26, 2015 at 06:28:18PM -0400, [email protected] wrote:
> From: Stephen Chandler Paul <[email protected]>
>
> A big problem with the current i8042 debugging option is that it outputs
> data going to and from the keyboard by default. As a result, many dmesg
> logs uploaded by users will unintentionally contain sensitive information
> such as their password, as such it's probably a good idea not to output
> data coming from the keyboard unless specifically enabled by the user.
>
> Signed-off-by: Stephen Chandler Paul <[email protected]>
> ---
> That patch was not working at all like I thought it was, the results on the
> machine I tested it on were very misleading so I apologize for any trouble that
> may have caused! I've done more thorough testing of this, and it should work
> perfectly on any machine.
>
> Changes
> * Bus structs are shared between devices, whoops. Now when the notifier goes
> off we check two things:
> * That the device actually belongs to the i8042 platform driver. This is
> something that's difficult to reproduce, since devices on the i8042 bus
> are usually handled by the plaform driver. If they don't happen to be
> however (which currently I've only seen with ps2emu, a kernel module I'm
> working on to replay PS/2 devices in the kernel), then we make the false
> assumption the port_data variable points to an i8042_port struct, and
> very likely crash the machine or worse.
> * That the IRQ of the port is equivalent to that of the KBD port, so that
> we only mask the interrupts coming from the keyboard.
>
> Documentation/kernel-parameters.txt | 3 ++
> drivers/input/serio/i8042.c | 64 +++++++++++++++++++++++++++++++++----
> drivers/input/serio/i8042.h | 13 ++++++++
> 3 files changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index ae44749..a9d2b19 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1304,6 +1304,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> <bus_id>,<clkrate>
>
> i8042.debug [HW] Toggle i8042 debug mode
> + i8042.debug_kbd [HW] Enable printing of interrupt data from the KBD port
> + (disabled by default, requires that i8042.debug=1
> + be enabled)
> i8042.direct [HW] Put keyboard port into non-translated mode
> i8042.dumbkbd [HW] Pretend that controller can only read data from
> keyboard and cannot control its state
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index cb5ece7..0e17bdd 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -88,6 +88,10 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
> static bool i8042_debug;
> module_param_named(debug, i8042_debug, bool, 0600);
> MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
> +
> +static bool i8042_debug_kbd;
> +module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600);
> +MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or off (requires i8042.debug=1)");
> #endif
>
> static bool i8042_bypass_aux_irq_test;
> @@ -116,6 +120,9 @@ struct i8042_port {
> struct serio *serio;
> int irq;
> bool exists;
> +#ifdef DEBUG
> + bool filter_dbg;
> +#endif
Could we call it "driver_bound" and drop #ifdef DEBUG? In fact, I'd
rager we dropped the #ifdef DEBUG arounf all bus notifier code.
> signed char mux;
> };
>
> @@ -133,6 +140,9 @@ static bool i8042_kbd_irq_registered;
> static bool i8042_aux_irq_registered;
> static unsigned char i8042_suppress_kbd_ack;
> static struct platform_device *i8042_platform_device;
> +#ifdef DEBUG
> +static struct notifier_block i8042_kbd_bind_notifier_block;
> +#endif
>
> static irqreturn_t i8042_interrupt(int irq, void *dev_id);
> static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
> @@ -528,10 +538,10 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
> port = &i8042_ports[port_no];
> serio = port->exists ? port->serio : NULL;
>
> - dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n",
> - data, port_no, irq,
> - dfl & SERIO_PARITY ? ", bad parity" : "",
> - dfl & SERIO_TIMEOUT ? ", timeout" : "");
> + filter_dbg(port->filter_dbg, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
> + port_no, irq,
> + dfl & SERIO_PARITY ? ", bad parity" : "",
> + dfl & SERIO_TIMEOUT ? ", timeout" : "");
>
> filtered = i8042_filter(data, str, serio);
>
> @@ -1329,6 +1339,13 @@ static void __init i8042_register_ports(void)
> i8042_ports[i].irq);
> serio_register_port(serio);
> device_set_wakeup_capable(&serio->dev, true);
> +#ifdef DEBUG
> + if (i == I8042_KBD_PORT_NO) {
> + bus_register_notifier(
> + serio->dev.bus,
> + &i8042_kbd_bind_notifier_block);
> + }
Umm, let's export serio_bus and register the notifier when we load the
module.
> +#endif
> }
> }
> }
> @@ -1338,8 +1355,17 @@ static void i8042_unregister_ports(void)
> int i;
>
> for (i = 0; i < I8042_NUM_PORTS; i++) {
> - if (i8042_ports[i].serio) {
> - serio_unregister_port(i8042_ports[i].serio);
> + struct serio *serio = i8042_ports[i].serio;
> +
> + if (serio) {
> +#ifdef DEBUG
> + if (i == I8042_KBD_PORT_NO) {
> + bus_unregister_notifier(
> + serio->dev.bus,
> + &i8042_kbd_bind_notifier_block);
> + }
> +#endif
> + serio_unregister_port(serio);
> i8042_ports[i].serio = NULL;
> }
> }
> @@ -1438,6 +1464,26 @@ static int __init i8042_setup_kbd(void)
> return error;
> }
>
> +#ifdef DEBUG
> +static int i8042_kbd_bind_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct device *dev = data;
> + struct i8042_port *port = to_serio_port(dev)->port_data;
> +
> + if (dev->parent != &i8042_platform_device->dev ||
> + port->irq != I8042_KBD_IRQ)
> + return 0;
Why don't you compare serio with i8042_ports[I8042_KBD_PORT_NO].serio
instead?
> +
> + if (action == BUS_NOTIFY_BOUND_DRIVER)
> + port->filter_dbg = true;
> + else if (action == BUS_NOTIFY_UNBOUND_DRIVER)
> + port->filter_dbg = false;
switch (action) {
}
please.
> +
> + return 0;
> +}
> +#endif
> +
> static int __init i8042_probe(struct platform_device *dev)
> {
> int error;
> @@ -1507,6 +1553,12 @@ static struct platform_driver i8042_driver = {
> .shutdown = i8042_shutdown,
> };
>
> +#ifdef DEBUG
> +static struct notifier_block i8042_kbd_bind_notifier_block = {
> + .notifier_call = i8042_kbd_bind_notifier,
> +};
> +#endif
> +
> static int __init i8042_init(void)
> {
> struct platform_device *pdev;
> diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
> index fc080be..a198f0d 100644
> --- a/drivers/input/serio/i8042.h
> +++ b/drivers/input/serio/i8042.h
> @@ -73,6 +73,17 @@ static unsigned long i8042_start_time;
> printk(KERN_DEBUG KBUILD_MODNAME ": [%d] " format, \
> (int) (jiffies - i8042_start_time), ##arg); \
> } while (0)
> +
> +#define filter_dbg(filter, data, format, args...) \
> + do { \
> + if (!i8042_debug) \
> + break; \
> + \
> + if (!filter || i8042_debug_kbd) \
> + dbg("%02x " format, data, ##args); \
> + else \
> + dbg("** " format, ##args); \
> + } while (0)
> #else
> #define dbg_init() do { } while (0)
> #define dbg(format, arg...) \
> @@ -80,6 +91,8 @@ static unsigned long i8042_start_time;
> if (0) \
> printk(KERN_DEBUG pr_fmt(format), ##arg); \
> } while (0)
> +
> +#define filter_dbg(filter, data, format, args...) do { } while (0)
> #endif
>
> #endif /* _I8042_H */
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks.
--
Dmitry
On Fri, 2015-06-26 at 16:05 -0700, Dmitry Torokhov wrote:
> Hi Stephen,
>
> On Fri, Jun 26, 2015 at 06:28:18PM -0400, [email protected] wrote:
> > From: Stephen Chandler Paul <[email protected]>
> >
> > A big problem with the current i8042 debugging option is that it
> > outputs
> > data going to and from the keyboard by default. As a result, many
> > dmesg
> > logs uploaded by users will unintentionally contain sensitive
> > information
> > such as their password, as such it's probably a good idea not to
> > output
> > data coming from the keyboard unless specifically enabled by the
> > user.
> >
> > Signed-off-by: Stephen Chandler Paul <[email protected]>
> > ---
> > That patch was not working at all like I thought it was, the
> > results on the
> > machine I tested it on were very misleading so I apologize for any
> > trouble that
> > may have caused! I've done more thorough testing of this, and it
> > should work
> > perfectly on any machine.
> >
> > Changes
> > * Bus structs are shared between devices, whoops. Now when the
> > notifier goes
> > off we check two things:
> > * That the device actually belongs to the i8042 platform
> > driver. This is
> > something that's difficult to reproduce, since devices on the
> > i8042 bus
> > are usually handled by the plaform driver. If they don't
> > happen to be
> > however (which currently I've only seen with ps2emu, a kernel
> > module I'm
> > working on to replay PS/2 devices in the kernel), then we
> > make the false
> > assumption the port_data variable points to an i8042_port
> > struct, and
> > very likely crash the machine or worse.
> > * That the IRQ of the port is equivalent to that of the KBD
> > port, so that
> > we only mask the interrupts coming from the keyboard.
> >
> > Documentation/kernel-parameters.txt | 3 ++
> > drivers/input/serio/i8042.c | 64
> > +++++++++++++++++++++++++++++++++----
> > drivers/input/serio/i8042.h | 13 ++++++++
> > 3 files changed, 74 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/kernel-parameters.txt
> > b/Documentation/kernel-parameters.txt
> > index ae44749..a9d2b19 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -1304,6 +1304,9 @@ bytes respectively. Such letter suffixes can
> > also be entirely omitted.
> > <bus_id>,<clkrate>
> >
> > i8042.debug [HW] Toggle i8042 debug mode
> > + i8042.debug_kbd [HW] Enable printing of interrupt data
> > from the KBD port
> > + (disabled by default, requires that
> > i8042.debug=1
> > + be enabled)
> > i8042.direct [HW] Put keyboard port into non-translated
> > mode
> > i8042.dumbkbd [HW] Pretend that controller can only read
> > data from
> > keyboard and cannot control its state
> > diff --git a/drivers/input/serio/i8042.c
> > b/drivers/input/serio/i8042.c
> > index cb5ece7..0e17bdd 100644
> > --- a/drivers/input/serio/i8042.c
> > +++ b/drivers/input/serio/i8042.c
> > @@ -88,6 +88,10 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to
> > detect controller settings");
> > static bool i8042_debug;
> > module_param_named(debug, i8042_debug, bool, 0600);
> > MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
> > +
> > +static bool i8042_debug_kbd;
> > +module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600);
> > +MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or
> > off (requires i8042.debug=1)");
> > #endif
> >
> > static bool i8042_bypass_aux_irq_test;
> > @@ -116,6 +120,9 @@ struct i8042_port {
> > struct serio *serio;
> > int irq;
> > bool exists;
> > +#ifdef DEBUG
> > + bool filter_dbg;
> > +#endif
>
> Could we call it "driver_bound" and drop #ifdef DEBUG? In fact, I'd
> rager we dropped the #ifdef DEBUG arounf all bus notifier code.
>
> > signed char mux;
> > };
> >
> > @@ -133,6 +140,9 @@ static bool i8042_kbd_irq_registered;
> > static bool i8042_aux_irq_registered;
> > static unsigned char i8042_suppress_kbd_ack;
> > static struct platform_device *i8042_platform_device;
> > +#ifdef DEBUG
> > +static struct notifier_block i8042_kbd_bind_notifier_block;
> > +#endif
> >
> > static irqreturn_t i8042_interrupt(int irq, void *dev_id);
> > static bool (*i8042_platform_filter)(unsigned char data, unsigned
> > char str,
> > @@ -528,10 +538,10 @@ static irqreturn_t i8042_interrupt(int irq,
> > void *dev_id)
> > port = &i8042_ports[port_no];
> > serio = port->exists ? port->serio : NULL;
> >
> > - dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n",
> > - data, port_no, irq,
> > - dfl & SERIO_PARITY ? ", bad parity" : "",
> > - dfl & SERIO_TIMEOUT ? ", timeout" : "");
> > + filter_dbg(port->filter_dbg, data, "<- i8042 (interrupt,
> > %d, %d%s%s)\n",
> > + port_no, irq,
> > + dfl & SERIO_PARITY ? ", bad parity" : "",
> > + dfl & SERIO_TIMEOUT ? ", timeout" : "");
> >
> > filtered = i8042_filter(data, str, serio);
> >
> > @@ -1329,6 +1339,13 @@ static void __init
> > i8042_register_ports(void)
> > i8042_ports[i].irq);
> > serio_register_port(serio);
> > device_set_wakeup_capable(&serio->dev,
> > true);
> > +#ifdef DEBUG
> > + if (i == I8042_KBD_PORT_NO) {
> > + bus_register_notifier(
> > + serio->dev.bus,
> > + &i8042_kbd_bind_notifier_b
> > lock);
> > + }
>
>
> Umm, let's export serio_bus and register the notifier when we load
> the
> module.
>
Sorry, but would you mind clarifying what you mean by "export
serio_bus"?
Cheers,
Stephen Chandler Paul
On Fri, Jun 26, 2015 at 07:21:47PM -0400, Stephen Chandler Paul wrote:
> On Fri, 2015-06-26 at 16:05 -0700, Dmitry Torokhov wrote:
> > Hi Stephen,
> >
> > On Fri, Jun 26, 2015 at 06:28:18PM -0400, [email protected] wrote:
> > > From: Stephen Chandler Paul <[email protected]>
> > >
> > > A big problem with the current i8042 debugging option is that it
> > > outputs
> > > data going to and from the keyboard by default. As a result, many
> > > dmesg
> > > logs uploaded by users will unintentionally contain sensitive
> > > information
> > > such as their password, as such it's probably a good idea not to
> > > output
> > > data coming from the keyboard unless specifically enabled by the
> > > user.
> > >
> > > Signed-off-by: Stephen Chandler Paul <[email protected]>
> > > ---
> > > That patch was not working at all like I thought it was, the
> > > results on the
> > > machine I tested it on were very misleading so I apologize for any
> > > trouble that
> > > may have caused! I've done more thorough testing of this, and it
> > > should work
> > > perfectly on any machine.
> > >
> > > Changes
> > > * Bus structs are shared between devices, whoops. Now when the
> > > notifier goes
> > > off we check two things:
> > > * That the device actually belongs to the i8042 platform
> > > driver. This is
> > > something that's difficult to reproduce, since devices on the
> > > i8042 bus
> > > are usually handled by the plaform driver. If they don't
> > > happen to be
> > > however (which currently I've only seen with ps2emu, a kernel
> > > module I'm
> > > working on to replay PS/2 devices in the kernel), then we
> > > make the false
> > > assumption the port_data variable points to an i8042_port
> > > struct, and
> > > very likely crash the machine or worse.
> > > * That the IRQ of the port is equivalent to that of the KBD
> > > port, so that
> > > we only mask the interrupts coming from the keyboard.
> > >
> > > Documentation/kernel-parameters.txt | 3 ++
> > > drivers/input/serio/i8042.c | 64
> > > +++++++++++++++++++++++++++++++++----
> > > drivers/input/serio/i8042.h | 13 ++++++++
> > > 3 files changed, 74 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/kernel-parameters.txt
> > > b/Documentation/kernel-parameters.txt
> > > index ae44749..a9d2b19 100644
> > > --- a/Documentation/kernel-parameters.txt
> > > +++ b/Documentation/kernel-parameters.txt
> > > @@ -1304,6 +1304,9 @@ bytes respectively. Such letter suffixes can
> > > also be entirely omitted.
> > > <bus_id>,<clkrate>
> > >
> > > i8042.debug [HW] Toggle i8042 debug mode
> > > + i8042.debug_kbd [HW] Enable printing of interrupt data
> > > from the KBD port
> > > + (disabled by default, requires that
> > > i8042.debug=1
> > > + be enabled)
> > > i8042.direct [HW] Put keyboard port into non-translated
> > > mode
> > > i8042.dumbkbd [HW] Pretend that controller can only read
> > > data from
> > > keyboard and cannot control its state
> > > diff --git a/drivers/input/serio/i8042.c
> > > b/drivers/input/serio/i8042.c
> > > index cb5ece7..0e17bdd 100644
> > > --- a/drivers/input/serio/i8042.c
> > > +++ b/drivers/input/serio/i8042.c
> > > @@ -88,6 +88,10 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to
> > > detect controller settings");
> > > static bool i8042_debug;
> > > module_param_named(debug, i8042_debug, bool, 0600);
> > > MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
> > > +
> > > +static bool i8042_debug_kbd;
> > > +module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600);
> > > +MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or
> > > off (requires i8042.debug=1)");
> > > #endif
> > >
> > > static bool i8042_bypass_aux_irq_test;
> > > @@ -116,6 +120,9 @@ struct i8042_port {
> > > struct serio *serio;
> > > int irq;
> > > bool exists;
> > > +#ifdef DEBUG
> > > + bool filter_dbg;
> > > +#endif
> >
> > Could we call it "driver_bound" and drop #ifdef DEBUG? In fact, I'd
> > rager we dropped the #ifdef DEBUG arounf all bus notifier code.
> >
> > > signed char mux;
> > > };
> > >
> > > @@ -133,6 +140,9 @@ static bool i8042_kbd_irq_registered;
> > > static bool i8042_aux_irq_registered;
> > > static unsigned char i8042_suppress_kbd_ack;
> > > static struct platform_device *i8042_platform_device;
> > > +#ifdef DEBUG
> > > +static struct notifier_block i8042_kbd_bind_notifier_block;
> > > +#endif
> > >
> > > static irqreturn_t i8042_interrupt(int irq, void *dev_id);
> > > static bool (*i8042_platform_filter)(unsigned char data, unsigned
> > > char str,
> > > @@ -528,10 +538,10 @@ static irqreturn_t i8042_interrupt(int irq,
> > > void *dev_id)
> > > port = &i8042_ports[port_no];
> > > serio = port->exists ? port->serio : NULL;
> > >
> > > - dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n",
> > > - data, port_no, irq,
> > > - dfl & SERIO_PARITY ? ", bad parity" : "",
> > > - dfl & SERIO_TIMEOUT ? ", timeout" : "");
> > > + filter_dbg(port->filter_dbg, data, "<- i8042 (interrupt,
> > > %d, %d%s%s)\n",
> > > + port_no, irq,
> > > + dfl & SERIO_PARITY ? ", bad parity" : "",
> > > + dfl & SERIO_TIMEOUT ? ", timeout" : "");
> > >
> > > filtered = i8042_filter(data, str, serio);
> > >
> > > @@ -1329,6 +1339,13 @@ static void __init
> > > i8042_register_ports(void)
> > > i8042_ports[i].irq);
> > > serio_register_port(serio);
> > > device_set_wakeup_capable(&serio->dev,
> > > true);
> > > +#ifdef DEBUG
> > > + if (i == I8042_KBD_PORT_NO) {
> > > + bus_register_notifier(
> > > + serio->dev.bus,
> > > + &i8042_kbd_bind_notifier_b
> > > lock);
> > > + }
> >
> >
> > Umm, let's export serio_bus and register the notifier when we load
> > the
> > module.
> >
>
> Sorry, but would you mind clarifying what you mean by "export
> serio_bus"?
Right now in serio.c we have:
static struct bus_type serio_bus;
and that is why you have to go by serio->dev.bus to access it. If you
export the symbol you can use it directly, before you allocate any
serio ports.
Thanks.
--
Dmitry
From: Stephen Chandler Paul <[email protected]>
A big problem with the current i8042 debugging option is that it outputs
data going to and from the keyboard by default. As a result, many dmesg
logs uploaded by users will unintentionally contain sensitive information
such as their password, as such it's probably a good idea not to output
data coming from the keyboard unless specifically enabled by the user.
Signed-off-by: Stephen Chandler Paul <[email protected]>
---
Changes
* Remove #ifdefs around notifier code
* Rename i8042_port.filter_dbg to i8042.driver_bound
* Instead of checking the parent of the current device, compare serio against
i8042_ports[I8042_KBD_PORT_NO].serio
* Use a switch case instead of an if-else when checking the action in the bus
notifier
* Export serio_bus into serio.h, register the bus notifiers at module load time
as opposed to port registration time
Documentation/kernel-parameters.txt | 3 +++
drivers/input/serio/i8042.c | 43 +++++++++++++++++++++++++++++++++----
drivers/input/serio/i8042.h | 13 +++++++++++
drivers/input/serio/serio.c | 4 +---
include/linux/serio.h | 2 ++
5 files changed, 58 insertions(+), 7 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ae44749..a9d2b19 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1304,6 +1304,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
<bus_id>,<clkrate>
i8042.debug [HW] Toggle i8042 debug mode
+ i8042.debug_kbd [HW] Enable printing of interrupt data from the KBD port
+ (disabled by default, requires that i8042.debug=1
+ be enabled)
i8042.direct [HW] Put keyboard port into non-translated mode
i8042.dumbkbd [HW] Pretend that controller can only read data from
keyboard and cannot control its state
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index cb5ece7..e29d6a4 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -88,6 +88,10 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
static bool i8042_debug;
module_param_named(debug, i8042_debug, bool, 0600);
MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
+
+static bool i8042_debug_kbd;
+module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600);
+MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or off (requires i8042.debug=1)");
#endif
static bool i8042_bypass_aux_irq_test;
@@ -116,6 +120,7 @@ struct i8042_port {
struct serio *serio;
int irq;
bool exists;
+ bool driver_bound;
signed char mux;
};
@@ -133,6 +138,7 @@ static bool i8042_kbd_irq_registered;
static bool i8042_aux_irq_registered;
static unsigned char i8042_suppress_kbd_ack;
static struct platform_device *i8042_platform_device;
+static struct notifier_block i8042_kbd_bind_notifier_block;
static irqreturn_t i8042_interrupt(int irq, void *dev_id);
static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
@@ -528,10 +534,10 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
port = &i8042_ports[port_no];
serio = port->exists ? port->serio : NULL;
- dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n",
- data, port_no, irq,
- dfl & SERIO_PARITY ? ", bad parity" : "",
- dfl & SERIO_TIMEOUT ? ", timeout" : "");
+ filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
+ port_no, irq,
+ dfl & SERIO_PARITY ? ", bad parity" : "",
+ dfl & SERIO_TIMEOUT ? ", timeout" : "");
filtered = i8042_filter(data, str, serio);
@@ -1438,6 +1444,29 @@ static int __init i8042_setup_kbd(void)
return error;
}
+static int i8042_kbd_bind_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ struct serio *serio = to_serio_port(dev);
+ struct i8042_port *port = serio->port_data;
+
+ if (serio != i8042_ports[I8042_KBD_PORT_NO].serio)
+ return 0;
+
+ switch (action) {
+ case BUS_NOTIFY_BOUND_DRIVER:
+ port->driver_bound = true;
+ break;
+
+ case BUS_NOTIFY_UNBOUND_DRIVER:
+ port->driver_bound = false;
+ break;
+ }
+
+ return 0;
+}
+
static int __init i8042_probe(struct platform_device *dev)
{
int error;
@@ -1507,6 +1536,10 @@ static struct platform_driver i8042_driver = {
.shutdown = i8042_shutdown,
};
+static struct notifier_block i8042_kbd_bind_notifier_block = {
+ .notifier_call = i8042_kbd_bind_notifier,
+};
+
static int __init i8042_init(void)
{
struct platform_device *pdev;
@@ -1528,6 +1561,7 @@ static int __init i8042_init(void)
goto err_platform_exit;
}
+ bus_register_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
panic_blink = i8042_panic_blink;
return 0;
@@ -1543,6 +1577,7 @@ static void __exit i8042_exit(void)
platform_driver_unregister(&i8042_driver);
i8042_platform_exit();
+ bus_unregister_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
panic_blink = NULL;
}
diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
index fc080be..a198f0d 100644
--- a/drivers/input/serio/i8042.h
+++ b/drivers/input/serio/i8042.h
@@ -73,6 +73,17 @@ static unsigned long i8042_start_time;
printk(KERN_DEBUG KBUILD_MODNAME ": [%d] " format, \
(int) (jiffies - i8042_start_time), ##arg); \
} while (0)
+
+#define filter_dbg(filter, data, format, args...) \
+ do { \
+ if (!i8042_debug) \
+ break; \
+ \
+ if (!filter || i8042_debug_kbd) \
+ dbg("%02x " format, data, ##args); \
+ else \
+ dbg("** " format, ##args); \
+ } while (0)
#else
#define dbg_init() do { } while (0)
#define dbg(format, arg...) \
@@ -80,6 +91,8 @@ static unsigned long i8042_start_time;
if (0) \
printk(KERN_DEBUG pr_fmt(format), ##arg); \
} while (0)
+
+#define filter_dbg(filter, data, format, args...) do { } while (0)
#endif
#endif /* _I8042_H */
diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index a05a517..63422d2 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -49,8 +49,6 @@ static DEFINE_MUTEX(serio_mutex);
static LIST_HEAD(serio_list);
-static struct bus_type serio_bus;
-
static void serio_add_port(struct serio *serio);
static int serio_reconnect_port(struct serio *serio);
static void serio_disconnect_port(struct serio *serio);
@@ -1017,7 +1015,7 @@ irqreturn_t serio_interrupt(struct serio *serio,
}
EXPORT_SYMBOL(serio_interrupt);
-static struct bus_type serio_bus = {
+struct bus_type serio_bus = {
.name = "serio",
.drv_groups = serio_driver_groups,
.match = serio_bus_match,
diff --git a/include/linux/serio.h b/include/linux/serio.h
index 9f779c7..df4ab5d 100644
--- a/include/linux/serio.h
+++ b/include/linux/serio.h
@@ -18,6 +18,8 @@
#include <linux/mod_devicetable.h>
#include <uapi/linux/serio.h>
+extern struct bus_type serio_bus;
+
struct serio {
void *port_data;
--
2.4.3
Hi,
[no In-Reply-To header - lkml.org "headers" is broken ATM]
> +
> +static bool i8042_debug_kbd;
> +module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600);
> +MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or off
> (requires i8042.debug=1)");
seems inconsistent:
i8042_debug_kbd != debug_kbd != i8042_kbd
While the first two seem perfectly fine,
"i8042_kbd" sounds like a build error or similar to me, on the severity front.
(grepping kernel tree drivers/ on quick glance
does not seem to show any naming deviations in the MODULE_PARM_DESC area)
> "Turn i8042 kbd debugging output on or off (requires i8042.debug=1)"
should be improved to
"Turn i8042 kbd debugging output on (requires i8042.debug=1)"
(it *is* default-off)
The point is that it
(at least now that it reached current implementation version?)
merely *enables* additional output,
it does *not* actively *disable* (veto)
something which may have been default-enabled elsewhere.
Also, since this is about "special" situations only
(many standard situations already have this output enabled),
it should be worded to somehow include this "special" enabling.
Also, I'd prefer to also see the *reason*
for it being default-disabled in modinfo output.
Also, "i8042" is useless (since completely scope-superfluous) information
(this *is* i8042 driver)
So perhaps wording in total could be something like
"Turn kbd debugging output unconditionally on (may reveal sensitive data)"
or possibly best
"Unconditional enable (may reveal sensitive data) of normally sanitize-filtered kbd data traffic log"
and in combination:
"[DESCRIPTION] (pre-condition: i8042.debug=1 enabled)"
"kbd debugging output"
could be shortened to
"kbd debug log"
So, a final suggestion might be:
"Unconditional enable (may reveal sensitive data) of normally sanitize-filtered kbd data traffic debug log [pre-condition: i8042.debug=1 enabled]"
And given that this description is now completely different,
one might choose to rename debug_kbd variable
to something more specific, too
("debug_full" / "debug_data" / "debug_traffic"?).
> + i8042.debug_kbd [HW] Enable printing of interrupt data from the
> KBD port
> + (disabled by default, requires that
> i8042.debug=1
> + be enabled)
is not correct - code implementation definitely conveys
that it needs to be "*and* requires"
(especially since current wording strongly suggests that
*while it's default-disabled*,
i8042.debug_kbd will be implicitly enabled once i8042.debug=1 is available,
which is wrong).
Perhaps write it as something like
"and as pre-condition requires i8042.debug=1 to be enabled too".
Definitely very good to see this (quote) "big problem" corrected!
Thanks,
Andreas Mohr
From: Stephen Chandler Paul <[email protected]>
A big problem with the current i8042 debugging option is that it outputs
data going to and from the keyboard by default. As a result, many dmesg
logs uploaded by users will unintentionally contain sensitive information
such as their password, as such it's probably a good idea not to output
data coming from the keyboard unless specifically enabled by the user.
Signed-off-by: Stephen Chandler Paul <[email protected]>
---
Changes
* Fix incorrect option name in MODULE_PARM_DESC() (I have no idea how that
happened in the first place)
* Replaced the description with one of the suggestions given by Andreas
* Rename debug_kbd to unmask_kbd_data
* Improve description in Documentation/kernel-parameters.txt
Documentation/kernel-parameters.txt | 4 ++++
drivers/input/serio/i8042.c | 43 +++++++++++++++++++++++++++++++++----
drivers/input/serio/i8042.h | 13 +++++++++++
drivers/input/serio/serio.c | 4 +---
include/linux/serio.h | 2 ++
5 files changed, 59 insertions(+), 7 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ae44749..352a5f1 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1304,6 +1304,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
<bus_id>,<clkrate>
i8042.debug [HW] Toggle i8042 debug mode
+ i8042.unmask_kbd_data
+ [HW] Enable printing of interrupt data from the KBD port
+ (disabled by default, and as a pre-condition
+ requires that i8042.debug=1 be enabled)
i8042.direct [HW] Put keyboard port into non-translated mode
i8042.dumbkbd [HW] Pretend that controller can only read data from
keyboard and cannot control its state
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index cb5ece7..e6d1529 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -88,6 +88,10 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
static bool i8042_debug;
module_param_named(debug, i8042_debug, bool, 0600);
MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
+
+static bool i8042_unmask_kbd_data;
+module_param_named(unmask_kbd_data, i8042_unmask_kbd_data, bool, 0600);
+MODULE_PARM_DESC(unmask_kbd_data, "Unconditional enable (may reveal sensitive data) of normally sanitize-filtered kbd data traffic debug log [pre-condition: i8042.debug=1 enabled]");
#endif
static bool i8042_bypass_aux_irq_test;
@@ -116,6 +120,7 @@ struct i8042_port {
struct serio *serio;
int irq;
bool exists;
+ bool driver_bound;
signed char mux;
};
@@ -133,6 +138,7 @@ static bool i8042_kbd_irq_registered;
static bool i8042_aux_irq_registered;
static unsigned char i8042_suppress_kbd_ack;
static struct platform_device *i8042_platform_device;
+static struct notifier_block i8042_kbd_bind_notifier_block;
static irqreturn_t i8042_interrupt(int irq, void *dev_id);
static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
@@ -528,10 +534,10 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
port = &i8042_ports[port_no];
serio = port->exists ? port->serio : NULL;
- dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n",
- data, port_no, irq,
- dfl & SERIO_PARITY ? ", bad parity" : "",
- dfl & SERIO_TIMEOUT ? ", timeout" : "");
+ filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
+ port_no, irq,
+ dfl & SERIO_PARITY ? ", bad parity" : "",
+ dfl & SERIO_TIMEOUT ? ", timeout" : "");
filtered = i8042_filter(data, str, serio);
@@ -1438,6 +1444,29 @@ static int __init i8042_setup_kbd(void)
return error;
}
+static int i8042_kbd_bind_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ struct serio *serio = to_serio_port(dev);
+ struct i8042_port *port = serio->port_data;
+
+ if (serio != i8042_ports[I8042_KBD_PORT_NO].serio)
+ return 0;
+
+ switch (action) {
+ case BUS_NOTIFY_BOUND_DRIVER:
+ port->driver_bound = true;
+ break;
+
+ case BUS_NOTIFY_UNBOUND_DRIVER:
+ port->driver_bound = false;
+ break;
+ }
+
+ return 0;
+}
+
static int __init i8042_probe(struct platform_device *dev)
{
int error;
@@ -1507,6 +1536,10 @@ static struct platform_driver i8042_driver = {
.shutdown = i8042_shutdown,
};
+static struct notifier_block i8042_kbd_bind_notifier_block = {
+ .notifier_call = i8042_kbd_bind_notifier,
+};
+
static int __init i8042_init(void)
{
struct platform_device *pdev;
@@ -1528,6 +1561,7 @@ static int __init i8042_init(void)
goto err_platform_exit;
}
+ bus_register_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
panic_blink = i8042_panic_blink;
return 0;
@@ -1543,6 +1577,7 @@ static void __exit i8042_exit(void)
platform_driver_unregister(&i8042_driver);
i8042_platform_exit();
+ bus_unregister_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
panic_blink = NULL;
}
diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
index fc080be..1db0a40 100644
--- a/drivers/input/serio/i8042.h
+++ b/drivers/input/serio/i8042.h
@@ -73,6 +73,17 @@ static unsigned long i8042_start_time;
printk(KERN_DEBUG KBUILD_MODNAME ": [%d] " format, \
(int) (jiffies - i8042_start_time), ##arg); \
} while (0)
+
+#define filter_dbg(filter, data, format, args...) \
+ do { \
+ if (!i8042_debug) \
+ break; \
+ \
+ if (!filter || i8042_unmask_kbd_data) \
+ dbg("%02x " format, data, ##args); \
+ else \
+ dbg("** " format, ##args); \
+ } while (0)
#else
#define dbg_init() do { } while (0)
#define dbg(format, arg...) \
@@ -80,6 +91,8 @@ static unsigned long i8042_start_time;
if (0) \
printk(KERN_DEBUG pr_fmt(format), ##arg); \
} while (0)
+
+#define filter_dbg(filter, data, format, args...) do { } while (0)
#endif
#endif /* _I8042_H */
diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index a05a517..63422d2 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -49,8 +49,6 @@ static DEFINE_MUTEX(serio_mutex);
static LIST_HEAD(serio_list);
-static struct bus_type serio_bus;
-
static void serio_add_port(struct serio *serio);
static int serio_reconnect_port(struct serio *serio);
static void serio_disconnect_port(struct serio *serio);
@@ -1017,7 +1015,7 @@ irqreturn_t serio_interrupt(struct serio *serio,
}
EXPORT_SYMBOL(serio_interrupt);
-static struct bus_type serio_bus = {
+struct bus_type serio_bus = {
.name = "serio",
.drv_groups = serio_driver_groups,
.match = serio_bus_match,
diff --git a/include/linux/serio.h b/include/linux/serio.h
index 9f779c7..df4ab5d 100644
--- a/include/linux/serio.h
+++ b/include/linux/serio.h
@@ -18,6 +18,8 @@
#include <linux/mod_devicetable.h>
#include <uapi/linux/serio.h>
+extern struct bus_type serio_bus;
+
struct serio {
void *port_data;
--
2.4.3
On Thu, Jul 02, 2015 at 11:46:48AM -0400, [email protected] wrote:
> Changes
> * Fix incorrect option name in MODULE_PARM_DESC() (I have no idea how that
> happened in the first place)
> * Replaced the description with one of the suggestions given by Andreas
> * Rename debug_kbd to unmask_kbd_data
> * Improve description in Documentation/kernel-parameters.txt
Patch in its entirety seems fine to me, thanks!
Reviewed-by: Andreas Mohr <[email protected]>
On Jul 02 2015 or thereabouts, [email protected] wrote:
> From: Stephen Chandler Paul <[email protected]>
>
> A big problem with the current i8042 debugging option is that it outputs
> data going to and from the keyboard by default. As a result, many dmesg
> logs uploaded by users will unintentionally contain sensitive information
> such as their password, as such it's probably a good idea not to output
> data coming from the keyboard unless specifically enabled by the user.
>
> Signed-off-by: Stephen Chandler Paul <[email protected]>
> ---
FWIW, in case you are missing my input Dmitry:
Reviewed-by: Benjamin Tissoires <[email protected]>
Thanks everyone involved in this process!
Cheers,
Benjamin
> Changes
> * Fix incorrect option name in MODULE_PARM_DESC() (I have no idea how that
> happened in the first place)
> * Replaced the description with one of the suggestions given by Andreas
> * Rename debug_kbd to unmask_kbd_data
> * Improve description in Documentation/kernel-parameters.txt
>
> Documentation/kernel-parameters.txt | 4 ++++
> drivers/input/serio/i8042.c | 43 +++++++++++++++++++++++++++++++++----
> drivers/input/serio/i8042.h | 13 +++++++++++
> drivers/input/serio/serio.c | 4 +---
> include/linux/serio.h | 2 ++
> 5 files changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index ae44749..352a5f1 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1304,6 +1304,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> <bus_id>,<clkrate>
>
> i8042.debug [HW] Toggle i8042 debug mode
> + i8042.unmask_kbd_data
> + [HW] Enable printing of interrupt data from the KBD port
> + (disabled by default, and as a pre-condition
> + requires that i8042.debug=1 be enabled)
> i8042.direct [HW] Put keyboard port into non-translated mode
> i8042.dumbkbd [HW] Pretend that controller can only read data from
> keyboard and cannot control its state
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index cb5ece7..e6d1529 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -88,6 +88,10 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
> static bool i8042_debug;
> module_param_named(debug, i8042_debug, bool, 0600);
> MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
> +
> +static bool i8042_unmask_kbd_data;
> +module_param_named(unmask_kbd_data, i8042_unmask_kbd_data, bool, 0600);
> +MODULE_PARM_DESC(unmask_kbd_data, "Unconditional enable (may reveal sensitive data) of normally sanitize-filtered kbd data traffic debug log [pre-condition: i8042.debug=1 enabled]");
> #endif
>
> static bool i8042_bypass_aux_irq_test;
> @@ -116,6 +120,7 @@ struct i8042_port {
> struct serio *serio;
> int irq;
> bool exists;
> + bool driver_bound;
> signed char mux;
> };
>
> @@ -133,6 +138,7 @@ static bool i8042_kbd_irq_registered;
> static bool i8042_aux_irq_registered;
> static unsigned char i8042_suppress_kbd_ack;
> static struct platform_device *i8042_platform_device;
> +static struct notifier_block i8042_kbd_bind_notifier_block;
>
> static irqreturn_t i8042_interrupt(int irq, void *dev_id);
> static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
> @@ -528,10 +534,10 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
> port = &i8042_ports[port_no];
> serio = port->exists ? port->serio : NULL;
>
> - dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n",
> - data, port_no, irq,
> - dfl & SERIO_PARITY ? ", bad parity" : "",
> - dfl & SERIO_TIMEOUT ? ", timeout" : "");
> + filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
> + port_no, irq,
> + dfl & SERIO_PARITY ? ", bad parity" : "",
> + dfl & SERIO_TIMEOUT ? ", timeout" : "");
>
> filtered = i8042_filter(data, str, serio);
>
> @@ -1438,6 +1444,29 @@ static int __init i8042_setup_kbd(void)
> return error;
> }
>
> +static int i8042_kbd_bind_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct device *dev = data;
> + struct serio *serio = to_serio_port(dev);
> + struct i8042_port *port = serio->port_data;
> +
> + if (serio != i8042_ports[I8042_KBD_PORT_NO].serio)
> + return 0;
> +
> + switch (action) {
> + case BUS_NOTIFY_BOUND_DRIVER:
> + port->driver_bound = true;
> + break;
> +
> + case BUS_NOTIFY_UNBOUND_DRIVER:
> + port->driver_bound = false;
> + break;
> + }
> +
> + return 0;
> +}
> +
> static int __init i8042_probe(struct platform_device *dev)
> {
> int error;
> @@ -1507,6 +1536,10 @@ static struct platform_driver i8042_driver = {
> .shutdown = i8042_shutdown,
> };
>
> +static struct notifier_block i8042_kbd_bind_notifier_block = {
> + .notifier_call = i8042_kbd_bind_notifier,
> +};
> +
> static int __init i8042_init(void)
> {
> struct platform_device *pdev;
> @@ -1528,6 +1561,7 @@ static int __init i8042_init(void)
> goto err_platform_exit;
> }
>
> + bus_register_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
> panic_blink = i8042_panic_blink;
>
> return 0;
> @@ -1543,6 +1577,7 @@ static void __exit i8042_exit(void)
> platform_driver_unregister(&i8042_driver);
> i8042_platform_exit();
>
> + bus_unregister_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
> panic_blink = NULL;
> }
>
> diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
> index fc080be..1db0a40 100644
> --- a/drivers/input/serio/i8042.h
> +++ b/drivers/input/serio/i8042.h
> @@ -73,6 +73,17 @@ static unsigned long i8042_start_time;
> printk(KERN_DEBUG KBUILD_MODNAME ": [%d] " format, \
> (int) (jiffies - i8042_start_time), ##arg); \
> } while (0)
> +
> +#define filter_dbg(filter, data, format, args...) \
> + do { \
> + if (!i8042_debug) \
> + break; \
> + \
> + if (!filter || i8042_unmask_kbd_data) \
> + dbg("%02x " format, data, ##args); \
> + else \
> + dbg("** " format, ##args); \
> + } while (0)
> #else
> #define dbg_init() do { } while (0)
> #define dbg(format, arg...) \
> @@ -80,6 +91,8 @@ static unsigned long i8042_start_time;
> if (0) \
> printk(KERN_DEBUG pr_fmt(format), ##arg); \
> } while (0)
> +
> +#define filter_dbg(filter, data, format, args...) do { } while (0)
> #endif
>
> #endif /* _I8042_H */
> diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
> index a05a517..63422d2 100644
> --- a/drivers/input/serio/serio.c
> +++ b/drivers/input/serio/serio.c
> @@ -49,8 +49,6 @@ static DEFINE_MUTEX(serio_mutex);
>
> static LIST_HEAD(serio_list);
>
> -static struct bus_type serio_bus;
> -
> static void serio_add_port(struct serio *serio);
> static int serio_reconnect_port(struct serio *serio);
> static void serio_disconnect_port(struct serio *serio);
> @@ -1017,7 +1015,7 @@ irqreturn_t serio_interrupt(struct serio *serio,
> }
> EXPORT_SYMBOL(serio_interrupt);
>
> -static struct bus_type serio_bus = {
> +struct bus_type serio_bus = {
> .name = "serio",
> .drv_groups = serio_driver_groups,
> .match = serio_bus_match,
> diff --git a/include/linux/serio.h b/include/linux/serio.h
> index 9f779c7..df4ab5d 100644
> --- a/include/linux/serio.h
> +++ b/include/linux/serio.h
> @@ -18,6 +18,8 @@
> #include <linux/mod_devicetable.h>
> #include <uapi/linux/serio.h>
>
> +extern struct bus_type serio_bus;
> +
> struct serio {
> void *port_data;
>
> --
> 2.4.3
>
Hi Stephen,
On Thu, Jul 02, 2015 at 11:46:48AM -0400, [email protected] wrote:
> +static int i8042_kbd_bind_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct device *dev = data;
> + struct serio *serio = to_serio_port(dev);
> + struct i8042_port *port = serio->port_data;
> +
> + if (serio != i8042_ports[I8042_KBD_PORT_NO].serio)
> + return 0;
> +
> + switch (action) {
> + case BUS_NOTIFY_BOUND_DRIVER:
> + port->driver_bound = true;
> + break;
> +
> + case BUS_NOTIFY_UNBOUND_DRIVER:
I think it should be BUS_NOTIFY_UNBIND_DRIVER so that we see the KBD
data as it goes through driver cleanup. Yell if you disagree, or I'll
change it locally.
Thanks.
--
Dmitry
On Wed, 2015-07-15 at 09:59 -0700, Dmitry Torokhov wrote:
> Hi Stephen,
>
> On Thu, Jul 02, 2015 at 11:46:48AM -0400, [email protected] wrote:
> > +static int i8042_kbd_bind_notifier(struct notifier_block *nb,
> > + unsigned long action, void
> > *data)
> > +{
> > + struct device *dev = data;
> > + struct serio *serio = to_serio_port(dev);
> > + struct i8042_port *port = serio->port_data;
> > +
> > + if (serio != i8042_ports[I8042_KBD_PORT_NO].serio)
> > + return 0;
> > +
> > + switch (action) {
> > + case BUS_NOTIFY_BOUND_DRIVER:
> > + port->driver_bound = true;
> > + break;
> > +
> > + case BUS_NOTIFY_UNBOUND_DRIVER:
>
> I think it should be BUS_NOTIFY_UNBIND_DRIVER so that we see the KBD
> data as it goes through driver cleanup. Yell if you disagree, or I'll
> change it locally.
Sounds good to me
Cheers,
Stephen Chandler Paul
>
> Thanks.
>