[PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Alex Hung]

Unsupported events is only useful for developers and does not meaningful
for users. Using dev_dbg makes more sense and reduces noise in kernel
messages.

Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 drivers/platform/x86/intel-vbtn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 61f1063..10f92ac 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -83,7 +83,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	} else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) {
 		return;
 	}
-	dev_info(&device->dev, "unknown event index 0x%x\n", event);
+	dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
 }
 
 static int intel_vbtn_probe(struct platform_device *device)
-- 
2.7.4

Re: [PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Darren Hart]

On Thu, Jul 20, 2017 at 08:56:48PM -0700, Alex Hung wrote:
> Unsupported events is only useful for developers and does not meaningful
> for users. Using dev_dbg makes more sense and reduces noise in kernel
> messages.
> 
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>  drivers/platform/x86/intel-vbtn.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
> index 61f1063..10f92ac 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -83,7 +83,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  	} else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) {
>  		return;
>  	}
> -	dev_info(&device->dev, "unknown event index 0x%x\n", event);
> +	dev_dbg(&device->dev, "unknown event index 0x%x\n", event);

info is the most common log level for these events in the platform
driver x86 subsystem per 'git grep -i "unknown event"'.

My take on this is that we want these to be reported by users, rather
than rely on developers to find them all - especially as the developers
only see a fraction of the affected hardware.

Are you finding these to be causing a problem / or producing really
excessive log messages?

Andy, what are your thoughts?

>  }
>  
>  static int intel_vbtn_probe(struct platform_device *device)
> -- 
> 2.7.4
> 
> 

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Rafael J. Wysocki]

On Friday, July 21, 2017 04:16:20 PM Darren Hart wrote:
> On Thu, Jul 20, 2017 at 08:56:48PM -0700, Alex Hung wrote:
> > Unsupported events is only useful for developers and does not meaningful
> > for users. Using dev_dbg makes more sense and reduces noise in kernel
> > messages.
> > 
> > Signed-off-by: Alex Hung <alex.hung@canonical.com>
> > ---
> >  drivers/platform/x86/intel-vbtn.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
> > index 61f1063..10f92ac 100644
> > --- a/drivers/platform/x86/intel-vbtn.c
> > +++ b/drivers/platform/x86/intel-vbtn.c
> > @@ -83,7 +83,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
> >  	} else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) {
> >  		return;
> >  	}
> > -	dev_info(&device->dev, "unknown event index 0x%x\n", event);
> > +	dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> 
> info is the most common log level for these events in the platform
> driver x86 subsystem per 'git grep -i "unknown event"'.
> 
> My take on this is that we want these to be reported by users,

Well, for what purpose?

> rather than rely on developers to find them all - especially as the
> developers only see a fraction of the affected hardware.
> 
> Are you finding these to be causing a problem / or producing really
> excessive log messages?

These messages suggest that there's something wrong while it may just be
events that we really don't care about.

I'd rather want users to report missing functionality or things not working as
expected than silly messages in the log that may just be meaningless in
practice.

Thanks,
Rafael

Re: [PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Andy Shevchenko]

On Sat, Jul 22, 2017 at 2:16 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Thu, Jul 20, 2017 at 08:56:48PM -0700, Alex Hung wrote:
>> Unsupported events is only useful for developers and does not meaningful
>> for users. Using dev_dbg makes more sense and reduces noise in kernel
>> messages.

>> -     dev_info(&device->dev, "unknown event index 0x%x\n", event);
>> +     dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
>
> info is the most common log level for these events in the platform
> driver x86 subsystem per 'git grep -i "unknown event"'.
>
> My take on this is that we want these to be reported by users, rather
> than rely on developers to find them all - especially as the developers
> only see a fraction of the affected hardware.
>
> Are you finding these to be causing a problem / or producing really
> excessive log messages?
>
> Andy, what are your thoughts?

My opinion is slightly closer to Rafael's one.

I think this is a debugging context.

If we really care about reporting them we might go HID way, i.e. using
dev_printk(KERN_DEBUG ) with module parameter debug.

As developer I'm against that.
As a regular user I do not need to recompile a kernel, in case there
is no dynamic debug support, and it allows me to enable debug
messages.

P.S. To see how important message above is, do we have any statistics
how many bug reports / email we got wrt the issue and how many had
been addressed?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Rafael J. Wysocki]

