Re: [systemd-devel] [WIP PATCH 2/4] ACPI: button: remove the LID input node when the state is unknown

[Peter Hutterer]

On Tue, Jun 06, 2017 at 12:22:09PM +0200, Benjamin Tissoires wrote:
[...]
> > This is because the aml table puts many Notify(LID, 0x80) in various 
> > control methods.
> > And not one of them but multiple of them will be invoked by various OS 
> > drivers during suspend/resume period.
> > I think this is not an isolated platform that will invoke multiple 
> > redundant "Notify(lid)".
> > 
> > Fortunately, the lid state for the multiple notify(lid) should be same as 
> > the first "Notify(lid)".
> > I suppose this is why SW_LID is invented, as it can really filter such 
> > redundant events.
> > And user space finally can only see 1 "close" event.
> > 
> > But unconditionally prepending "open" before all "close" events surely can 
> > break the logic by
> > delivering multiple "close" events to the user space.
> 
> That doesn't matter. What matters is the state of the switch, not the
> event. So if user space receives (in case we marked the switch as not
> reliable) several close events, all user space will do is realize that
> the state is still closed and will act accordingly.

[...]

> Again, we don't care if the "event" comes from ACPI, the driver itself or
> user space (libinput). All that matters is the current state of the
> input node switch, that needs to match the physical world at any time.

as extra comment on those two: you cannot guarantee when e.g. libinput
checks the state. it may start up after the kernel has updated the EV_SW
state, it may close and re-open a device without notice (disabling a
device in X has that effect for example). So the EV_SW/SW_LID events are
nice to have, but we really rely on the *state* of the switch more than the
events.

Cheers,
   Peter

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] nfs.man: document incompatibility between "bg" option and systemd.

[NeilBrown]

On Tue, Jun 06 2017, Steve Dickson wrote:

> Hello,
>
> On 05/29/2017 06:19 PM, NeilBrown wrote:
>> 
>> Systemd does not, and will not, support "bg" correctly.
>> It has other, better, ways to handle "background" mounting.
> The only problem with this is bg mounts still work at least
> up to 4.11 kernel... 

I don't think this is correct.
In the default configuration, "mount -t nfs -o bg "
runs for longer than 90 seconds, so systemd kill it.

A working "bg" mount would successfully mount the filesystem is the
server came back after 5 minutes. If you use current systemd in the
default configuration, it won't.

bg mounts do work sometimes, but I don't think they are reliable, and
there seems to be no interest in changing this.
Maybe the text should say "Systemd does not, and will not, reliably
support "bg" mounts...".


>
> It appears there is a problem with a 4.12 kernel. The mount no 
> longer errors out with ECONNREFUSED it just hangs in the 
> kernel trying forever... It sounds like a bug to me but 
> maybe that change was intentional.. Anna?? Trond???

Might be related to my patch
  [PATCH] SUNRPC: ensure correct error is reported by xs_tcp_setup_socket()

sent 25th May.

>
> So I'm a bit hesitant to commit this since not accurate, yet.
>
> Finally, the whole idea of systemd randomly/silently 
> strip off mount options is crazy... IMHO... 

It isn't exactly systemd, it is systemd-fstab-generator.
The options lists in /etc/fstab are not all equal.  Some
are relevant to /bin/mount, some to mount.nfs, some to the kernel.
I think /bin/mount processes the option lists before passing it
to mount.nfs.  There is no intrinsic reason that systemd-fstab-generator
shouldn't do the same thing.

>
> Just because a concept that has been around for years
> does not fix well in the systemd world it gets
> rip out??? IDK... but I think we can do better than that.

I could suggest that automount is uniformly better than bg.  Give how
long automount has been around, and how easy it is to use with systemd,
it might be time to start encouraging people to stop using the inferior
technology.

I could say that, but I'm not 100% sure.  The difference between
automount and bg is that with bg it is easy to see if the mount has
succeeded yet - just look for an empty directory.  With automount,
you'll typically get a delay at that point.  We could possibly wind down
that delay...

>
> Note, the 'bg' is used by clients that do want their
> booting to hang by servers that are down so if the 
> option is rip out, boots will start hang. This
> will make it very difficult to debug since the bg
> will still exist in fstab.

Not exactly.
Current default behaviour is that systemd will wait 90 seconds, then
kill the mount program and fail the boot.  If we strip out "bg", the
same thing will happen.

