| Message ID | 20210217190718.11035-3-ronald@innovation.ch |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] Input: applespi: Don't wait for responses to commands indefinitely. | expand |
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.
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
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
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
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 --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) {
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(+)