On Monday, July 24, 2017 12:19:25 PM Andy Shevchenko wrote:
> On Sat, Jul 22, 2017 at 2:16 AM, Darren Hart <dvhart@infradead.org> wrote:
> > On Thu, Jul 20, 2017 at 08:56:48PM -0700, Alex Hung wrote:
> >> Unsupported events is only useful for developers and does not meaningful
> >> for users. Using dev_dbg makes more sense and reduces noise in kernel
> >> messages.
> 
> >> -     dev_info(&device->dev, "unknown event index 0x%x\n", event);
> >> +     dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> >
> > info is the most common log level for these events in the platform
> > driver x86 subsystem per 'git grep -i "unknown event"'.
> >
> > My take on this is that we want these to be reported by users, rather
> > than rely on developers to find them all - especially as the developers
> > only see a fraction of the affected hardware.
> >
> > Are you finding these to be causing a problem / or producing really
> > excessive log messages?
> >
> > Andy, what are your thoughts?
> 
> My opinion is slightly closer to Rafael's one.
> 
> I think this is a debugging context.
> 
> If we really care about reporting them we might go HID way, i.e. using
> dev_printk(KERN_DEBUG ) with module parameter debug.
> 
> As developer I'm against that.
> As a regular user I do not need to recompile a kernel, in case there
> is no dynamic debug support, and it allows me to enable debug
> messages.
> 
> P.S. To see how important message above is, do we have any statistics
> how many bug reports / email we got wrt the issue and how many had
> been addressed?

Well, there is one I'm aware of, but this particular one we aren't going to
address at all, because apparently we handle the event in question anyway
in a different way.

In any case, the only situation in which this information is useful at all is
when some functionality is missing and you want to find out why.

Otherwise you get messages telling you that something *may* *be* missing,
but as a user you have no idea what to do about that, because you don't
even know how to report it and to whom (in case you want to report it).

Thanks,
Rafael

Re: [PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Darren Hart]

On Mon, Jul 24, 2017 at 01:41:06PM +0200, Rafael Wysocki wrote:
> On Monday, July 24, 2017 12:19:25 PM Andy Shevchenko wrote:
> > On Sat, Jul 22, 2017 at 2:16 AM, Darren Hart <dvhart@infradead.org> wrote:
> > > On Thu, Jul 20, 2017 at 08:56:48PM -0700, Alex Hung wrote:
> > >> Unsupported events is only useful for developers and does not meaningful
> > >> for users. Using dev_dbg makes more sense and reduces noise in kernel
> > >> messages.
> > 
> > >> -     dev_info(&device->dev, "unknown event index 0x%x\n", event);
> > >> +     dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> > >
> > > info is the most common log level for these events in the platform
> > > driver x86 subsystem per 'git grep -i "unknown event"'.
> > >
> > > My take on this is that we want these to be reported by users, rather
> > > than rely on developers to find them all - especially as the developers
> > > only see a fraction of the affected hardware.
> > >
> > > Are you finding these to be causing a problem / or producing really
> > > excessive log messages?
> > >
> > > Andy, what are your thoughts?
> > 
> > My opinion is slightly closer to Rafael's one.
> > 
> > I think this is a debugging context.
> > 
> > If we really care about reporting them we might go HID way, i.e. using
> > dev_printk(KERN_DEBUG ) with module parameter debug.
> > 
> > As developer I'm against that.
> > As a regular user I do not need to recompile a kernel, in case there
> > is no dynamic debug support, and it allows me to enable debug
> > messages.
> > 
> > P.S. To see how important message above is, do we have any statistics
> > how many bug reports / email we got wrt the issue and how many had
> > been addressed?
> 
> Well, there is one I'm aware of, but this particular one we aren't going to
> address at all, because apparently we handle the event in question anyway
> in a different way.
> 
> In any case, the only situation in which this information is useful at all is
> when some functionality is missing and you want to find out why.
> 
> Otherwise you get messages telling you that something *may* *be* missing,
> but as a user you have no idea what to do about that, because you don't
> even know how to report it and to whom (in case you want to report it).

My thinking was along the lines of the keymaps where we explicitly add
KE_IGNORE when it is a key we don't care about. In that case, it is
useful to have the messages because if that occurs we want to know so we
can update the driver.

