diff mbox series

[3/3] Input: applespi: Add trace_event module param for early tracing.

Message ID 20210217190718.11035-3-ronald@innovation.ch (mailing list archive)
State New
Headers show
Series [1/3] Input: applespi: Don't wait for responses to commands indefinitely. | expand

Commit Message

Life is hard, and then you die Feb. 17, 2021, 7:07 p.m. UTC
The problem is that tracing can't be set via sysfs until the module is
loaded, at which point the keyboard and trackpad initialization commands
have already been run and hence tracing can't be used to debug problems
here.

Adding this param allows tracing to be enabled for messages sent and
received during module probing. It takes comma-separated list of events,
e.g.

  trace_event=applespi_tp_ini_cmd,applespi_bad_crc

Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
 drivers/input/keyboard/applespi.c | 32 +++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Dmitry Torokhov Feb. 17, 2021, 8:26 p.m. UTC | #1
Hi Ronald,

On Wed, Feb 17, 2021 at 11:07:18AM -0800, Ronald Tschalär wrote:
> The problem is that tracing can't be set via sysfs until the module is
> loaded, at which point the keyboard and trackpad initialization commands
> have already been run and hence tracing can't be used to debug problems
> here.
> 
> Adding this param allows tracing to be enabled for messages sent and
> received during module probing. It takes comma-separated list of events,
> e.g.
> 
>   trace_event=applespi_tp_ini_cmd,applespi_bad_crc

You can unbind and rebind a device to a driver via sysfs as many times
as needed (see bind and unbind driver sysfs attributes), so I believe

Thanks.
Life is hard, and then you die Feb. 17, 2021, 8:52 p.m. UTC | #2
Hi Dmitry,

On Wed, Feb 17, 2021 at 12:26:18PM -0800, Dmitry Torokhov wrote:
> 
> On Wed, Feb 17, 2021 at 11:07:18AM -0800, Ronald Tschalär wrote:
> > The problem is that tracing can't be set via sysfs until the module is
> > loaded, at which point the keyboard and trackpad initialization commands
> > have already been run and hence tracing can't be used to debug problems
> > here.
> > 
> > Adding this param allows tracing to be enabled for messages sent and
> > received during module probing. It takes comma-separated list of events,
> > e.g.
> > 
> >   trace_event=applespi_tp_ini_cmd,applespi_bad_crc
> 
> You can unbind and rebind a device to a driver via sysfs as many times
> as needed (see bind and unbind driver sysfs attributes), so I believe

Hmm, I'm going to have to play with that a bit, but one place it still
won't help I think is something we ran into in practise: init was
failing during boot, but was successfull later on.


  Cheers,

  Ronald
Dmitry Torokhov Feb. 17, 2021, 9:06 p.m. UTC | #3
On Wed, Feb 17, 2021 at 12:52:57PM -0800, Life is hard, and then you die wrote:
> 
>   Hi Dmitry,
> 
> On Wed, Feb 17, 2021 at 12:26:18PM -0800, Dmitry Torokhov wrote:
> > 
> > On Wed, Feb 17, 2021 at 11:07:18AM -0800, Ronald Tschalär wrote:
> > > The problem is that tracing can't be set via sysfs until the module is
> > > loaded, at which point the keyboard and trackpad initialization commands
> > > have already been run and hence tracing can't be used to debug problems
> > > here.
> > > 
> > > Adding this param allows tracing to be enabled for messages sent and
> > > received during module probing. It takes comma-separated list of events,
> > > e.g.
> > > 
> > >   trace_event=applespi_tp_ini_cmd,applespi_bad_crc
> > 
> > You can unbind and rebind a device to a driver via sysfs as many times
> > as needed (see bind and unbind driver sysfs attributes), so I believe
> 
> Hmm, I'm going to have to play with that a bit, but one place it still
> won't help I think is something we ran into in practise: init was
> failing during boot, but was successfull later on.

Maybe compiling module as a built-in and then using kernel command line
option to initiate the trace would work?

If this facility is really needed, it would be beneficial for other
modules as well, and thus better implemented in the module loading code
to activate desired tracing after loading the module but before invoking
module init code.

Thanks.
Life is hard, and then you die Feb. 17, 2021, 10:34 p.m. UTC | #4
Hi Dmitry,

On Wed, Feb 17, 2021 at 01:06:27PM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 17, 2021 at 12:52:57PM -0800, Life is hard, and then you die wrote:
> > 
> > On Wed, Feb 17, 2021 at 12:26:18PM -0800, Dmitry Torokhov wrote:
> > > 
> > > On Wed, Feb 17, 2021 at 11:07:18AM -0800, Ronald Tschalär wrote:
> > > > The problem is that tracing can't be set via sysfs until the module is
> > > > loaded, at which point the keyboard and trackpad initialization commands
> > > > have already been run and hence tracing can't be used to debug problems
> > > > here.
> > > > 
> > > > Adding this param allows tracing to be enabled for messages sent and
> > > > received during module probing. It takes comma-separated list of events,
> > > > e.g.
> > > > 
> > > >   trace_event=applespi_tp_ini_cmd,applespi_bad_crc
> > > 
> > > You can unbind and rebind a device to a driver via sysfs as many times
> > > as needed (see bind and unbind driver sysfs attributes), so I believe

Ok yes, that works well, except for the boot-debug scenario.

> > Hmm, I'm going to have to play with that a bit, but one place it still
> > won't help I think is something we ran into in practise: init was
> > failing during boot, but was successfull later on.
> 
> Maybe compiling module as a built-in and then using kernel command line
> option to initiate the trace would work?