I'm OK with the patch not being applied just yet.  I think we need to
resolve this issue, but it isn't 100% clear to me what the best
resolution would be.  So I'm happy to see a conversation happening and
opinions being shared.
I'd be particularly pleased if you could double check how "bg" is
currently handled on some systemd-enabled system that you use.
Does the mount program get killed like I see?  Does boot succeed if
there is a bg mount from an unresponsive server?

Thanks,
NeilBrown


>
> Again, the whole concept of systemd messing with mounts options
> is just not a good one... IMHO.. 
>
> steved.
>
>> 
>> Explain this.
>> 
>> See also https://github.com/systemd/systemd/issues/6046
>> 
>> Signed-off-by: NeilBrown 
>> ---
>>  utils/mount/nfs.man | 18 +-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/utils/mount/nfs.man b/utils/mount/nfs.man
>> index cc6e992ed807..7e76492d454f 100644
>> --- a/utils/mount/nfs.man
>> +++ b/utils/mount/nfs.man
>> @@ -372,6 +372,21 @@ Alternatively these issues can be addressed
>>  using an automounter (refer to
>>  .BR automount (8)
>>  for details).
>> +.IP
>> +When
>> +.B systemd
>> +is used to mount the filesystems listed in
>> +.IR /etc/fstab ,
>> +the
>> +.B bg
>> +option is not supported, and may be stripped from the option list.
>> +Similar functionality can be achieved by providing the
>> +.B x-system.automount
>> +option.  This will cause
>> +.B systemd
>> +to attempt to mount the filesystem when the mountpoint is first
>> +accessed, rather than during system boot.  The mount still happens in
>> +the "background", though in a different way.
>>  .TP 1.5i
>>  .BR rdirplus " / " nordirplus
>>  Selects whether to use NFS v3 or v4 READDIRPLUS requests.
>> @@ -1810,7 +1825,8 @@ such as security negotiation, server referrals, and 
>> named attributes.
>>  .BR rpc.idmapd (8),
>>  .BR rpc.gssd (8),
>>  .BR rpc.svcgssd (8),
>> -.BR kerberos (1)
>> +.BR kerberos (1),
>> +.BR systemd.mount (5) .
>>  .sp
>>  RFC 768 for the UDP specification.
>>  .br
>> 


signature.asc
Description: PGP signature
___

Re: [systemd-devel] [PATCH] nfs.man: document incompatibility between "bg" option and systemd.

[Michael Biebl]

2017-06-06 20:07 GMT+02:00 Steve Dickson :
> Finally, the whole idea of systemd randomly/silently
> strip off mount options is crazy... IMHO...

Personally, I would prefer if systemd simply logged a warning/error
message but would *not* strip the bg option.


-- 
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] About stable network interface names

[Andrei Borzenkov]

06.06.2017 16:20, Martin Wilck пишет:
> As others have remarked already, PCI bus-device-function is subject to
> change.
> 

Can device and function really change? My understanding is that device
part is determined by bus physical wiring and function by PCI card
design; this leaves bus as volatile run-time enumeration value.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] nfs.man: document incompatibility between "bg" option and systemd.

[Steve Dickson]

Hello,

On 05/29/2017 06:19 PM, NeilBrown wrote:
> 
> Systemd does not, and will not, support "bg" correctly.
> It has other, better, ways to handle "background" mounting.
The only problem with this is bg mounts still work at least
up to 4.11 kernel... 

It appears there is a problem with a 4.12 kernel. The mount no 
longer errors out with ECONNREFUSED it just hangs in the 
kernel trying forever... It sounds like a bug to me but 
maybe that change was intentional.. Anna?? Trond???

So I'm a bit hesitant to commit this since not accurate, yet.

Finally, the whole idea of systemd randomly/silently 
strip off mount options is crazy... IMHO... 

Just because a concept that has been around for years
does not fix well in the systemd world it gets
rip out??? IDK... but I think we can do better than that.

Note, the 'bg' is used by clients that do want their
booting to hang by servers that are down so if the 
option is rip out, boots will start hang. This
will make it very difficult to debug since the bg
will still exist in fstab.  

Again, the whole concept of systemd messing with mounts options
is just not a good one... IMHO.. 

steved.