This driver also has KE_IGNORE entries in the intel_vbtn_keymap.

Are we talking about unknown keys - or are we talking about something
else?

If unknown keys, the additional messages will be minimal, and missing
keys are most likely to be reported if the message is obvious. Given the
sparse access to hardware by the developers, I prefer to have the
message.

If there is more to what is going on here - can someone provide an
example of where this is causing a problem? Or where the event causing
the message is not something we will add code to catch / ignore?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Darren Hart]

+Steven Rostedt

Steve is looking into current issues with printk and expressed an interest in
this example.

On Mon, Jul 24, 2017 at 06:14:36PM -0700, Darren Hart wrote:
> On Mon, Jul 24, 2017 at 01:41:06PM +0200, Rafael Wysocki wrote:
> > On Monday, July 24, 2017 12:19:25 PM Andy Shevchenko wrote:
> > > On Sat, Jul 22, 2017 at 2:16 AM, Darren Hart <dvhart@infradead.org> wrote:
> > > > On Thu, Jul 20, 2017 at 08:56:48PM -0700, Alex Hung wrote:
> > > >> Unsupported events is only useful for developers and does not meaningful
> > > >> for users. Using dev_dbg makes more sense and reduces noise in kernel
> > > >> messages.
> > > 
> > > >> -     dev_info(&device->dev, "unknown event index 0x%x\n", event);
> > > >> +     dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> > > >
> > > > info is the most common log level for these events in the platform
> > > > driver x86 subsystem per 'git grep -i "unknown event"'.
> > > >
> > > > My take on this is that we want these to be reported by users, rather
> > > > than rely on developers to find them all - especially as the developers
> > > > only see a fraction of the affected hardware.
> > > >
> > > > Are you finding these to be causing a problem / or producing really
> > > > excessive log messages?
> > > >
> > > > Andy, what are your thoughts?
> > > 
> > > My opinion is slightly closer to Rafael's one.
> > > 
> > > I think this is a debugging context.
> > > 
> > > If we really care about reporting them we might go HID way, i.e. using
> > > dev_printk(KERN_DEBUG ) with module parameter debug.
> > > 
> > > As developer I'm against that.
> > > As a regular user I do not need to recompile a kernel, in case there
> > > is no dynamic debug support, and it allows me to enable debug
> > > messages.
> > > 
> > > P.S. To see how important message above is, do we have any statistics
> > > how many bug reports / email we got wrt the issue and how many had
> > > been addressed?
> > 
> > Well, there is one I'm aware of, but this particular one we aren't going to
> > address at all, because apparently we handle the event in question anyway
> > in a different way.
> > 
> > In any case, the only situation in which this information is useful at all is
> > when some functionality is missing and you want to find out why.
> > 
> > Otherwise you get messages telling you that something *may* *be* missing,
> > but as a user you have no idea what to do about that, because you don't
> > even know how to report it and to whom (in case you want to report it).
> 
> My thinking was along the lines of the keymaps where we explicitly add
> KE_IGNORE when it is a key we don't care about. In that case, it is
> useful to have the messages because if that occurs we want to know so we
> can update the driver.
> 
> This driver also has KE_IGNORE entries in the intel_vbtn_keymap.
> 
> Are we talking about unknown keys - or are we talking about something
> else?
> 
> If unknown keys, the additional messages will be minimal, and missing
> keys are most likely to be reported if the message is obvious. Given the
> sparse access to hardware by the developers, I prefer to have the
> message.
> 
> If there is more to what is going on here - can someone provide an
> example of where this is causing a problem? Or where the event causing
> the message is not something we will add code to catch / ignore?
> 
> -- 
> Darren Hart
> VMware Open Source Technology Center

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Rafael J. Wysocki]