My personal issue with this is the fact that most folks reporting
issues are running their distro's standard kernel, which invariably
has this (and most others) compiled as a loadable module; and asking
folks to rebuild their kernel is actually quite a hurdle for them, in
particular compared to asking them to just add some boot params or
manipulating some sysfs entries. So I prefer to try to provide easy
ways for folks to be able to generate and report info back that work
and are enabled out-of-the-box on most distros.

> If this facility is really needed, it would be beneficial for other
> modules as well, and thus better implemented in the module loading code
> to activate desired tracing after loading the module but before invoking
> module init code.

I don't know if it rises to the level of "really needed" - I certainly
needed something like this to debug an issue and hence the module
param. And I figured if somebody adds/debugs additional init commands
they could find it useful too. But this may not be commonly needed
after all, or folks are using some other solution.

If there's interest, I might be able to take a stab a this in the near
future, but not sure.


  Cheers,

  Ronald
kernel test robot Feb. 18, 2021, 12:18 a.m. UTC | #5
Hi "Ronald,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on input/next]
[also build test ERROR on v5.11 next-20210217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/Input-applespi-Don-t-wait-for-responses-to-commands-indefinitely/20210218-032205
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: i386-randconfig-s001-20210217 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-215-g0fb77bb6-dirty
        # https://github.com/0day-ci/linux/commit/838cd23e96675132bdd30878d1b24a59b8a534e1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ronald-Tschal-r/Input-applespi-Don-t-wait-for-responses-to-commands-indefinitely/20210218-032205
        git checkout 838cd23e96675132bdd30878d1b24a59b8a534e1
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: drivers/input/keyboard/applespi.o: in function `applespi_enable_early_event_tracing':
>> drivers/input/keyboard/applespi.c:1666: undefined reference to `trace_set_clr_event'


vim +1666 drivers/input/keyboard/applespi.c

  1653	
  1654	static void applespi_enable_early_event_tracing(struct device *dev)
  1655	{
  1656		char *buf, *event;
  1657		int sts;
  1658	
  1659		if (trace_event && trace_event[0]) {
  1660			buf = kstrdup(trace_event, GFP_KERNEL);
  1661			if (!buf)
  1662				return;
  1663	
  1664			while ((event = strsep(&buf, ","))) {
  1665				if (event[0]) {
> 1666					sts = trace_set_clr_event("applespi", event, 1);
  1667					if (sts)
  1668						dev_warn(dev,
  1669							 "Error setting trace event '%s': %d\n",
  1670							 event, sts);
  1671				}
  1672			}
  1673	
  1674			kfree(buf);
  1675		}
  1676	}
  1677	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Feb. 18, 2021, 4:41 a.m. UTC | #6
Hi "Ronald,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on input/next]
[also build test ERROR on v5.11 next-20210217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/Input-applespi-Don-t-wait-for-responses-to-commands-indefinitely/20210218-032205
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: ia64-allyesconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/838cd23e96675132bdd30878d1b24a59b8a534e1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ronald-Tschal-r/Input-applespi-Don-t-wait-for-responses-to-commands-indefinitely/20210218-032205
        git checkout 838cd23e96675132bdd30878d1b24a59b8a534e1
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ia64-linux-ld: drivers/input/keyboard/applespi.o: in function `applespi_probe':
>> applespi.c:(.text+0xf22): undefined reference to `trace_set_clr_event'

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for FRAME_POINTER
   Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS
   Selected by
   - FAULT_INJECTION_STACKTRACE_FILTER && FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM && !ARC && !X86

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index f0a0067c48ff6..03f9ad0f83967 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -53,6 +53,8 @@ 
 #include <linux/module.h>
 #include <linux/spinlock.h>
 #include <linux/spi/spi.h>
+#include <linux/string.h>
+#include <linux/trace_events.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 
@@ -110,6 +112,10 @@  module_param_string(touchpad_dimensions, touchpad_dimensions,
 		    sizeof(touchpad_dimensions), 0444);
 MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the touchpad, as XxY+W+H .");
 
+static char *trace_event;
+module_param(trace_event, charp, 0444);
+MODULE_PARM_DESC(trace_event, "Enable early event tracing. It takes the form of a comma-separated list of events to enable.");
+
 /**
  * struct keyboard_protocol - keyboard message.
  * message.type = 0x0110, message.length = 0x000a
@@ -1645,6 +1651,30 @@  static void applespi_save_bl_level(struct applespi_data *applespi,
 			 "Error saving backlight level to EFI vars: %d\n", sts);
 }
 
+static void applespi_enable_early_event_tracing(struct device *dev)
+{
+	char *buf, *event;
+	int sts;
+
+	if (trace_event && trace_event[0]) {
+		buf = kstrdup(trace_event, GFP_KERNEL);
+		if (!buf)
+			return;
+
+		while ((event = strsep(&buf, ","))) {
+			if (event[0]) {
+				sts = trace_set_clr_event("applespi", event, 1);
+				if (sts)
+					dev_warn(dev,
+						 "Error setting trace event '%s': %d\n",
+						 event, sts);
+			}
+		}
+
+		kfree(buf);
+	}
+}
+
 static int applespi_probe(struct spi_device *spi)
 {
 	struct applespi_data *applespi;
@@ -1653,6 +1683,8 @@  static int applespi_probe(struct spi_device *spi)
 	int sts, i;
 	unsigned long long gpe, usb_status;
 
+	applespi_enable_early_event_tracing(&spi->dev);
+
 	/* check if the USB interface is present and enabled already */
 	acpi_sts = acpi_evaluate_integer(spi_handle, "UIST", NULL, &usb_status);
 	if (ACPI_SUCCESS(acpi_sts) && usb_status) {