> 
> Explain this.
> 
> See also https://github.com/systemd/systemd/issues/6046
> 
> Signed-off-by: NeilBrown 
> ---
>  utils/mount/nfs.man | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/utils/mount/nfs.man b/utils/mount/nfs.man
> index cc6e992ed807..7e76492d454f 100644
> --- a/utils/mount/nfs.man
> +++ b/utils/mount/nfs.man
> @@ -372,6 +372,21 @@ Alternatively these issues can be addressed
>  using an automounter (refer to
>  .BR automount (8)
>  for details).
> +.IP
> +When
> +.B systemd
> +is used to mount the filesystems listed in
> +.IR /etc/fstab ,
> +the
> +.B bg
> +option is not supported, and may be stripped from the option list.
> +Similar functionality can be achieved by providing the
> +.B x-system.automount
> +option.  This will cause
> +.B systemd
> +to attempt to mount the filesystem when the mountpoint is first
> +accessed, rather than during system boot.  The mount still happens in
> +the "background", though in a different way.
>  .TP 1.5i
>  .BR rdirplus " / " nordirplus
>  Selects whether to use NFS v3 or v4 READDIRPLUS requests.
> @@ -1810,7 +1825,8 @@ such as security negotiation, server referrals, and 
> named attributes.
>  .BR rpc.idmapd (8),
>  .BR rpc.gssd (8),
>  .BR rpc.svcgssd (8),
> -.BR kerberos (1)
> +.BR kerberos (1),
> +.BR systemd.mount (5) .
>  .sp
>  RFC 768 for the UDP specification.
>  .br
> 
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Systemd license vs. libcryptsetup license

[Zbigniew Jędrzejewski-Szmek]

On Tue, Jun 06, 2017 at 04:33:11PM +0200, Krzysztof Jackiewicz wrote:
> Hi,
> 
> I noticed that when systemd is configured with libcryptsetup option enabled
> it will link to libcryptsetup which is distributed under GPL 2.0. It seems
> like a license conflict to me. Can anyone explain it?

A license conflict appears when two pieces of software are combined and
the licenses have incompatible terms, so they cannot be both satisfied
at the same time. Systemd is LGPL which is compatible with GPL and pretty
much anything else. Also, we don't distribute the combined product, which
is the only thing LGPL and GPL put any conditions on.

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] Systemd license vs. libcryptsetup license

[Krzysztof Jackiewicz]

Hi,

I noticed that when systemd is configured with libcryptsetup option enabled
it will link to libcryptsetup which is distributed under GPL 2.0. It seems
like a license conflict to me. Can anyone explain it?

Thanks in advance.

Krzy

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] About stable network interface names

[Martin Wilck]

On Mon, 2017-05-29 at 02:35 +0200, Cesare Leonardi wrote:

> I ask because I've done several tests, with different motherboards, 
> adding and removing PCI-express cards and that expectation was not 
> satisfied in many cases.
> 
> For example, in one of those tests I initially had this setup:
> Integrated NIC: enp9s0
> PCIE1 (x1): dual port ethernet card [enp3s0, enp4s0]
> PCIE2 (x16): empty
> PCIE3 (x1): dual port ethernet card [enp7s0, enp8s0]
> 
> Then i inserted a SATA controller in the PCIE2 slot and three NICs
> got 
> renamed:
> Integrated NIC: enp10s0
> PCIE1 (x1): dual port ethernet card [enp3s0, enp4s0]
> PCIE2 (x16): empty
> PCIE3 (x1): dual port ethernet card [enp8s0, enp9s0]
> 
> Why?
> Didn't this interface naming scheme supposed to avoid this kind of
> renaming?
>  From what i've experimented network names are guaranteed to be
> stable 
> across reboots *and* if you doesn't add or remove hardware.

As others have remarked already, PCI bus-device-function is subject to
change.

Yet this highlights a problem with the current "predictable" network
device naming scheme. "biosdevname" uses information from various
sources, including ACPI _DSM method and SMBIOS type 41 and type 9. The
systemd device naming scheme (or the kernel code in pci-label.c, for
that matter) evaluates only _DSM and SMBIOS type 41, and not SMBIOS
type 9. The latter is necessary for mapping system PCI slot numbers to
bus-device-function tuples. Obviously, the physical slot a controller
is connected to is less likely to change than the bus-device-function
number, so exposing it might make a lot of sense.

All of this requires support on the BIOS/Firmware side - without that,
none of the "predictable" schemes work correctly.

Regards,
Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [WIP PATCH 2/4] ACPI: button: remove the LID input node when the state is unknown

[Benjamin Tissoires]

Hi Lv,