On Monday, July 24, 2017 06:14:36 PM Darren Hart wrote:
> On Mon, Jul 24, 2017 at 01:41:06PM +0200, Rafael Wysocki wrote:
> > On Monday, July 24, 2017 12:19:25 PM Andy Shevchenko wrote:
> > > On Sat, Jul 22, 2017 at 2:16 AM, Darren Hart <dvhart@infradead.org> wrote:
> > > > On Thu, Jul 20, 2017 at 08:56:48PM -0700, Alex Hung wrote:
> > > >> Unsupported events is only useful for developers and does not meaningful
> > > >> for users. Using dev_dbg makes more sense and reduces noise in kernel
> > > >> messages.
> > > 
> > > >> -     dev_info(&device->dev, "unknown event index 0x%x\n", event);
> > > >> +     dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> > > >
> > > > info is the most common log level for these events in the platform
> > > > driver x86 subsystem per 'git grep -i "unknown event"'.
> > > >
> > > > My take on this is that we want these to be reported by users, rather
> > > > than rely on developers to find them all - especially as the developers
> > > > only see a fraction of the affected hardware.
> > > >
> > > > Are you finding these to be causing a problem / or producing really
> > > > excessive log messages?
> > > >
> > > > Andy, what are your thoughts?
> > > 
> > > My opinion is slightly closer to Rafael's one.
> > > 
> > > I think this is a debugging context.
> > > 
> > > If we really care about reporting them we might go HID way, i.e. using
> > > dev_printk(KERN_DEBUG ) with module parameter debug.
> > > 
> > > As developer I'm against that.
> > > As a regular user I do not need to recompile a kernel, in case there
> > > is no dynamic debug support, and it allows me to enable debug
> > > messages.
> > > 
> > > P.S. To see how important message above is, do we have any statistics
> > > how many bug reports / email we got wrt the issue and how many had
> > > been addressed?
> > 
> > Well, there is one I'm aware of, but this particular one we aren't going to
> > address at all, because apparently we handle the event in question anyway
> > in a different way.
> > 
> > In any case, the only situation in which this information is useful at all is
> > when some functionality is missing and you want to find out why.
> > 
> > Otherwise you get messages telling you that something *may* *be* missing,
> > but as a user you have no idea what to do about that, because you don't
> > even know how to report it and to whom (in case you want to report it).
> 
> My thinking was along the lines of the keymaps where we explicitly add
> KE_IGNORE when it is a key we don't care about. In that case, it is
> useful to have the messages because if that occurs we want to know so we
> can update the driver.

Well, I'm not sure if adding every reported event to keymaps as KE_IGNORE
is a good idea, because that would require somebody to figure out what the
event is about every time and that's work which quite frankly is not very
useful (the key is still ignored if it is "unknown").

This kind of is blacklisting which is always painful from the maintenance
standpoint.

> This driver also has KE_IGNORE entries in the intel_vbtn_keymap.
> 
> Are we talking about unknown keys - or are we talking about something
> else?

Unknown keys.

> If unknown keys, the additional messages will be minimal, and missing
> keys are most likely to be reported if the message is obvious. Given the
> sparse access to hardware by the developers, I prefer to have the
> message.

Having it does not guarantee that it will be reported and even so, it is
not particularly clear where to send those reports and who is going to act on
them.

> If there is more to what is going on here - can someone provide an
> example of where this is causing a problem? Or where the event causing
> the message is not something we will add code to catch / ignore?

The particular one that tirggered this is related to the switching between
the "laptop" and "tablet" modes on Dell 9365 which according to Mario is
handled already through the ISH driver.

We could potentially propagate this event to user space, but we have no
idea how user space decides to handle it and we are not sure if this is going
to be used consistently for this purpose on all platforms.

Anyway, my opinion is that dev_dbg() messages are sufficient for such things
as they allow people to selectively turn the messages on and see what happens
if something seems to be missing, but otherwise they don't generate log noise.

Re: [PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Darren Hart]