On Jun 05 2017 or thereabouts, Zheng, Lv wrote:
> Hi, Benjamin
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > Subject: [WIP PATCH 2/4] ACPI: button: remove the LID input node when the 
> > state is unknown
> > 
> > Because of the variation of firmware implementation, there is a chance
> > the LID state is unknown:
> > 1. Some platforms send "open" ACPI notification to the OS and the event
> >arrive before the button driver is resumed;
> > 2. Some platforms send "open" ACPI notification to the OS, but the event
> >arrives after the button driver is resumed, ex., Samsung N210+;
> > 3. Some platforms never send an "open" ACPI notification to the OS, but
> >update the cached _LID return value to "open", and this update arrives
> >before the button driver is resumed;
> > 4. Some platforms never send an "open" ACPI notification to the OS, but
> >update the cached _LID return value to "open", but this update arrives
> >after the button driver is resumed, ex., Surface Pro 3;
> > 5. Some platforms never send an "open" ACPI notification to the OS, and
> >_LID ACPI method returns a value which stays to "close", ex.,
> >Surface Pro 1.
> > 
> > We can mark the unreliable platform (cases 2, 4, 5 above) as such and make
> > sure we do not export an input node with an unknown state to prevent
> > suspend loops.
> > 
> > The database of unreliable devices is left to userspace to handle with
> > a hwdb file and a udev rule.
> > 
> > Note that this patch removes the filtering of duplicate events when
> > calling blocking_notifier_call_chain(), but this will be addressed in
> > a following patch.
> > 
> > Signed-off-by: Benjamin Tissoires 
> > ---
> >  drivers/acpi/button.c | 207 
> > --
> >  1 file changed, 131 insertions(+), 76 deletions(-)
> > 
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 48bcdca..9ad7604 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -25,6 +25,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -79,6 +80,8 @@ MODULE_DEVICE_TABLE(acpi, button_device_ids);
> >  static int acpi_button_add(struct acpi_device *device);
> >  static int acpi_button_remove(struct acpi_device *device);
> >  static void acpi_button_notify(struct acpi_device *device, u32 event);
> > +static int acpi_button_add_input(struct acpi_device *device);
> > +static int acpi_lid_update_reliable(struct acpi_device *device);
> > 
> >  #ifdef CONFIG_PM_SLEEP
> >  static int acpi_button_suspend(struct device *dev);
> > @@ -111,6 +114,8 @@ struct acpi_button {
> > bool suspended;
> >  };
> > 
> > +static DEFINE_MUTEX(button_input_lock);
> > +
> >  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> >  static struct acpi_device *lid_device;
> >  static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > @@ -119,6 +124,44 @@ static unsigned long lid_report_interval __read_mostly 
> > = 500;
> >  module_param(lid_report_interval, ulong, 0644);
> >  MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key 
> > events");
> > 
> > +static bool lid_reliable = true;
> > +
> > +static int param_set_lid_reliable(const char *val,
> > + const struct kernel_param *kp)
> > +{
> > +   bool prev_lid_reliable = lid_reliable;
> > +   int ret;
> > +
> > +   mutex_lock(_input_lock);
> > +
> > +   ret = param_set_bool(val, kp);
> > +   if (ret) {
> > +   mutex_unlock(_input_lock);
> > +   return ret;
> > +   }
> > +
> > +   /*
> > +* prevent a loop when we show up the device to userspace because
> > +* of an acpi notification, and userspace immediately removes it
> > +* by marking it as unreliable when this was already known.
> > +*/
> > +   if (lid_device && prev_lid_reliable != lid_reliable) {
> > +   ret = acpi_lid_update_reliable(lid_device);
> > +   if (ret)
> > +   lid_reliable = prev_lid_reliable;
> > +   }
> > +
> > +   mutex_unlock(_input_lock);
> > +   return ret;
> > +}
> > +
> > +static const struct kernel_param_ops lid_reliable_ops = {
> > +   .get = param_get_bool,
> > +   .set = param_set_lid_reliable,
> > +};
> > +module_param_cb(lid_reliable, _reliable_ops, _reliable, 0644);
> > +MODULE_PARM_DESC(lid_reliable, "Is the LID switch reliable (true|false)?");
> > +
> >  /* 
> > --
> >FS Interface (/proc)
> > 
> > -- 
> > */
> > @@ -142,79 +185,22 @@ static int acpi_lid_notify_state(struct acpi_device 
> > *device, int state)
> >  {
> > struct acpi_button *button = acpi_driver_data(device);
> > int ret;
> > -   ktime_t next_report;
> > -   bool do_update;
> > +
> > +   /* button_input_lock must be 

Re: [systemd-devel] [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events

[Benjamin Tissoires]

On Jun 05 2017 or thereabouts, Zheng, Lv wrote:
> Hi, Benjamin
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > Subject: [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events
> > 
> > The input stack already filters out the LID events. So instead of
> > filtering them out at the source, we can hook up after the input
> > processing and propagate the lid switch events when the input stack
> > tells us to.
> > 
> > An other benefit is that if userspace (think libinput) "fixes" the lid
> > switch state by some heuristics, this new state is forwarded to the
> > listeners in the kernel.
> 
> See my comments to PATCH 4.
> IMO, it sounds better that
> 1. ACPI lid works as a driver of SW_LID, and
> 2. i915 registers notification (the only user) via input layer.
> So it looks i915 rather than button driver should call 
> input_register_handler().
> And input layer may help to provide a simplified interface for drivers to 
> register key notifications.

Sounds good.

Dmitry, would a simplified API for other drivers to listen to input
events be something you would agree on?

Cheers,
Benjamin

> 
> Thanks and best regards
> Lv
> 
> > 
> > Signed-off-by: Benjamin Tissoires 
> > ---
> >  drivers/acpi/button.c | 156 
> > --
> >  1 file changed, 139 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 9ad7604..03e5981 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -109,8 +109,6 @@ struct acpi_button {
> > struct input_dev *input;
> > char phys[32];  /* for input device */
> > unsigned long pushed;
> > -   int last_state;
> > -   ktime_t last_time;
> > bool suspended;
> >  };
> > 
> > @@ -184,7 +182,6 @@ static int acpi_lid_evaluate_state(struct acpi_device 
> > *device)
> >  static int acpi_lid_notify_state(struct acpi_device *device, int state)
> >  {
> > struct acpi_button *button = acpi_driver_data(device);
> > -   int ret;
> > 
> > /* button_input_lock must be held */
> > 
> > @@ -205,20 +202,129 @@ static int acpi_lid_notify_state(struct acpi_device 
> > *device, int state)
> > if (state)
> > pm_wakeup_hard_event(>dev);
> > 
> > -   ret = blocking_notifier_call_chain(_lid_notifier, state, device);
> > -   if (ret == NOTIFY_DONE)
> > -   ret = blocking_notifier_call_chain(_lid_notifier, state,
> > -  device);
> > -   if (ret == NOTIFY_DONE || ret == NOTIFY_OK) {
> > -   /*
> > -* It is also regarded as success if the notifier_chain
> > -* returns NOTIFY_OK or NOTIFY_DONE.
> > -*/
> > -   ret = 0;
> > +   return 0;
> > +}
> > +
> > +/*
> > + * Pass incoming event to all connected clients.
> > + */
> > +static void acpi_button_lid_events(struct input_handle *handle,
> > +  const struct input_value *vals,
> > +  unsigned int count)
> > +{
> > +   const struct input_value *v;
> > +   int state = -1;
> > +   int ret;
> > +
> > +   for (v = vals; v != vals + count; v++) {
> > +   switch (v->type) {
> > +   case EV_SYN:
> > +   if (v->code == SYN_REPORT && state >= 0) {
> > +   ret = 
> > blocking_notifier_call_chain(_lid_notifier,
> > +   state,
> > +   lid_device);
> > +   if (ret == NOTIFY_DONE)
> > +   ret = 
> > blocking_notifier_call_chain(_lid_notifier,
> > +   state,
> > +   lid_device);
> > +   if (ret == NOTIFY_DONE || ret == NOTIFY_OK) {
> > +   /*
> > +* It is also regarded as success if
> > +* the notifier_chain returns NOTIFY_OK
> > +* or NOTIFY_DONE.
> > +*/
> > +   ret = 0;
> > +   }
> > +   }
> > +   break;
> > +   case EV_SW:
> > +   if (v->code == SW_LID)
> > +   state = !v->value;
> > +   break;
> > +   }
> > }
> > -   return ret;
> >  }
> > 
> > +static int acpi_button_lid_connect(struct input_handler *handler,
> > +  struct input_dev *dev,
> > +  const struct input_device_id *id)
> > +{
> > +   struct input_handle *handle;
> > +   int error;
> > +
> > +   handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
> > +   if (!handle)
> > +   

Re: [systemd-devel] [WIP PATCH 4/4] ACPI: button: Fix lid notification locks

[Benjamin Tissoires]

On Jun 05 2017 or thereabouts, Zheng, Lv wrote:
> Hi,
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > Subject: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks
> > 
> > From: Lv Zheng 
> > 
> > acpi/button.c now contains the logic to avoid frequently replayed events
> > which originally was ensured by using blocking notifier.
> > On the contrary, using a blocking notifier is wrong as it could keep on
> > returning NOTIFY_DONE, causing events lost.
> > 
> > This patch thus changes lid notification to raw notifier in order not to
> > have any events lost.
> 
> This patch is on top of the following:
> https://patchwork.kernel.org/patch/9756467/
> where button driver implements a frequency check and
> thus is capable of filtering redundant events itself:
> I saw you have deleted it from PATCH 02.
> So this patch is not applicable now.

I actually rebased it in this series. I kept your SoB line given that
the idea came from you and the resulting patch was rather similar (only
one hunk differs, but the meaning is the same).

> 
> Is input layer capable of filtering redundant events.

I don't think it does, and it should not. If an event is emitted, it has
to be forwarded. However, the logic of the protocol makes that the only
state that matters is when an EV_SYN is emitted. So if a SW_LID 0 then 1
is sent between the 2 EV_SYN, and the state was 1 before, from a
protocol point of view it's a no-op.

> I saw you unconditionally prepend "open" before "close",
> which may make input layer incapable of filtering redundant close events.

Again, we don't care about events. We care about states, and those are
only emitted when the lid is marked as non reliable.

> 
> If input layer is capable of filtering redundant events,
> why don't you:
> 1. drop this commit;
> 2. remove all ACPI lid notifier APIs;
> 3. change lid notifier callers to register notification via input layer?

Having the i915 driver listening to the input events is actually a good
solution. Let me think about it a little bit more and I'll come back.

Cheers,
Benjamin

> 
> Thanks and best regards
> Lv 
> 
> > 
> > Signed-off-by: Lv Zheng 
> > Signed-off-by: Benjamin Tissoires 
> > ---
> >  drivers/acpi/button.c | 68 
> > ---
> >  1 file changed, 27 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 03e5981..1927b08 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -114,7 +114,7 @@ struct acpi_button {
> > 
> >  static DEFINE_MUTEX(button_input_lock);
> > 
> > -static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> > +static RAW_NOTIFIER_HEAD(acpi_lid_notifier);
> >  static struct acpi_device *lid_device;
> >  static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > 
> > @@ -179,14 +179,12 @@ static int acpi_lid_evaluate_state(struct acpi_device 
> > *device)
> > return lid_state ? 1 : 0;
> >  }
> > 
> > -static int acpi_lid_notify_state(struct acpi_device *device, int state)
> > +static void acpi_lid_notify_state(struct acpi_device *device, int state)
> >  {
> > struct acpi_button *button = acpi_driver_data(device);
> > 
> > -   /* button_input_lock must be held */
> > -
> > if (!button->input)
> > -   return 0;
> > +   return;
> > 
> > /*
> >  * If the lid is unreliable, always send an "open" event before any
> > @@ -201,8 +199,6 @@ static int acpi_lid_notify_state(struct acpi_device 
> > *device, int state)
> > 
> > if (state)
> > pm_wakeup_hard_event(>dev);
> > -
> > -   return 0;
> >  }
> > 
> >  /*
> > @@ -214,28 +210,14 @@ static void acpi_button_lid_events(struct 
> > input_handle *handle,
> >  {
> > const struct input_value *v;
> > int state = -1;
> > -   int ret;
> > 
> > for (v = vals; v != vals + count; v++) {
> > switch (v->type) {
> > case EV_SYN:
> > -   if (v->code == SYN_REPORT && state >= 0) {
> > -   ret = 
> > blocking_notifier_call_chain(_lid_notifier,
> > +   if (v->code == SYN_REPORT && state >= 0)
> > +   
> > (void)raw_notifier_call_chain(_lid_notifier,
> > state,
> > lid_device);
> > -   if (ret == NOTIFY_DONE)
> > -   ret = 
> > blocking_notifier_call_chain(_lid_notifier,
> > -   state,
> > -   lid_device);
> > -   if (ret == NOTIFY_DONE || ret == NOTIFY_OK) {
> > -   /*
> > -* It is also regarded as success if
> > -*