On Fri, Jul 28, 2017 at 03:01:48AM +0200, Rafael Wysocki wrote:
> On Monday, July 24, 2017 06:14:36 PM Darren Hart wrote:
> > On Mon, Jul 24, 2017 at 01:41:06PM +0200, Rafael Wysocki wrote:
> > > On Monday, July 24, 2017 12:19:25 PM Andy Shevchenko wrote:
> > > > On Sat, Jul 22, 2017 at 2:16 AM, Darren Hart <dvhart@infradead.org> wrote:
> > > > > On Thu, Jul 20, 2017 at 08:56:48PM -0700, Alex Hung wrote:
> > > > >> Unsupported events is only useful for developers and does not meaningful
> > > > >> for users. Using dev_dbg makes more sense and reduces noise in kernel
> > > > >> messages.
> > > > 
> > > > >> -     dev_info(&device->dev, "unknown event index 0x%x\n", event);
> > > > >> +     dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> > > > >
> > > > > info is the most common log level for these events in the platform
> > > > > driver x86 subsystem per 'git grep -i "unknown event"'.
> > > > >
> > > > > My take on this is that we want these to be reported by users, rather
> > > > > than rely on developers to find them all - especially as the developers
> > > > > only see a fraction of the affected hardware.
> > > > >
> > > > > Are you finding these to be causing a problem / or producing really
> > > > > excessive log messages?
> > > > >
> > > > > Andy, what are your thoughts?
> > > > 
> > > > My opinion is slightly closer to Rafael's one.
> > > > 
> > > > I think this is a debugging context.
> > > > 
> > > > If we really care about reporting them we might go HID way, i.e. using
> > > > dev_printk(KERN_DEBUG ) with module parameter debug.
> > > > 
> > > > As developer I'm against that.
> > > > As a regular user I do not need to recompile a kernel, in case there
> > > > is no dynamic debug support, and it allows me to enable debug
> > > > messages.
> > > > 
> > > > P.S. To see how important message above is, do we have any statistics
> > > > how many bug reports / email we got wrt the issue and how many had
> > > > been addressed?
> > > 
> > > Well, there is one I'm aware of, but this particular one we aren't going to
> > > address at all, because apparently we handle the event in question anyway
> > > in a different way.
> > > 
> > > In any case, the only situation in which this information is useful at all is
> > > when some functionality is missing and you want to find out why.
> > > 
> > > Otherwise you get messages telling you that something *may* *be* missing,
> > > but as a user you have no idea what to do about that, because you don't
> > > even know how to report it and to whom (in case you want to report it).
> > 
> > My thinking was along the lines of the keymaps where we explicitly add
> > KE_IGNORE when it is a key we don't care about. In that case, it is
> > useful to have the messages because if that occurs we want to know so we
> > can update the driver.
> 
> Well, I'm not sure if adding every reported event to keymaps as KE_IGNORE
> is a good idea, because that would require somebody to figure out what the
> event is about every time and that's work which quite frankly is not very
> useful (the key is still ignored if it is "unknown").
> 
> This kind of is blacklisting which is always painful from the maintenance
> standpoint.

OK, yes, I've made similar arguments in other areas.

> 
> > This driver also has KE_IGNORE entries in the intel_vbtn_keymap.
> > 
> > Are we talking about unknown keys - or are we talking about something
> > else?
> 
> Unknown keys.
> 
> > If unknown keys, the additional messages will be minimal, and missing
> > keys are most likely to be reported if the message is obvious. Given the
> > sparse access to hardware by the developers, I prefer to have the
> > message.
> 
> Having it does not guarantee that it will be reported and even so, it is
> not particularly clear where to send those reports and who is going to act on
> them.

We get quite a few to bugzilla.kernel.org for this subsystem, certainly
more than we can handle. But, your point is taken.

> 
> > If there is more to what is going on here - can someone provide an
> > example of where this is causing a problem? Or where the event causing
> > the message is not something we will add code to catch / ignore?
> 
> The particular one that tirggered this is related to the switching between
> the "laptop" and "tablet" modes on Dell 9365 which according to Mario is
> handled already through the ISH driver.
> 
> We could potentially propagate this event to user space, but we have no
> idea how user space decides to handle it and we are not sure if this is going
> to be used consistently for this purpose on all platforms.

Yeah, this is an ongoing issue, and I don't see vendors converging on
how they handle this.

> Anyway, my opinion is that dev_dbg() messages are sufficient for such things
> as they allow people to selectively turn the messages on and see what happens
> if something seems to be missing, but otherwise they don't generate log noise.

I'm convinced. Thanks.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] platform/x86: intel-vbtn: reduce unnecessary messages for normal users

[Darren Hart]

On Thu, Jul 20, 2017 at 08:56:48PM -0700, Alex Hung wrote:
> Unsupported events is only useful for developers and does not meaningful
> for users. Using dev_dbg makes more sense and reduces noise in kernel
> messages.
> 
> Signed-off-by: Alex Hung <alex.hung@canonical.com>

Apologies for the delay Alex, both now queued to testing.

-- 
Darren Hart
VMware Open Source Technology Center