Re: [PATCH 0/4 POC] Allow executing code and syscalls in another address space

[Benjamin Berg]

On Wed, 2021-04-14 at 09:34 +0200, Johannes Berg wrote:
> On Wed, 2021-04-14 at 08:22 +0100, Anton Ivanov wrote:
> > On 14/04/2021 06:52, Andrei Vagin wrote:
> > > We already have process_vm_readv and process_vm_writev to read and
> > > write
> > > to a process memory faster than we can do this with ptrace. And now
> > > it
> > > is time for process_vm_exec that allows executing code in an
> > > address
> > > space of another process. We can do this with ptrace but it is much
> > > slower.
> > > 
> > > = Use-cases =
> > > 
> > > Here are two known use-cases. The first one is “application kernel”
> > > sandboxes like User-mode Linux and gVisor. In this case, we have a
> > > process that runs the sandbox kernel and a set of stub processes
> > > that
> > > are used to manage guest address spaces. Guest code is executed in
> > > the
> > > context of stub processes but all system calls are intercepted and
> > > handled in the sandbox kernel. Right now, these sort of sandboxes
> > > use
> > > PTRACE_SYSEMU to trap system calls, but the process_vm_exec can
> > > significantly speed them up.
> > 
> > Certainly interesting, but will require um to rework most of its
> > memory 
> > management and we will most likely need extra mm support to make use
> > of 
> > it in UML. We are not likely to get away just with one syscall there.
> 
> Might help the seccomp mode though:
> 
> https://patchwork.ozlabs.org/project/linux-um/list/?series=231980

Hmm, to me it sounds like it replaces both ptrace and seccomp mode
while completely avoiding the scheduling overhead that these techniques
have. I think everything UML needs is covered:

 * The new API can do syscalls in the target memory space
   (we can modify the address space)
 * The new API can run code until the next syscall happens
   (or a signal happens, which means SIGALRM for scheduling works)
 * Single step tracing should work by setting EFLAGS

I think the memory management itself stays fundamentally the same. We
just do the initial clone() using CLONE_STOPPED. We don't need any stub
code/data and we have everything we need to modify the address space
and run the userspace process.

Benjamin


signature.asc
Description: This is a digitally signed message part

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Benjamin Berg]

Hi,

On Fri, 2020-11-06 at 11:47 +0100, Greg Kroah-Hartman wrote:
> Due to the lack of response, I guess they don't need to go to any
> stable kernel, so will queue them up for 5.11-rc1.

Sorry, forgot to reply.

Not including them in stable seems reasonable as I have not seen it
cause major trouble in the wild so far (i.e. only one user report that
seems related).

Benjamin


signature.asc
Description: This is a digitally signed message part

[PATCH 1/2] usb: typec: ucsi: acpi: Always decode connector change information

[Benjamin Berg]

From: Benjamin Berg 

Normal commands may be reporting that a connector has changed. Always
call the usci_connector_change handler and let it take care of
scheduling the work when needed.
Doing this makes the ACPI code path identical to the CCG one.

Cc: Hans de Goede 
Cc: Heikki Krogerus 
Signed-off-by: Benjamin Berg 
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c 
b/drivers/usb/typec/ucsi/ucsi_acpi.c
index fbfe8f5933af..04976435ad73 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -103,11 +103,12 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 
event, void *data)
if (ret)
return;
 
+   if (UCSI_CCI_CONNECTOR(cci))
+   ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
+
if (test_bit(COMMAND_PENDING, >flags) &&
cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))
complete(>complete);
-   else if (UCSI_CCI_CONNECTOR(cci))
-   ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
 }
 
 static int ucsi_acpi_probe(struct platform_device *pdev)
-- 
2.26.2

[PATCH 0/2] UCSI race condition resulting in wrong port state

[Benjamin Berg]

From: Benjamin Berg 

Hi all,

so, I kept running in an issue where the UCSI port information was saying
that power was being delivered (online: 1), while no cable was attached.

The core of the problem is that there are scenarios where UCSI change
notifications are lost. This happens because querying the changes that
happened is done using the GET_CONNECTOR_STATUS command while clearing the
bitfield happens from the separate ACK command. Any change in between will
be lost.

Note that the problem may be almost invisible in the UI as e.g. GNOME will
still show the battery as discharging. But some policies like automatic
suspend may be applied incorrectly.

Cc: Hans de Goede 
Cc: Heikki Krogerus 

Benjamin Berg (2):
  usb: typec: ucsi: acpi: Always decode connector change information
  usb: typec: ucsi: Work around PPM losing change information

 drivers/usb/typec/ucsi/ucsi.c  | 125 -
 drivers/usb/typec/ucsi/ucsi.h  |   2 +
 drivers/usb/typec/ucsi/ucsi_acpi.c |   5 +-
 3 files changed, 110 insertions(+), 22 deletions(-)

-- 
2.26.2

[PATCH 2/2] usb: typec: ucsi: Work around PPM losing change information

[Benjamin Berg]

From: Benjamin Berg 

Some/many PPMs are simply clearing the change bitfield when a
notification on a port is acknowledge. Unfortunately, doing so means
that any changes between the GET_CONNECTOR_STATUS and ACK_CC_CI commands
is simply lost.

Work around this by re-fetching the connector status afterwards. We can
then infer any changes that we see have happened but that may not be
respresented in the change bitfield.

We end up with the following actions:
 1. UCSI_GET_CONNECTOR_STATUS, store result, update unprocessed_changes
 2. UCSI_GET_CAM_SUPPORTED, discard result
 3. ACK connector change
 4. UCSI_GET_CONNECTOR_STATUS, store result
 5. Infere lost changes by comparing UCSI_GET_CONNECTOR_STATUS results
 6. If PPM reported a new change, then restart in order to ACK
 7. Process everything as usual.

The worker is also changed to re-schedule itself if a new change
notification happened while it was running.

Doing this fixes quite commonly occurring issues where e.g. the UCSI
power supply would remain online even thought the ThunderBolt cable was
unplugged.

Cc: Hans de Goede 
Cc: Heikki Krogerus 
Signed-off-by: Benjamin Berg 
---
 drivers/usb/typec/ucsi/ucsi.c | 125 --
 drivers/usb/typec/ucsi/ucsi.h |   2 +
 2 files changed, 107 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 758b988ac518..fad8680be7ab 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -53,7 +53,7 @@ static int ucsi_acknowledge_connector_change(struct ucsi 
*ucsi)
ctrl = UCSI_ACK_CC_CI;
ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
 
-   return ucsi->ops->async_write(ucsi, UCSI_CONTROL, , sizeof(ctrl));
+   return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, , sizeof(ctrl));
 }
 
 static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
@@ -625,21 +625,113 @@ static void ucsi_handle_connector_change(struct 
work_struct *work)
struct ucsi_connector *con = container_of(work, struct ucsi_connector,
  work);
struct ucsi *ucsi = con->ucsi;
+   struct ucsi_connector_status pre_ack_status;
+   struct ucsi_connector_status post_ack_status;
enum typec_role role;
+   u16 inferred_changes;
+   u16 changed_flags;
u64 command;
int ret;
 
mutex_lock(>lock);
 
+   /*
+* Some/many PPMs have an issue where all fields in the change bitfield
+* are cleared when an ACK is send. This will causes any change
+* between GET_CONNECTOR_STATUS and ACK to be lost.
+*
+* We work around this by re-fetching the connector status afterwards.
+* We then infer any changes that we see have happened but that may not
+* be represented in the change bitfield.
+*
+* Also, even though we don't need to know the currently supported alt
+* modes, we run the GET_CAM_SUPPORTED command to ensure the PPM does
+* not get stuck in case it assumes we do.
+* Always do this, rather than relying on UCSI_CONSTAT_CAM_CHANGE to be
+* set in the change bitfield.
+*
+* We end up with the following actions:
+*  1. UCSI_GET_CONNECTOR_STATUS, store result, update 
unprocessed_changes
+*  2. UCSI_GET_CAM_SUPPORTED, discard result
+*  3. ACK connector change
+*  4. UCSI_GET_CONNECTOR_STATUS, store result
+*  5. Infere lost changes by comparing UCSI_GET_CONNECTOR_STATUS 
results
+*  6. If PPM reported a new change, then restart in order to ACK
+*  7. Process everything as usual.
+*
+* We may end up seeing a change twice, but we can only miss extremely
+* short transitional changes.
+*/
+
+   /* 1. First UCSI_GET_CONNECTOR_STATUS */
+   command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
+   ret = ucsi_send_command(ucsi, command, _ack_status,
+   sizeof(pre_ack_status));
+   if (ret < 0) {
+   dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
+   __func__, ret);
+   goto out_unlock;
+   }
+   con->unprocessed_changes |= pre_ack_status.change;
+
+   /* 2. Run UCSI_GET_CAM_SUPPORTED and discard the result. */
+   command = UCSI_GET_CAM_SUPPORTED;
+   command |= UCSI_CONNECTOR_NUMBER(con->num);
+   ucsi_send_command(con->ucsi, command, NULL, 0);
+
+   /* 3. ACK connector change */
+   clear_bit(EVENT_PENDING, >flags);
+   ret = ucsi_acknowledge_connector_change(ucsi);
+   if (ret) {
+   dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
+   goto out_unlock;
+   }
+
+   /* 4. Second UCSI_GET_CONNECTOR_STATUS */
command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con

Re: [RFC] Documentation: Add documentation for new performance_profile sysfs class

[Benjamin Berg]

Hi,

seems reasonable to me. Quite simple, but likely good enough as we are
sticking to only use well known names.

Just found a small typo.

Benjamin

On Sat, 2020-10-03 at 15:19 +0200, Hans de Goede wrote:
> On modern systems CPU/GPU/... performance is often dynamically configurable
> in the form of e.g. variable clock-speeds and TPD. The performance is often
> automatically adjusted to the load by some automatic-mechanism (which may
> very well live outside the kernel).
> 
> These auto performance-adjustment mechanisms often can be configured with
> one of several performance-profiles, with either a bias towards low-power
> consumption (and cool and quiet) or towards performance (and higher power
> consumption and thermals).
> 
> Introduce a new performance_profile class/sysfs API which offers a generic
> API for selecting the performance-profile of these automatic-mechanisms.
> 
> Cc: Mark Pearson 
> Cc: Elia Devito 
> Cc: Bastien Nocera 
> Cc: Benjamin Berg 
> Cc: linux...@vger.kernel.org
> Cc: linux-a...@vger.kernel.org
> Signed-off-by: Hans de Goede 
> ---
>  .../testing/sysfs-class-performance_profile   | 104 ++
>  1 file changed, 104 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-performance_profile
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-performance_profile 
> b/Documentation/ABI/testing/sysfs-class-performance_profile
> new file mode 100644
> index ..9c67cae39600
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-performance_profile
> @@ -0,0 +1,104 @@
> +Performance-profile selection (e.g. 
> /sys/class/performance_profile/thinkpad_acpi/)
> +
> +On modern systems CPU/GPU/... performance is often dynamically configurable
> +in the form of e.g. variable clock-speeds and TPD. The performance is often
> +automatically adjusted to the load by some automatic-mechanism (which may
> +very well live outside the kernel).
> +
> +These auto performance-adjustment mechanisms often can be configured with
> +one of several performance-profiles, with either a bias towards low-power
> +consumption (and cool and quiet) or towards performance (and higher power
> +consumption and thermals).
> +
> +The purpose of the performance_profile class is to offer a generic sysfs
> +API for selecting the performance-profile of these automatic-mechanisms.
> +
> +Note that this API is only for selecting the performance-profile, it is
> +NOT a goal of this API to allow monitoring the resulting performance
> +characteristics. Monitoring performance is best done with device/vendor
> +specific tools such as e.g. turbostat.
> +
> +Specifically when selecting a high-performance profile the actual achieved
> +performance may be limited by various factors such as: the heat generated by
> +other components, room temperature, free air flow at the bottom of a laptop,
> +etc. It is explicitly NOT a goal of this API to let userspace know about
> +any sub-optimal conditions which are impeding reaching the requested
> +performance level.
> +
> +Since numbers are a rather meaningless way to describe performance-profiles
> +this API uses strings to describe the various profiles. To make sure that
> +userspace gets a consistent experience when using this API this API document
> +defines a fixed set of profile-names. Drivers *must* map their internal
> +profile representation/names onto this fixed set.
> +
> +If for some reason there is no good match when mapping then a new 
> profile-name
> +may be added. Drivers which wish to introduce new profile-names must:
> +1. Have very good reasons to do so.
> +2. Add the new profile-name to this document, so that future drivers which 
> also
> +   have a similar problem can use the same new. Usually new profile-names 
> will

Typo, "new" -> "name" I suppose.

> +   be added to the "extra profile-names" section of this document. But in 
> some
> +   cases the set of standard profile-names may be extended.
> +
> +What:
> /sys/class/performance_profile//available_profiles
> +Date:October 2020
> +Contact: Hans de Goede 
> +Description:
> + Reading this file gives a space separated list of profiles
> + supported for this device.
> +
> + Drivers must use the following standard profile-names whenever
> + possible:
> +
> + low-power:  Emphasises low power consumption
> + (and also cool and quiet)
> + balanced-low-power: Balances between low power consumption
> + and performance with a slight bias
> +   

[tip: ras/core] x86/mce: Lower throttling MCE messages' priority to warning

[tip-bot2 for Benjamin Berg]

The following commit has been merged into the ras/core branch of tip:

Commit-ID: 9c3bafaa1fd88e4dd2dba3735a1f1abb0f2c7bb7
Gitweb:
https://git.kernel.org/tip/9c3bafaa1fd88e4dd2dba3735a1f1abb0f2c7bb7
Author:Benjamin Berg 
AuthorDate:Wed, 09 Oct 2019 17:54:24 +02:00
Committer: Borislav Petkov 
CommitterDate: Thu, 17 Oct 2019 09:07:09 +02:00

x86/mce: Lower throttling MCE messages' priority to warning

On modern CPUs it is quite normal that the temperature limits are
reached and the CPU is throttled. In fact, often the thermal design is
not sufficient to cool the CPU at full load and limits can quickly be
reached when a burst in load happens. This will even happen with
technologies like RAPL limitting the long term power consumption of
the package.

Also, these limits are "softer", as Srinivas explains:

"CPU temperature doesn't have to hit max(TjMax) to get these warnings.
OEMs ha[ve] an ability to program a threshold where a thermal interrupt
can be generated. In some systems the offset is 20C+ (Read only value).

In recent systems, there is another offset on top of it which can be
programmed by OS, once some agent can adjust power limits dynamically.
By default this is set to low by the firmware, which I guess the
prime motivation of Benjamin to submit the patch."

So these messages do not usually indicate a hardware issue (e.g.
insufficient cooling). Log them as warnings to avoid confusion about
their severity.

 [ bp: Massage commit mesage. ]

Signed-off-by: Benjamin Berg 
Signed-off-by: Borislav Petkov 
Reviewed-by: Hans de Goede 
Tested-by: Christian Kellner 
Cc: "H. Peter Anvin" 
Cc: Ingo Molnar 
Cc: linux-edac 
Cc: Peter Zijlstra 
Cc: Srinivas Pandruvada 
Cc: Thomas Gleixner 
Cc: Tony Luck 
Cc: x86-ml 
Link: https://lkml.kernel.org/r/20191009155424.249277-1-bb...@redhat.com
---
 arch/x86/kernel/cpu/mce/therm_throt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mce/therm_throt.c 
b/arch/x86/kernel/cpu/mce/therm_throt.c
index 6e2becf..bc441d6 100644
--- a/arch/x86/kernel/cpu/mce/therm_throt.c
+++ b/arch/x86/kernel/cpu/mce/therm_throt.c
@@ -188,7 +188,7 @@ static void therm_throt_process(bool new_event, int event, 
int level)
/* if we just entered the thermal event */
if (new_event) {
if (event == THERMAL_THROTTLING_EVENT)
-   pr_crit("CPU%d: %s temperature above threshold, cpu 
clock throttled (total events = %lu)\n",
+   pr_warn("CPU%d: %s temperature above threshold, cpu 
clock throttled (total events = %lu)\n",
this_cpu,
level == CORE_LEVEL ? "Core" : "Package",
state->count);

Re: [PATCH] x86/mce: Lower throttling MCE messages to warnings

[Benjamin Berg]

Hi Srinivas,

On Thu, 2019-10-10 at 14:08 -0700, Srinivas Pandruvada wrote:
> I have a patch to address this. Instead of avoiding any critical
> warnings or wait for 300 seconds for next one, the warning is based on
> how long the system is working on throttled condition. If for example
> the fan broke, then the throttling is extended for a long time. Then we
> better warn.
> I am waiting for internal review, and hope to post by tomorrow.

Nice! I agree that a heuristic seems better than the very simple
approach taken in this patch.

Thanks,
Benjamin

> Thanks
> Srinivas
> 
> > > Signed-off-by: Benjamin Berg 
> > > Tested-by: Christian Kellner 
> > > ---
> > >  arch/x86/kernel/cpu/mce/therm_throt.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kernel/cpu/mce/therm_throt.c
> > > b/arch/x86/kernel/cpu/mce/therm_throt.c
> > > index 6e2becf547c5..bc441d68d060 100644
> > > --- a/arch/x86/kernel/cpu/mce/therm_throt.c
> > > +++ b/arch/x86/kernel/cpu/mce/therm_throt.c
> > > @@ -188,7 +188,7 @@ static void therm_throt_process(bool
> > > new_event,
> > > int event, int level)
> > >   /* if we just entered the thermal event */
> > >   if (new_event) {
> > >   if (event == THERMAL_THROTTLING_EVENT)
> > > - pr_crit("CPU%d: %s temperature above threshold,
> > > cpu clock throttled (total events = %lu)\n",
> > > + pr_warn("CPU%d: %s temperature above threshold,
> > > cpu clock throttled (total events = %lu)\n",
> > >   this_cpu,
> > >   level == CORE_LEVEL ? "Core" :
> > > "Package",
> > >   state->count);
> > > -- 
> > 
> > This has carried over since its very first addition in
> > 
> > commit 3867eb75b9279c7b0f6840d2ad9f27694ba6c4e4
> > Author: Dave Jones 
> > Date:   Tue Apr 2 20:02:27 2002 -0800
> > 
> > [PATCH] x86 bluesmoke update.
> > 
> > o  Make MCE compile time optional   (Paul Gortmaker)
> > o  P4 thermal trip monitoring.  (Zwane Mwaikambo)
> > o  Non-fatal MCE logging.   (Me)
> > 
> > 
> > It used to be KERN_EMERG back then, though.
> > 
> > And yes, this issue has come up in the past already so I think I'll
> > take
> > it. I'll just give Intel folks a couple of days to object should
> > there
> > be anything to object to.
> > 
> > Thx.
> > 

[PATCH] x86/mce: Lower throttling MCE messages to warnings

[Benjamin Berg]

On modern CPUs it is quite normal that the temperature limits are
reached and the CPU is throttled. In fact, often the thermal design is
not sufficient to cool the CPU at full load and limits can quickly be
reached when a burst in load happens. This will even happen with
technologies like RAPL limitting the long term power consumption of
the package.

So these messages do not usually indicate a hardware issue (e.g.
insufficient cooling). Log them as warnings to avoid confusion about
their severity.

Signed-off-by: Benjamin Berg 
Tested-by: Christian Kellner 
---
 arch/x86/kernel/cpu/mce/therm_throt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mce/therm_throt.c 
b/arch/x86/kernel/cpu/mce/therm_throt.c
index 6e2becf547c5..bc441d68d060 100644
--- a/arch/x86/kernel/cpu/mce/therm_throt.c
+++ b/arch/x86/kernel/cpu/mce/therm_throt.c
@@ -188,7 +188,7 @@ static void therm_throt_process(bool new_event, int event, 
int level)
/* if we just entered the thermal event */
if (new_event) {
if (event == THERMAL_THROTTLING_EVENT)
-   pr_crit("CPU%d: %s temperature above threshold, cpu 
clock throttled (total events = %lu)\n",
+   pr_warn("CPU%d: %s temperature above threshold, cpu 
clock throttled (total events = %lu)\n",
this_cpu,
level == CORE_LEVEL ? "Core" : "Package",
state->count);
-- 
2.23.0

Re: [PATCH] drivers: thermal: processor_thermal_device: Export sysfs inteface for TCC offset

[Benjamin Berg]

Tested-by: Benjamin Berg 

Hi,

this patch allows performance improvements of some machines. It would
be nice if this could still make 5.3.

Benjamin

On Mon, 2019-07-22 at 18:03 -0700, Srinivas Pandruvada wrote:
> This change exports an interface to read tcc offset and allow writing if
> the platform is not locked.
> 
> Refer to Intel SDM for details on the MSR: MSR_TEMPERATURE_TARGET.
> Here TCC Activation Offset (R/W) bits allow temperature offset in degrees
> in relation to TjMAX.
> 
> This change will be useful for improving performance from user space for
> some platforms, if the current offset is not optimal.
> 
> Signed-off-by: Srinivas Pandruvada 
> ---
>  .../processor_thermal_device.c| 91
> ++-
>  1 file changed, 87 insertions(+), 4 deletions(-)
> 
> diff --git
> a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> index 213ab3cc6b80..a35635129fed 100644
> ---
> a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> +++
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> @@ -137,6 +137,72 @@ static const struct attribute_group
> power_limit_attribute_group = {
>   .name = "power_limits"
>  };
>  
> +static ssize_t tcc_offset_degree_celsius_show(struct device *dev,
> +struct device_attribute *attr, char
> *buf)
> +{
> + u64 val;
> + int err;
> +
> + err = rdmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, );
> + if (err)
> + return err;
> +
> + val = (val >> 24) & 0xff;
> + return sprintf(buf, "%d\n", (int)val);
> +}
> +
> +static int tcc_offset_update(int tcc)
> +{
> + u64 val;
> + int err;
> +
> + if (!tcc)
> + return -EINVAL;
> +
> + err = rdmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, );
> + if (err)
> + return err;
> +
> + val = ~GENMASK_ULL(31, 24);
> + val = (tcc & 0xff) << 24;
> +
> + err = wrmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, val);
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int tcc_offset_save;
> +
> +static ssize_t tcc_offset_degree_celsius_store(struct device *dev,
> + struct device_attribute *attr, const
> char *buf,
> + size_t count)
> +{
> + u64 val;
> + int tcc, err;
> +
> + err = rdmsrl_safe(MSR_PLATFORM_INFO, );
> + if (err)
> + return err;
> +
> + if (!(val & BIT(30)))
> + return -EACCES;
> +
> + if (kstrtoint(buf, 0, ))
> + return -EINVAL;
> +
> + err = tcc_offset_update(tcc);
> + if (err)
> + return err;
> +
> + tcc_offset_save = tcc;
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(tcc_offset_degree_celsius);
> +
>  static int stored_tjmax; /* since it is fixed, we can have local
> storage */
>  
>  static int get_tjmax(void)
> @@ -332,6 +398,7 @@ static void proc_thermal_remove(struct
> proc_thermal_device *proc_priv)
>   acpi_remove_notify_handler(proc_priv->adev->handle,
>  ACPI_DEVICE_NOTIFY,
> proc_thermal_notify);
>   int340x_thermal_zone_remove(proc_priv->int340x_zone);
> + sysfs_remove_file(_priv->dev->kobj,
> _attr_tcc_offset_degree_celsius.attr);
>   sysfs_remove_group(_priv->dev->kobj,
>  _limit_attribute_group);
>  }
> @@ -355,8 +422,15 @@ static int int3401_add(struct platform_device
> *pdev)
>  
>   dev_info(>dev, "Creating sysfs group for
> PROC_THERMAL_PLATFORM_DEV\n");
>  
> - return sysfs_create_group(>dev.kobj,
> -  _limit_attribute_group);
> + ret = sysfs_create_file(>dev.kobj,
> _attr_tcc_offset_degree_celsius.attr);
> + if (ret)
> + return ret;
> +
> + ret = sysfs_create_group(>dev.kobj,
> _limit_attribute_group);
> + if (ret)
> + sysfs_remove_file(>dev.kobj,
> _attr_tcc_offset_degree_celsius.attr);
> +
> + return ret;
>  }
>  
>  static int int3401_remove(struct platform_device *pdev)
> @@ -584,8 +658,15 @@ static int  proc_thermal_pci_probe(struct
> pci_dev *pdev,
>  
>   dev_info(>dev, "Creating sysfs group for
> PROC_THERMAL_PCI\n");
>  
> - return sysfs_create_group(>dev.kobj,
> -  _limit_attribute_group);
> + ret = sysfs_create_file(>dev.kobj,
> _attr_tcc_offset

Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

[Benjamin Berg]

Hi,

On Wed, 2018-06-06 at 10:50 +0800, Chris Chiu wrote:
> On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede 
> wrote:
> > Hi,
> > 
> > 
> > On 05-06-18 12:46, Benjamin Berg wrote:
> > > 
> > > Hey,
> > > 
> > > On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
> > > > 
> > > > On 05-06-18 12:14, Bastien Nocera wrote:
> > > > > 
> > > > > On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
> > > > > > 
> > > > > > On 05-06-18 11:58, Bastien Nocera wrote:
> > > > > > > 
> > > > > > > [SNIP]
> > > > > > 
> > > > > > 
> > > > > > Ok, so what are you suggestion, do you really want to
> > > > > > hardcode
> > > > > > the cycle behavior in the kernel as these 2 patches are
> > > > > > doing,
> > > > > > without any option to intervene from userspace?
> > > > > > 
> > > > > > As mentioned before in the thread there are several example
> > > > > > of the kernel deciding to handle key-presses itself,
> > > > > > putting
> > > > > > policy in the kernel and they have all ended poorly (think
> > > > > > e.g. rfkill, acpi-video dealing with LC brightnesskey
> > > > > > presses
> > > > > > itself).
> > > > > > 
> > > > > > I guess one thing we could do here is code out both
> > > > > > solutions,
> > > > > > have a module option which controls if we:
> > > > > > 
> > > > > > 1) Handle this in the kernel as these patches do
> > > > > > 2) Or send a new KEY_KBDILLUMCYCLE event
> > > > > > 
> > > > > > Combined with a Kconfig option to select which is the
> > > > > > default
> > > > > > behavior. Then Endless can select 1 for now and then in
> > > > > > Fedora (which defaults to Wayland now) we could default to
> > > > > > 2. once all the code for handling 2 is in place.
> > > > > > 
> > > > > > This is ugly (on the kernel side) but it might be the best
> > > > > > compromise we can do.
> > > > > 
> > > > > 
> > > > > I don't really mind which option is used, I'm listing the
> > > > > problems with
> > > > > the different options. If you don't care about Xorg, then
> > > > > definitely go
> > > > > for adding a new key. Otherwise, processing it in the kernel
> > > > > is the
> > > > > least ugly, especially given that the key goes through the
> > > > > same driver
> > > > > that controls the brightness anyway. There's no crazy cross
> > > > > driver
> > > > > interaction as there was in the other cases you listed.
> > > > 
> > > > 
> > > > Unfortunately not caring about Xorg is not really an option.
> > > > 
> > > > Ok, new idea, how about we make g-s-d behavior upon detecting a
> > > > KEY_KBDILLUMTOGGLE event configurable, if we're on a Mac do a
> > > > toggle, otherwise do a cycle.
> > > > 
> > > > Or we could do this through hwdb, then we could add a hwdb entry
> > > > for this laptop setting the udev property to do a cycle instead of
> > > > a toggle on receiving the keypress.
> > > 
> > > If we are adding hwdb entries anyway to control the userspace
> > > interpretation of the TOGGLE key, then we could also add the new CYCLE
> > > key and explicitly re-map it to TOGGLE. That requires slightly more
> > > logic in hwdb, but it does mean that we could theoretically just drop
> > > the workaround if we ever stop caring about Xorg.
> > 
> > Hmm, interesting proposal, I say go for it :)
> > 
> 
> So maybe the next stop is that I can follow Darren's suggestion to eliminate
> the is_kbd_led_event() and send a v2 for review?

I believe the best compromise we have right now is to do what Hans
suggested in an earlier proposal. That is implementing the two separate
behaviours in the kernel

 1) handle this in the kernel as if the hardware changed it, and
 2) send a new KEY_KBDILLUMCYCLE event [default].

Which one is used would be a compile time option for the kernel.

Then we have three different choices for handling these devices from a
userspace/distribution point of view:
 1. Let the kernel handle these devices (quick fix)
 2. Assume we are on wayland and handle KEY_KBDILLUMCYCLE
(great if Xorg support is not a requirement)
 3. For Xorg support:
- Add hwdb entry
- remap key to KEY_KBDILLUMTOGGLE
- set a flag on the keyboard
- detect the flag in userspace and handle KEY_KBDILLUMTOGGLE
  as if KEY_KBDILLUMCYCLE was pressed
(yep, quite ugly)

The "beauty" of this approach is that the workaround from option 3 can
be simply removed again if we stop caring about Xorg (or should we find
a solution to handle high keycodes in Xorg).

Benjamin

signature.asc
Description: This is a digitally signed message part

Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

[Benjamin Berg]

Hi,

On Wed, 2018-06-06 at 10:50 +0800, Chris Chiu wrote:
> On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede 
> wrote:
> > Hi,
> > 
> > 
> > On 05-06-18 12:46, Benjamin Berg wrote:
> > > 
> > > Hey,
> > > 
> > > On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
> > > > 
> > > > On 05-06-18 12:14, Bastien Nocera wrote:
> > > > > 
> > > > > On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
> > > > > > 
> > > > > > On 05-06-18 11:58, Bastien Nocera wrote:
> > > > > > > 
> > > > > > > [SNIP]
> > > > > > 
> > > > > > 
> > > > > > Ok, so what are you suggestion, do you really want to
> > > > > > hardcode
> > > > > > the cycle behavior in the kernel as these 2 patches are
> > > > > > doing,
> > > > > > without any option to intervene from userspace?
> > > > > > 
> > > > > > As mentioned before in the thread there are several example
> > > > > > of the kernel deciding to handle key-presses itself,
> > > > > > putting
> > > > > > policy in the kernel and they have all ended poorly (think
> > > > > > e.g. rfkill, acpi-video dealing with LC brightnesskey
> > > > > > presses
> > > > > > itself).
> > > > > > 
> > > > > > I guess one thing we could do here is code out both
> > > > > > solutions,
> > > > > > have a module option which controls if we:
> > > > > > 
> > > > > > 1) Handle this in the kernel as these patches do
> > > > > > 2) Or send a new KEY_KBDILLUMCYCLE event
> > > > > > 
> > > > > > Combined with a Kconfig option to select which is the
> > > > > > default
> > > > > > behavior. Then Endless can select 1 for now and then in
> > > > > > Fedora (which defaults to Wayland now) we could default to
> > > > > > 2. once all the code for handling 2 is in place.
> > > > > > 
> > > > > > This is ugly (on the kernel side) but it might be the best
> > > > > > compromise we can do.
> > > > > 
> > > > > 
> > > > > I don't really mind which option is used, I'm listing the
> > > > > problems with
> > > > > the different options. If you don't care about Xorg, then
> > > > > definitely go
> > > > > for adding a new key. Otherwise, processing it in the kernel
> > > > > is the
> > > > > least ugly, especially given that the key goes through the
> > > > > same driver
> > > > > that controls the brightness anyway. There's no crazy cross
> > > > > driver
> > > > > interaction as there was in the other cases you listed.
> > > > 
> > > > 
> > > > Unfortunately not caring about Xorg is not really an option.
> > > > 
> > > > Ok, new idea, how about we make g-s-d behavior upon detecting a
> > > > KEY_KBDILLUMTOGGLE event configurable, if we're on a Mac do a
> > > > toggle, otherwise do a cycle.
> > > > 
> > > > Or we could do this through hwdb, then we could add a hwdb entry
> > > > for this laptop setting the udev property to do a cycle instead of
> > > > a toggle on receiving the keypress.
> > > 
> > > If we are adding hwdb entries anyway to control the userspace
> > > interpretation of the TOGGLE key, then we could also add the new CYCLE
> > > key and explicitly re-map it to TOGGLE. That requires slightly more
> > > logic in hwdb, but it does mean that we could theoretically just drop
> > > the workaround if we ever stop caring about Xorg.
> > 
> > Hmm, interesting proposal, I say go for it :)
> > 
> 
> So maybe the next stop is that I can follow Darren's suggestion to eliminate
> the is_kbd_led_event() and send a v2 for review?

I believe the best compromise we have right now is to do what Hans
suggested in an earlier proposal. That is implementing the two separate
behaviours in the kernel

 1) handle this in the kernel as if the hardware changed it, and
 2) send a new KEY_KBDILLUMCYCLE event [default].

Which one is used would be a compile time option for the kernel.

Then we have three different choices for handling these devices from a
userspace/distribution point of view:
 1. Let the kernel handle these devices (quick fix)
 2. Assume we are on wayland and handle KEY_KBDILLUMCYCLE
(great if Xorg support is not a requirement)
 3. For Xorg support:
- Add hwdb entry
- remap key to KEY_KBDILLUMTOGGLE
- set a flag on the keyboard
- detect the flag in userspace and handle KEY_KBDILLUMTOGGLE
  as if KEY_KBDILLUMCYCLE was pressed
(yep, quite ugly)

The "beauty" of this approach is that the workaround from option 3 can
be simply removed again if we stop caring about Xorg (or should we find
a solution to handle high keycodes in Xorg).

Benjamin

signature.asc
Description: This is a digitally signed message part

Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

[Benjamin Berg]

Hey,

On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
> On 05-06-18 12:14, Bastien Nocera wrote:
> > On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
> > > On 05-06-18 11:58, Bastien Nocera wrote:
> > > > [SNIP]
> > > 
> > > Ok, so what are you suggestion, do you really want to hardcode
> > > the cycle behavior in the kernel as these 2 patches are doing,
> > > without any option to intervene from userspace?
> > > 
> > > As mentioned before in the thread there are several example
> > > of the kernel deciding to handle key-presses itself, putting
> > > policy in the kernel and they have all ended poorly (think
> > > e.g. rfkill, acpi-video dealing with LC brightnesskey presses
> > > itself).
> > > 
> > > I guess one thing we could do here is code out both solutions,
> > > have a module option which controls if we:
> > > 
> > > 1) Handle this in the kernel as these patches do
> > > 2) Or send a new KEY_KBDILLUMCYCLE event
> > > 
> > > Combined with a Kconfig option to select which is the default
> > > behavior. Then Endless can select 1 for now and then in
> > > Fedora (which defaults to Wayland now) we could default to
> > > 2. once all the code for handling 2 is in place.
> > > 
> > > This is ugly (on the kernel side) but it might be the best
> > > compromise we can do.
> > 
> > I don't really mind which option is used, I'm listing the problems with
> > the different options. If you don't care about Xorg, then definitely go
> > for adding a new key. Otherwise, processing it in the kernel is the
> > least ugly, especially given that the key goes through the same driver
> > that controls the brightness anyway. There's no crazy cross driver
> > interaction as there was in the other cases you listed.
> 
> Unfortunately not caring about Xorg is not really an option.
> 
> Ok, new idea, how about we make g-s-d behavior upon detecting a
> KEY_KBDILLUMTOGGLE event configurable, if we're on a Mac do a
> toggle, otherwise do a cycle.
>
> Or we could do this through hwdb, then we could add a hwdb entry
> for this laptop setting the udev property to do a cycle instead of
> a toggle on receiving the keypress.

If we are adding hwdb entries anyway to control the userspace
interpretation of the TOGGLE key, then we could also add the new CYCLE
key and explicitly re-map it to TOGGLE. That requires slightly more
logic in hwdb, but it does mean that we could theoretically just drop
the workaround if we ever stop caring about Xorg.

> I guess alternatively I could live with hardcoding this in the
> kernel as these 2 patches do, but that solves it just for *this*
> laptop, I've a feeling that if we do that we end up with similar
> code in all laptop vendor drivers under drivers/platform/x86
> soon.  Which really is the acpi_video.brightness_event thing
> again, where the kernel would handle brightness key-presses
> but only if the acpi_video backlight interface was in use
> and not on models with a vendor specific or native-hardware
> backlight driver.  Hmm, so writing this, I'm still quite sure
> the kernel approach is actually a bad idea.

Benjamin

signature.asc
Description: This is a digitally signed message part

Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

[Benjamin Berg]

Hey,

On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
> On 05-06-18 12:14, Bastien Nocera wrote:
> > On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
> > > On 05-06-18 11:58, Bastien Nocera wrote:
> > > > [SNIP]
> > > 
> > > Ok, so what are you suggestion, do you really want to hardcode
> > > the cycle behavior in the kernel as these 2 patches are doing,
> > > without any option to intervene from userspace?
> > > 
> > > As mentioned before in the thread there are several example
> > > of the kernel deciding to handle key-presses itself, putting
> > > policy in the kernel and they have all ended poorly (think
> > > e.g. rfkill, acpi-video dealing with LC brightnesskey presses
> > > itself).
> > > 
> > > I guess one thing we could do here is code out both solutions,
> > > have a module option which controls if we:
> > > 
> > > 1) Handle this in the kernel as these patches do
> > > 2) Or send a new KEY_KBDILLUMCYCLE event
> > > 
> > > Combined with a Kconfig option to select which is the default
> > > behavior. Then Endless can select 1 for now and then in
> > > Fedora (which defaults to Wayland now) we could default to
> > > 2. once all the code for handling 2 is in place.
> > > 
> > > This is ugly (on the kernel side) but it might be the best
> > > compromise we can do.
> > 
> > I don't really mind which option is used, I'm listing the problems with
> > the different options. If you don't care about Xorg, then definitely go
> > for adding a new key. Otherwise, processing it in the kernel is the
> > least ugly, especially given that the key goes through the same driver
> > that controls the brightness anyway. There's no crazy cross driver
> > interaction as there was in the other cases you listed.
> 
> Unfortunately not caring about Xorg is not really an option.
> 
> Ok, new idea, how about we make g-s-d behavior upon detecting a
> KEY_KBDILLUMTOGGLE event configurable, if we're on a Mac do a
> toggle, otherwise do a cycle.
>
> Or we could do this through hwdb, then we could add a hwdb entry
> for this laptop setting the udev property to do a cycle instead of
> a toggle on receiving the keypress.

If we are adding hwdb entries anyway to control the userspace
interpretation of the TOGGLE key, then we could also add the new CYCLE
key and explicitly re-map it to TOGGLE. That requires slightly more
logic in hwdb, but it does mean that we could theoretically just drop
the workaround if we ever stop caring about Xorg.

> I guess alternatively I could live with hardcoding this in the
> kernel as these 2 patches do, but that solves it just for *this*
> laptop, I've a feeling that if we do that we end up with similar
> code in all laptop vendor drivers under drivers/platform/x86
> soon.  Which really is the acpi_video.brightness_event thing
> again, where the kernel would handle brightness key-presses
> but only if the acpi_video backlight interface was in use
> and not on models with a vendor specific or native-hardware
> backlight driver.  Hmm, so writing this, I'm still quite sure
> the kernel approach is actually a bad idea.

Benjamin

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Benjamin Berg]

Hi,

On Thu, 2018-05-24 at 16:33 +0800, Chris Chiu wrote:
> I've made my change to set the brightness level directly in the
> driver, but the
> OSD doesn't show correctly correspond to the level value. The brightness shows
> OK in /sys/class/led//brighness but the OSD always shows level 0. I 
> thought
> GNOME should read the brightness from /sys before showing OSD?

Sorry for the late response.

There is a special mechanism to report that the HW changed the
brightness. This works using the "brightness_hw_changed" sysfs
attribute. So you will need to set the LED_BRIGHT_HW_CHANGED flag on
the LED and then call led_classdev_notify_brightness_hw_changed to make
it work.

Userspace should correctly show the OSD when this is done.

Benjamin

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Benjamin Berg]

Hi,

On Thu, 2018-05-24 at 16:33 +0800, Chris Chiu wrote:
> I've made my change to set the brightness level directly in the
> driver, but the
> OSD doesn't show correctly correspond to the level value. The brightness shows
> OK in /sys/class/led//brighness but the OSD always shows level 0. I 
> thought
> GNOME should read the brightness from /sys before showing OSD?

Sorry for the late response.

There is a special mechanism to report that the HW changed the
brightness. This works using the "brightness_hw_changed" sysfs
attribute. So you will need to set the LED_BRIGHT_HW_CHANGED flag on
the LED and then call led_classdev_notify_brightness_hw_changed to make
it work.

Userspace should correctly show the OSD when this is done.

Benjamin

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Benjamin Berg]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

Hi Daniel,

I had a quick chat with Bastien about this. The conclusion was that
reusing the TOGGLE key may be problematic for gnome-settings-daemon.
And the alternative of a new CYCLE key also has some caveats.

The most straight forward solution is likely to simply handle the
brightness change in the kernel and not report the key to userspace at
all. This should work just fine and at least GNOME will show an on
screen display in response to the brightness change.

Do you think that approach would work well?

Benjamin


On Mon, 2018-05-14 at 18:25 -0600, Daniel Drake wrote:
> Hi Andy,
> 
> On Mon, May 7, 2018 at 8:46 AM, Daniel Drake 
> wrote:
> > > > Some Asus laptops like UX550GE has hotkey (Fn+F7) for keyboard
> > > > backlight toggle. In this UX550GE, the hotkey incremet the
> > > > level
> > > > of brightness for each keypress from 1 to 3, and then switch it
> > > > off when the brightness has been the max. This commit
> > > > interprets
> > > > the code 0xc7 generated from hotkey to KEY_KBDILLUMUP to
> > > > increment
> > > > the brightness, then pass KEY_KBDILLUMTOGGLE to user space
> > > > after
> > > > the brightness max been reached for switching the led off.
> > > > 
> > > 
> > > Pushed to my review and testing queue, thanks!
> > 
> > We found that GNOME's handling of the toggle key is somewhat
> > imperfect
> > and it will need modifying before we achieve the
> > Up-Up-Up-off-Up-Up-Up-off... cycle that we are looking for.
> > 
> > https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/41
> > 
> > In that discussion an alternative perspective was raised:
> > 
> > Is it right for the kernel to modify the key sent to userspace,
> > when
> > it is then relying on the specific userspace action of it changing
> > the
> > brightness to the next expected level? (and this userspace
> > behaviour
> > is not even working right in the GNOME case)
> > 
> > Instead, would it make sense for the kernel to always report TOGGLE
> > in
> > this case, and for GNOME to interpret toggle as simply "cycle
> > through
> > all the available brightness levels"?
> 
> Any comments on this? I am tempted to send a patch to just make this
> key always emit TOGGLE from the kernel given that we have a tentative
> agreement on implementing the brightness cycle within GNOME.
> 
> Daniel
-BEGIN PGP SIGNATURE-

iQIzBAEBCgAdFiEED2NO4vMS33W8E4AFq6ZWhpmFY3AFAlr6434ACgkQq6ZWhpmF
Y3D97A/9Hqi/xvHU5jcgtKQ2AH3pN445whjXrnUPpcN3I6zgeo/2kXHxn+DEFzD/
xsBLRy2hzfvw5HXWsclf8u31TajmP9OOihHTMB+Ng7ZJohIEgwPjIMr/eaHiiLYr
W9aamm/5DJF5cwYXmiQ46+y+Wbc5ZJbu0wSgiMDY2uEhlph/9J3NGRadb6xEWJX+
Rrp2te5lqwIvHKv4tF/HOrXev6R77AdpHy+WYrA1IKdG996kFDAZcLErk5EL/J0U
+p+pkGbul/JHKcwU77L/Sg8daM1GMQfxWKbR9k21Fow27iaJRMn9Vn9s8HEmHJ+5
S8cBIo71EjNHXO2oDaSzI9Ng1q+Zy7XjHtbK8/MmauQ/3n/z3tt8ZkW4/FumdMkm
ZZwmS9OxOTc+G9hc3SLkggRF1ygbaCgsZaynyEiBHSq37V0G9WBL6BaDW9P6jEIV
7aPJN6SEBLqXStBZiwNXn/yr1AC7duhnIvzxYZj/SUdCk18aDS/tGIehGTqTumGV
2jhyHQek2fp7J0AZH8oOnU9rAIdKtPb3vf5DJuCOD2TtB/yMZDJVj1Uc1g565IPs
KcVdMdiqTy7m65BAwjJyrFu04GIi02SBFFEsdKGOkAcuPPWpG+4EnUwGpArwxwYv
SaUnUD4DRvsoyZHeXETsa7ePh9aBZwmNR+AGSoLj8UY+vbh3/dg=
=szZs
-END PGP SIGNATURE-

Re: [PATCH v2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Benjamin Berg]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

Hi Daniel,

I had a quick chat with Bastien about this. The conclusion was that
reusing the TOGGLE key may be problematic for gnome-settings-daemon.
And the alternative of a new CYCLE key also has some caveats.

The most straight forward solution is likely to simply handle the
brightness change in the kernel and not report the key to userspace at
all. This should work just fine and at least GNOME will show an on
screen display in response to the brightness change.

Do you think that approach would work well?

Benjamin


On Mon, 2018-05-14 at 18:25 -0600, Daniel Drake wrote:
> Hi Andy,
> 
> On Mon, May 7, 2018 at 8:46 AM, Daniel Drake 
> wrote:
> > > > Some Asus laptops like UX550GE has hotkey (Fn+F7) for keyboard
> > > > backlight toggle. In this UX550GE, the hotkey incremet the
> > > > level
> > > > of brightness for each keypress from 1 to 3, and then switch it
> > > > off when the brightness has been the max. This commit
> > > > interprets
> > > > the code 0xc7 generated from hotkey to KEY_KBDILLUMUP to
> > > > increment
> > > > the brightness, then pass KEY_KBDILLUMTOGGLE to user space
> > > > after
> > > > the brightness max been reached for switching the led off.
> > > > 
> > > 
> > > Pushed to my review and testing queue, thanks!
> > 
> > We found that GNOME's handling of the toggle key is somewhat
> > imperfect
> > and it will need modifying before we achieve the
> > Up-Up-Up-off-Up-Up-Up-off... cycle that we are looking for.
> > 
> > https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/41
> > 
> > In that discussion an alternative perspective was raised:
> > 
> > Is it right for the kernel to modify the key sent to userspace,
> > when
> > it is then relying on the specific userspace action of it changing
> > the
> > brightness to the next expected level? (and this userspace
> > behaviour
> > is not even working right in the GNOME case)
> > 
> > Instead, would it make sense for the kernel to always report TOGGLE
> > in
> > this case, and for GNOME to interpret toggle as simply "cycle
> > through
> > all the available brightness levels"?
> 
> Any comments on this? I am tempted to send a patch to just make this
> key always emit TOGGLE from the kernel given that we have a tentative
> agreement on implementing the brightness cycle within GNOME.
> 
> Daniel
-BEGIN PGP SIGNATURE-

iQIzBAEBCgAdFiEED2NO4vMS33W8E4AFq6ZWhpmFY3AFAlr6434ACgkQq6ZWhpmF
Y3D97A/9Hqi/xvHU5jcgtKQ2AH3pN445whjXrnUPpcN3I6zgeo/2kXHxn+DEFzD/
xsBLRy2hzfvw5HXWsclf8u31TajmP9OOihHTMB+Ng7ZJohIEgwPjIMr/eaHiiLYr
W9aamm/5DJF5cwYXmiQ46+y+Wbc5ZJbu0wSgiMDY2uEhlph/9J3NGRadb6xEWJX+
Rrp2te5lqwIvHKv4tF/HOrXev6R77AdpHy+WYrA1IKdG996kFDAZcLErk5EL/J0U
+p+pkGbul/JHKcwU77L/Sg8daM1GMQfxWKbR9k21Fow27iaJRMn9Vn9s8HEmHJ+5
S8cBIo71EjNHXO2oDaSzI9Ng1q+Zy7XjHtbK8/MmauQ/3n/z3tt8ZkW4/FumdMkm
ZZwmS9OxOTc+G9hc3SLkggRF1ygbaCgsZaynyEiBHSq37V0G9WBL6BaDW9P6jEIV
7aPJN6SEBLqXStBZiwNXn/yr1AC7duhnIvzxYZj/SUdCk18aDS/tGIehGTqTumGV
2jhyHQek2fp7J0AZH8oOnU9rAIdKtPb3vf5DJuCOD2TtB/yMZDJVj1Uc1g565IPs
KcVdMdiqTy7m65BAwjJyrFu04GIi02SBFFEsdKGOkAcuPPWpG+4EnUwGpArwxwYv
SaUnUD4DRvsoyZHeXETsa7ePh9aBZwmNR+AGSoLj8UY+vbh3/dg=
=szZs
-END PGP SIGNATURE-

Re: [PATCH] ALSA: hda/realtek: Limit mic boost on T480

[Benjamin Berg]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

Hi Peter,

On Mon, 2018-02-26 at 08:46 +, Peter FP1 Zhang wrote:
> The following comments from our Accessory BU hardware team FYI.
> //
> Most of dock audio is converted from USB if it is connected by cable,
> but CS13 Mechnical dock is not. We need to know the specific dock
> model first.
> //

I think that basically answers the question and we can conclude that
the 80s series has USB audio on the docking stations.

CS13 is the previous generation docking (at the bottom of the laptops)
while the new models have a new docking connector at the side (which is
USB-C + 14 pins primarily for Ethernet incl. LEDs and likely some other
purpose).

Thanks,
Benjamin

> -Original Message-
> From: Peter FP1 Zhang 
> Sent: Wednesday, February 21, 2018 12:07 AM
> To: 'Takashi Iwai'
> Cc: Benjamin Berg; alsa-de...@alsa-project.org; Jaroslav Kysela; linu
> x-ker...@vger.kernel.org; kail...@realtek.com
> Subject: RE: [PATCH] ALSA: hda/realtek: Limit mic boost on T480
> 
> Thank you Takashi. 
> I will also double confirm it with our Accessory BU guys about Ben's
> question when I back to my office. And will let you know if there was
> any different answer from them.
> 
> 
> Peter Zhang \ 张福平,  PMP
> ThinkPad & ThinkStation Linux Solutions
> Tel: (+86) 181-1611-8005 | Lenovo Shanghai
> 
> Linux for Those Who Do - http://www.lenovo.com/linux
> 
> -Original Message-
> From: Takashi Iwai [mailto:ti...@suse.de]
> Sent: Tuesday, February 20, 2018 11:48 PM
> To: Peter FP1 Zhang
> Cc: Benjamin Berg; alsa-de...@alsa-project.org; Jaroslav Kysela; linu
> x-ker...@vger.kernel.org; kail...@realtek.com
> Subject: Re: [PATCH] ALSA: hda/realtek: Limit mic boost on T480
> 
> On Sat, 17 Feb 2018 14:54:02 +0100,
> Peter FP1 Zhang wrote:
> > 
> > Hi Ben,
> > 
> > My understanding is same as yours. Recently Realtek Audio expert
> > Kailang (on copy) submitted a patch for Dock as the attached, are
> > you asking the same thing?
> > Looks like the issue is related to Dock model, there are many
> > different Dock models, could you please let us know which specific
> > dock model you mean? Thanks.
> > 
> > @Kailang, Do you have any suggestions or comments?
> 
> FYI, because of this unclearness, the patch from Benjamin is still
> pending.  I'm OK to apply it at first, then adjust later if the
> docking station quirk is required, too.  Just let me know.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > Thanks,
> > 
> > Peter Zhang \ 张福平,  PMP
> > ThinkPad & ThinkStation Linux Solutions
> > Tel: (+86) 181-1611-8005 | Lenovo Shanghai
> > 
> > Linux for Those Who Do - http://www.lenovo.com/linux
> > 
> > 
> > -Original Message-
> > From: Benjamin Berg [mailto:bb...@redhat.com]
> > Sent: Wednesday, February 14, 2018 11:41 PM
> > To: Takashi Iwai
> > Cc: alsa-de...@alsa-project.org; Jaroslav Kysela; 
> > linux-kernel@vger.kernel.org; Peter FP1 Zhang
> > Subject: Re: [PATCH] ALSA: hda/realtek: Limit mic boost on T480
> > 
> > -BEGIN PGP SIGNED MESSAGE-
> > Hash: SHA512
> > 
> > On Wed, 2018-02-14 at 14:00 +0100, Takashi Iwai wrote:
> > > On Wed, 14 Feb 2018 13:29:39 +0100,
> > > Benjamin Berg wrote:
> > > > 
> > > > The internal mic boost on the T480 is too high. Fix this by 
> > > > applying the ALC269_FIXUP_LIMIT_INT_MIC_BOOST fixup to the
> > > > machine 
> > > > to limit the gain.
> > > > 
> > > > Signed-off-by: Benjamin Berg <bb...@redhat.com>
> > > > Tested-by: Benjamin Berg <bb...@redhat.com>
> > > 
> > > Applying this quirk itself is OK, but just wonder whether this
> > > model 
> > > has a docking station port.  Recently we fixed the dock issue,
> > > and 
> > > it required to apply ALC298_FIXUP_TPT470_DOCK.
> > 
> > I assumed that on the T480 and similar models the audio on the dock
> > is connected through USB (the dock is USB-C + further pins), but I
> > cannot confirm this myself right now as I only have a T480
> > currently.
> > 
> > I have now contacted Peter Zhang of Lenovo about this and also to
> > check if other 80 series models may need the same fixup.
> > 
> > Benjamin
> > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > > ---
> > > >  sound/pci/hda/patch_realtek.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/sound/pci/hda/patch_realtek.c 
&g

Re: [PATCH] ALSA: hda/realtek: Limit mic boost on T480

[Benjamin Berg]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

Hi Peter,

On Mon, 2018-02-26 at 08:46 +, Peter FP1 Zhang wrote:
> The following comments from our Accessory BU hardware team FYI.
> //
> Most of dock audio is converted from USB if it is connected by cable,
> but CS13 Mechnical dock is not. We need to know the specific dock
> model first.
> //

I think that basically answers the question and we can conclude that
the 80s series has USB audio on the docking stations.

CS13 is the previous generation docking (at the bottom of the laptops)
while the new models have a new docking connector at the side (which is
USB-C + 14 pins primarily for Ethernet incl. LEDs and likely some other
purpose).

Thanks,
Benjamin

> -Original Message-
> From: Peter FP1 Zhang 
> Sent: Wednesday, February 21, 2018 12:07 AM
> To: 'Takashi Iwai'
> Cc: Benjamin Berg; alsa-de...@alsa-project.org; Jaroslav Kysela; linu
> x-ker...@vger.kernel.org; kail...@realtek.com
> Subject: RE: [PATCH] ALSA: hda/realtek: Limit mic boost on T480
> 
> Thank you Takashi. 
> I will also double confirm it with our Accessory BU guys about Ben's
> question when I back to my office. And will let you know if there was
> any different answer from them.
> 
> 
> Peter Zhang \ 张福平,  PMP
> ThinkPad & ThinkStation Linux Solutions
> Tel: (+86) 181-1611-8005 | Lenovo Shanghai
> 
> Linux for Those Who Do - http://www.lenovo.com/linux
> 
> -Original Message-
> From: Takashi Iwai [mailto:ti...@suse.de]
> Sent: Tuesday, February 20, 2018 11:48 PM
> To: Peter FP1 Zhang
> Cc: Benjamin Berg; alsa-de...@alsa-project.org; Jaroslav Kysela; linu
> x-ker...@vger.kernel.org; kail...@realtek.com
> Subject: Re: [PATCH] ALSA: hda/realtek: Limit mic boost on T480
> 
> On Sat, 17 Feb 2018 14:54:02 +0100,
> Peter FP1 Zhang wrote:
> > 
> > Hi Ben,
> > 
> > My understanding is same as yours. Recently Realtek Audio expert
> > Kailang (on copy) submitted a patch for Dock as the attached, are
> > you asking the same thing?
> > Looks like the issue is related to Dock model, there are many
> > different Dock models, could you please let us know which specific
> > dock model you mean? Thanks.
> > 
> > @Kailang, Do you have any suggestions or comments?
> 
> FYI, because of this unclearness, the patch from Benjamin is still
> pending.  I'm OK to apply it at first, then adjust later if the
> docking station quirk is required, too.  Just let me know.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > Thanks,
> > 
> > Peter Zhang \ 张福平,  PMP
> > ThinkPad & ThinkStation Linux Solutions
> > Tel: (+86) 181-1611-8005 | Lenovo Shanghai
> > 
> > Linux for Those Who Do - http://www.lenovo.com/linux
> > 
> > 
> > -Original Message-
> > From: Benjamin Berg [mailto:bb...@redhat.com]
> > Sent: Wednesday, February 14, 2018 11:41 PM
> > To: Takashi Iwai
> > Cc: alsa-de...@alsa-project.org; Jaroslav Kysela; 
> > linux-kernel@vger.kernel.org; Peter FP1 Zhang
> > Subject: Re: [PATCH] ALSA: hda/realtek: Limit mic boost on T480
> > 
> > -BEGIN PGP SIGNED MESSAGE-
> > Hash: SHA512
> > 
> > On Wed, 2018-02-14 at 14:00 +0100, Takashi Iwai wrote:
> > > On Wed, 14 Feb 2018 13:29:39 +0100,
> > > Benjamin Berg wrote:
> > > > 
> > > > The internal mic boost on the T480 is too high. Fix this by 
> > > > applying the ALC269_FIXUP_LIMIT_INT_MIC_BOOST fixup to the
> > > > machine 
> > > > to limit the gain.
> > > > 
> > > > Signed-off-by: Benjamin Berg 
> > > > Tested-by: Benjamin Berg 
> > > 
> > > Applying this quirk itself is OK, but just wonder whether this
> > > model 
> > > has a docking station port.  Recently we fixed the dock issue,
> > > and 
> > > it required to apply ALC298_FIXUP_TPT470_DOCK.
> > 
> > I assumed that on the T480 and similar models the audio on the dock
> > is connected through USB (the dock is USB-C + further pins), but I
> > cannot confirm this myself right now as I only have a T480
> > currently.
> > 
> > I have now contacted Peter Zhang of Lenovo about this and also to
> > check if other 80 series models may need the same fixup.
> > 
> > Benjamin
> > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > > ---
> > > >  sound/pci/hda/patch_realtek.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/sound/pci/hda/patch_realtek.c 
> > > > b/sound/pci/hda/patch_realt

Re: [PATCH] ALSA: hda/realtek: Limit mic boost on T480

[Benjamin Berg]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

On Wed, 2018-02-14 at 14:00 +0100, Takashi Iwai wrote:
> On Wed, 14 Feb 2018 13:29:39 +0100,
> Benjamin Berg wrote:
> > 
> > The internal mic boost on the T480 is too high. Fix this by
> > applying the
> > ALC269_FIXUP_LIMIT_INT_MIC_BOOST fixup to the machine to limit the
> > gain.
> > 
> > Signed-off-by: Benjamin Berg <bb...@redhat.com>
> > Tested-by: Benjamin Berg <bb...@redhat.com>
> 
> Applying this quirk itself is OK, but just wonder whether this model
> has a docking station port.  Recently we fixed the dock issue, and it
> required to apply ALC298_FIXUP_TPT470_DOCK.

I assumed that on the T480 and similar models the audio on the dock is
connected through USB (the dock is USB-C + further pins), but I cannot
confirm this myself right now as I only have a T480 currently.

I have now contacted Peter Zhang of Lenovo about this and also to check
if other 80 series models may need the same fixup.

Benjamin

> 
> thanks,
> 
> Takashi
> 
> > ---
> >  sound/pci/hda/patch_realtek.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/sound/pci/hda/patch_realtek.c
> > b/sound/pci/hda/patch_realtek.c
> > index ce28f7ce64e6..8467ce3db767 100644
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -6510,6 +6510,7 @@ static const struct snd_pci_quirk
> > alc269_fixup_tbl[] = {
> > SND_PCI_QUIRK(0x17aa, 0x224b, "Thinkpad",
> > ALC298_FIXUP_TPT470_DOCK),
> > SND_PCI_QUIRK(0x17aa, 0x224c, "Thinkpad",
> > ALC298_FIXUP_TPT470_DOCK),
> > SND_PCI_QUIRK(0x17aa, 0x224d, "Thinkpad",
> > ALC298_FIXUP_TPT470_DOCK),
> > +   SND_PCI_QUIRK(0x17aa, 0x225d, "Thinkpad T480",
> > ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
> > SND_PCI_QUIRK(0x17aa, 0x30bb, "ThinkCentre AIO",
> > ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
> > SND_PCI_QUIRK(0x17aa, 0x30e2, "ThinkCentre AIO",
> > ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
> > SND_PCI_QUIRK(0x17aa, 0x310c, "ThinkCentre Station",
> > ALC294_FIXUP_LENOVO_MIC_LOCATION),
> > -- 
> > 2.14.3
> > 
> > 
-BEGIN PGP SIGNATURE-

iQIzBAEBCgAdFiEED2NO4vMS33W8E4AFq6ZWhpmFY3AFAlqEWKkACgkQq6ZWhpmF
Y3AEyQ/7B6l4Na1GLDf11P8OdzzcXOy6grjvrLYwq6WFA3M1DirlQsPwjSbnJ7V8
znIX30EK6My9Ip6jyvJQIpiBlaX1FKJuJ3So9mjgUeOcCQMFkwnC1fMMyyWEAWLq
TI1s7F5w4OQwhQpwtlId4AIB7A2ZzQmP/9YqfIPDtIiNJqjd9aRruBJi8A8j3sB5
slY1CYgCQhBUiAkUPisFrt+b4aFU5FlgINQR+6uOSKO0/IaSKq9rFwX76lOTOsBj
kHhywIF9Qef6CvUDaAOFWjQcTA/ooDPifUsfNzo6ZHnNZIks0Cov12JeO5UrxjiD
a3BuG6SZAVm4awfkTj5pIL0DuBAXYhTJd/hU3VMWdV0/kb0OeVONSahMzhSOCM30
WdFKpWC0+n3TEkTWSG9QGwL6wbXiaLNZ0JRgQkcoJ9JqPVngFf9LIRBzqdMyzyCE
UzLu/oQ9XlXQrFx1IG5KS/RJJPUwpJKhXOygSpRZx/HhmaWpPVLCu9GNg+ytoVIL
QEg381jOTTPi+DX0k07WYK+hUEvB83cbtsm+EiEGMa4bxuhYQp2p1moGqOrzv2XK
iEN8LfF97KJ4GtqVWybnstxMxMdS0CwXBPxUZ3OY6C03S3sdVqk/QrEpE4i0+N8P
CcX3g+rEJVUkpsdm17Sf5u4DjeGH7HBPsDmHirrwvFdeOkTiO4E=
=NIZ6
-END PGP SIGNATURE-

Re: [PATCH] ALSA: hda/realtek: Limit mic boost on T480

[Benjamin Berg]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

On Wed, 2018-02-14 at 14:00 +0100, Takashi Iwai wrote:
> On Wed, 14 Feb 2018 13:29:39 +0100,
> Benjamin Berg wrote:
> > 
> > The internal mic boost on the T480 is too high. Fix this by
> > applying the
> > ALC269_FIXUP_LIMIT_INT_MIC_BOOST fixup to the machine to limit the
> > gain.
> > 
> > Signed-off-by: Benjamin Berg 
> > Tested-by: Benjamin Berg 
> 
> Applying this quirk itself is OK, but just wonder whether this model
> has a docking station port.  Recently we fixed the dock issue, and it
> required to apply ALC298_FIXUP_TPT470_DOCK.

I assumed that on the T480 and similar models the audio on the dock is
connected through USB (the dock is USB-C + further pins), but I cannot
confirm this myself right now as I only have a T480 currently.

I have now contacted Peter Zhang of Lenovo about this and also to check
if other 80 series models may need the same fixup.

Benjamin

> 
> thanks,
> 
> Takashi
> 
> > ---
> >  sound/pci/hda/patch_realtek.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/sound/pci/hda/patch_realtek.c
> > b/sound/pci/hda/patch_realtek.c
> > index ce28f7ce64e6..8467ce3db767 100644
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -6510,6 +6510,7 @@ static const struct snd_pci_quirk
> > alc269_fixup_tbl[] = {
> > SND_PCI_QUIRK(0x17aa, 0x224b, "Thinkpad",
> > ALC298_FIXUP_TPT470_DOCK),
> > SND_PCI_QUIRK(0x17aa, 0x224c, "Thinkpad",
> > ALC298_FIXUP_TPT470_DOCK),
> > SND_PCI_QUIRK(0x17aa, 0x224d, "Thinkpad",
> > ALC298_FIXUP_TPT470_DOCK),
> > +   SND_PCI_QUIRK(0x17aa, 0x225d, "Thinkpad T480",
> > ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
> > SND_PCI_QUIRK(0x17aa, 0x30bb, "ThinkCentre AIO",
> > ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
> > SND_PCI_QUIRK(0x17aa, 0x30e2, "ThinkCentre AIO",
> > ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
> > SND_PCI_QUIRK(0x17aa, 0x310c, "ThinkCentre Station",
> > ALC294_FIXUP_LENOVO_MIC_LOCATION),
> > -- 
> > 2.14.3
> > 
> > 
-BEGIN PGP SIGNATURE-

iQIzBAEBCgAdFiEED2NO4vMS33W8E4AFq6ZWhpmFY3AFAlqEWKkACgkQq6ZWhpmF
Y3AEyQ/7B6l4Na1GLDf11P8OdzzcXOy6grjvrLYwq6WFA3M1DirlQsPwjSbnJ7V8
znIX30EK6My9Ip6jyvJQIpiBlaX1FKJuJ3So9mjgUeOcCQMFkwnC1fMMyyWEAWLq
TI1s7F5w4OQwhQpwtlId4AIB7A2ZzQmP/9YqfIPDtIiNJqjd9aRruBJi8A8j3sB5
slY1CYgCQhBUiAkUPisFrt+b4aFU5FlgINQR+6uOSKO0/IaSKq9rFwX76lOTOsBj
kHhywIF9Qef6CvUDaAOFWjQcTA/ooDPifUsfNzo6ZHnNZIks0Cov12JeO5UrxjiD
a3BuG6SZAVm4awfkTj5pIL0DuBAXYhTJd/hU3VMWdV0/kb0OeVONSahMzhSOCM30
WdFKpWC0+n3TEkTWSG9QGwL6wbXiaLNZ0JRgQkcoJ9JqPVngFf9LIRBzqdMyzyCE
UzLu/oQ9XlXQrFx1IG5KS/RJJPUwpJKhXOygSpRZx/HhmaWpPVLCu9GNg+ytoVIL
QEg381jOTTPi+DX0k07WYK+hUEvB83cbtsm+EiEGMa4bxuhYQp2p1moGqOrzv2XK
iEN8LfF97KJ4GtqVWybnstxMxMdS0CwXBPxUZ3OY6C03S3sdVqk/QrEpE4i0+N8P
CcX3g+rEJVUkpsdm17Sf5u4DjeGH7HBPsDmHirrwvFdeOkTiO4E=
=NIZ6
-END PGP SIGNATURE-

[PATCH] ALSA: hda/realtek: Limit mic boost on T480

[Benjamin Berg]

The internal mic boost on the T480 is too high. Fix this by applying the
ALC269_FIXUP_LIMIT_INT_MIC_BOOST fixup to the machine to limit the gain.

Signed-off-by: Benjamin Berg <bb...@redhat.com>
Tested-by: Benjamin Berg <bb...@redhat.com>
---
 sound/pci/hda/patch_realtek.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index ce28f7ce64e6..8467ce3db767 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6510,6 +6510,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x17aa, 0x224b, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
SND_PCI_QUIRK(0x17aa, 0x224c, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
SND_PCI_QUIRK(0x17aa, 0x224d, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
+   SND_PCI_QUIRK(0x17aa, 0x225d, "Thinkpad T480", 
ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
SND_PCI_QUIRK(0x17aa, 0x30bb, "ThinkCentre AIO", 
ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
SND_PCI_QUIRK(0x17aa, 0x30e2, "ThinkCentre AIO", 
ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
SND_PCI_QUIRK(0x17aa, 0x310c, "ThinkCentre Station", 
ALC294_FIXUP_LENOVO_MIC_LOCATION),
-- 
2.14.3

[PATCH] ALSA: hda/realtek: Limit mic boost on T480

[Benjamin Berg]

The internal mic boost on the T480 is too high. Fix this by applying the
ALC269_FIXUP_LIMIT_INT_MIC_BOOST fixup to the machine to limit the gain.

Signed-off-by: Benjamin Berg 
Tested-by: Benjamin Berg 
---
 sound/pci/hda/patch_realtek.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index ce28f7ce64e6..8467ce3db767 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6510,6 +6510,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x17aa, 0x224b, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
SND_PCI_QUIRK(0x17aa, 0x224c, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
SND_PCI_QUIRK(0x17aa, 0x224d, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
+   SND_PCI_QUIRK(0x17aa, 0x225d, "Thinkpad T480", 
ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
SND_PCI_QUIRK(0x17aa, 0x30bb, "ThinkCentre AIO", 
ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
SND_PCI_QUIRK(0x17aa, 0x30e2, "ThinkCentre AIO", 
ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
SND_PCI_QUIRK(0x17aa, 0x310c, "ThinkCentre Station", 
ALC294_FIXUP_LENOVO_MIC_LOCATION),
-- 
2.14.3

[PATCHv2] thinkpad_acpi: Implement tablet mode resolving using GMMS method

[Benjamin Berg]

Many thinkpad laptops and convertibles provide the GMMS method to
resolve how far the laptop has been opened and whether it has been
converted into tablet mode. This allows reporting a more precise tablet
mode state to userspace.

The current implementation only reports a summarized tablet mode state
which is triggered as soon as the input devices become unusable as they
are folded away from the display.

This will work on all models where the CMMD method was used previously and
it may also work in other cases.

Thanks to Peter Zhang of Lenovo for providing information on how to use the
GMMS method to query the tablet mode.

Signed-off-by: Benjamin Berg <bb...@redhat.com>
Cc: Peter FP1 Zhang <zhang...@lenovo.com>
Cc: Lyude Paul <ly...@redhat.com>

--

v2:
 - It appears that mode 4 may also report the FLAT state. This has
   been observed on an X1 Yoga 2nd Generation.
---
 drivers/platform/x86/thinkpad_acpi.c | 135 +++
 1 file changed, 122 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c 
b/drivers/platform/x86/thinkpad_acpi.c
index 2242d6035d9e..298a0088eb0c 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -310,8 +310,7 @@ static struct {
enum {
TP_HOTKEY_TABLET_NONE = 0,
TP_HOTKEY_TABLET_USES_MHKG,
-   /* X1 Yoga 2016, seen on BIOS N1FET44W */
-   TP_HOTKEY_TABLET_USES_CMMD,
+   TP_HOTKEY_TABLET_USES_GMMS,
} hotkey_tablet;
u32 kbdlight:1;
u32 light:1;
@@ -2044,8 +2043,28 @@ static void hotkey_poll_setup(const bool may_warn);
 
 /* HKEY.MHKG() return bits */
 #define TP_HOTKEY_TABLET_MASK (1 << 3)
-/* ThinkPad X1 Yoga (2016) */
-#define TP_EC_CMMD_TABLET_MODE 0x6
+enum {
+   TP_ACPI_MULTI_MODE_INVALID  = 0,
+   TP_ACPI_MULTI_MODE_UNKNOWN  = 1 << 0,
+   TP_ACPI_MULTI_MODE_LAPTOP   = 1 << 1,
+   TP_ACPI_MULTI_MODE_TABLET   = 1 << 2,
+   TP_ACPI_MULTI_MODE_FLAT = 1 << 3,
+   TP_ACPI_MULTI_MODE_STAND= 1 << 4,
+   TP_ACPI_MULTI_MODE_TENT = 1 << 5,
+   TP_ACPI_MULTI_MODE_STAND_TENT   = 1 << 6,
+};
+
+enum {
+   /* The following modes are considered tablet mode for the purpose of
+* reporting the status to userspace. i.e. in all these modes it makes
+* sense to disable the laptop input devices such as touchpad and
+* keyboard.
+*/
+   TP_ACPI_MULTI_MODE_TABLET_LIKE  = TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT |
+ TP_ACPI_MULTI_MODE_STAND_TENT,
+};
 
 static int hotkey_get_wlsw(void)
 {
@@ -2066,6 +2085,93 @@ static int hotkey_get_wlsw(void)
return (status) ? TPACPI_RFK_RADIO_ON : TPACPI_RFK_RADIO_OFF;
 }
 
+static int hotkey_gmms_get_tablet_mode(int s, int *has_tablet_mode)
+{
+   int type = (s >> 16) & 0x;
+   int value = s & 0x;
+   int mode = TP_ACPI_MULTI_MODE_INVALID;
+   int valid_modes = 0;
+
+   if (has_tablet_mode)
+   *has_tablet_mode = 0;
+
+   switch (type) {
+   case 1:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND_TENT;
+   break;
+   case 2:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   case 3:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT;
+   break;
+   case 4:
+   /* Flat mode is included here as it can be seen at least
+* on the X1 Yoga 2nd Gen. */
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   case 5:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   default:
+   pr_err("Unknown multi mode status type %d with value 0x%04X, 
please report this to %s\n",
+  type, value, TPACPI_MAIL);
+   return 0

[PATCHv2] thinkpad_acpi: Implement tablet mode resolving using GMMS method

[Benjamin Berg]

Many thinkpad laptops and convertibles provide the GMMS method to
resolve how far the laptop has been opened and whether it has been
converted into tablet mode. This allows reporting a more precise tablet
mode state to userspace.

The current implementation only reports a summarized tablet mode state
which is triggered as soon as the input devices become unusable as they
are folded away from the display.

This will work on all models where the CMMD method was used previously and
it may also work in other cases.

Thanks to Peter Zhang of Lenovo for providing information on how to use the
GMMS method to query the tablet mode.

Signed-off-by: Benjamin Berg 
Cc: Peter FP1 Zhang 
Cc: Lyude Paul 

--

v2:
 - It appears that mode 4 may also report the FLAT state. This has
   been observed on an X1 Yoga 2nd Generation.
---
 drivers/platform/x86/thinkpad_acpi.c | 135 +++
 1 file changed, 122 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c 
b/drivers/platform/x86/thinkpad_acpi.c
index 2242d6035d9e..298a0088eb0c 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -310,8 +310,7 @@ static struct {
enum {
TP_HOTKEY_TABLET_NONE = 0,
TP_HOTKEY_TABLET_USES_MHKG,
-   /* X1 Yoga 2016, seen on BIOS N1FET44W */
-   TP_HOTKEY_TABLET_USES_CMMD,
+   TP_HOTKEY_TABLET_USES_GMMS,
} hotkey_tablet;
u32 kbdlight:1;
u32 light:1;
@@ -2044,8 +2043,28 @@ static void hotkey_poll_setup(const bool may_warn);
 
 /* HKEY.MHKG() return bits */
 #define TP_HOTKEY_TABLET_MASK (1 << 3)
-/* ThinkPad X1 Yoga (2016) */
-#define TP_EC_CMMD_TABLET_MODE 0x6
+enum {
+   TP_ACPI_MULTI_MODE_INVALID  = 0,
+   TP_ACPI_MULTI_MODE_UNKNOWN  = 1 << 0,
+   TP_ACPI_MULTI_MODE_LAPTOP   = 1 << 1,
+   TP_ACPI_MULTI_MODE_TABLET   = 1 << 2,
+   TP_ACPI_MULTI_MODE_FLAT = 1 << 3,
+   TP_ACPI_MULTI_MODE_STAND= 1 << 4,
+   TP_ACPI_MULTI_MODE_TENT = 1 << 5,
+   TP_ACPI_MULTI_MODE_STAND_TENT   = 1 << 6,
+};
+
+enum {
+   /* The following modes are considered tablet mode for the purpose of
+* reporting the status to userspace. i.e. in all these modes it makes
+* sense to disable the laptop input devices such as touchpad and
+* keyboard.
+*/
+   TP_ACPI_MULTI_MODE_TABLET_LIKE  = TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT |
+ TP_ACPI_MULTI_MODE_STAND_TENT,
+};
 
 static int hotkey_get_wlsw(void)
 {
@@ -2066,6 +2085,93 @@ static int hotkey_get_wlsw(void)
return (status) ? TPACPI_RFK_RADIO_ON : TPACPI_RFK_RADIO_OFF;
 }
 
+static int hotkey_gmms_get_tablet_mode(int s, int *has_tablet_mode)
+{
+   int type = (s >> 16) & 0x;
+   int value = s & 0x;
+   int mode = TP_ACPI_MULTI_MODE_INVALID;
+   int valid_modes = 0;
+
+   if (has_tablet_mode)
+   *has_tablet_mode = 0;
+
+   switch (type) {
+   case 1:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND_TENT;
+   break;
+   case 2:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   case 3:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT;
+   break;
+   case 4:
+   /* Flat mode is included here as it can be seen at least
+* on the X1 Yoga 2nd Gen. */
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   case 5:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   default:
+   pr_err("Unknown multi mode status type %d with value 0x%04X, 
please report this to %s\n",
+  type, value, TPACPI_MAIL);
+   return 0;
+   }
+
+   if (has_tablet_mode && (valid_modes & TP_

[PATCH] thinkpad_acpi: Implement tablet mode resolving using GMMS method

[Benjamin Berg]

Many thinkpad laptops and convertibles provide the GMMS method to
resolve how far the laptop has been opened and whether it has been
converted into tablet mode. This allows reporting a more precise tablet
mode state to userspace.

The current implementation only reports a summarized tablet mode state
which is triggered as soon as the input devices become unusable as they
are folded away from the display.

This will work on all models where the CMMD method was used previously and
it may also work in other cases.

Thanks to Peter Zhang of Lenovo for providing information on how to use the
GMMS method to query the tablet mode.

Signed-off-by: Benjamin Berg <bb...@redhat.com>
Cc: Peter FP1 Zhang <zhang...@lenovo.com>
Cc: Lyude Paul <ly...@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 132 +++
 1 file changed, 119 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c 
b/drivers/platform/x86/thinkpad_acpi.c
index 2242d6035d9e..91fab1a13a6d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -310,8 +310,7 @@ static struct {
enum {
TP_HOTKEY_TABLET_NONE = 0,
TP_HOTKEY_TABLET_USES_MHKG,
-   /* X1 Yoga 2016, seen on BIOS N1FET44W */
-   TP_HOTKEY_TABLET_USES_CMMD,
+   TP_HOTKEY_TABLET_USES_GMMS,
} hotkey_tablet;
u32 kbdlight:1;
u32 light:1;
@@ -2044,8 +2043,28 @@ static void hotkey_poll_setup(const bool may_warn);
 
 /* HKEY.MHKG() return bits */
 #define TP_HOTKEY_TABLET_MASK (1 << 3)
-/* ThinkPad X1 Yoga (2016) */
-#define TP_EC_CMMD_TABLET_MODE 0x6
+enum {
+   TP_ACPI_MULTI_MODE_INVALID  = 0,
+   TP_ACPI_MULTI_MODE_UNKNOWN  = 1 << 0,
+   TP_ACPI_MULTI_MODE_LAPTOP   = 1 << 1,
+   TP_ACPI_MULTI_MODE_TABLET   = 1 << 2,
+   TP_ACPI_MULTI_MODE_FLAT = 1 << 3,
+   TP_ACPI_MULTI_MODE_STAND= 1 << 4,
+   TP_ACPI_MULTI_MODE_TENT = 1 << 5,
+   TP_ACPI_MULTI_MODE_STAND_TENT   = 1 << 6,
+};
+
+enum {
+   /* The following modes are considered tablet mode for the purpose of
+* reporting the status to userspace. i.e. in all these modes it makes
+* sense to disable the laptop input devices such as touchpad and
+* keyboard.
+*/
+   TP_ACPI_MULTI_MODE_TABLET_LIKE  = TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT |
+ TP_ACPI_MULTI_MODE_STAND_TENT,
+};
 
 static int hotkey_get_wlsw(void)
 {
@@ -2066,6 +2085,90 @@ static int hotkey_get_wlsw(void)
return (status) ? TPACPI_RFK_RADIO_ON : TPACPI_RFK_RADIO_OFF;
 }
 
+static int hotkey_gmms_get_tablet_mode(int s, int *has_tablet_mode)
+{
+   int type = (s >> 16) & 0x;
+   int value = s & 0x;
+   int mode = TP_ACPI_MULTI_MODE_INVALID;
+   int valid_modes = 0;
+
+   if (has_tablet_mode)
+   *has_tablet_mode = 0;
+
+   switch (type) {
+   case 1:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND_TENT;
+   break;
+   case 2:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   case 3:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT;
+   break;
+   case 4:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   case 5:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   default:
+   pr_err("Unknown multi mode status type %d with value 0x%04X, 
please report this to %s\n",
+  type, value, TPACPI_MAIL);
+   return 0;
+   }
+
+   if (has_tablet_mode && (valid_modes & TP_ACPI_MULTI_MODE_TABLET_LIKE))
+   *has_tablet_mode = 1;
+
+   switch (value) {
+   case 1:
+   mode = TP_ACPI_MULTI_MODE_LAPTOP;
+   break;
+   case 2:
+

[PATCH] thinkpad_acpi: Implement tablet mode resolving using GMMS method

[Benjamin Berg]

Many thinkpad laptops and convertibles provide the GMMS method to
resolve how far the laptop has been opened and whether it has been
converted into tablet mode. This allows reporting a more precise tablet
mode state to userspace.

The current implementation only reports a summarized tablet mode state
which is triggered as soon as the input devices become unusable as they
are folded away from the display.

This will work on all models where the CMMD method was used previously and
it may also work in other cases.

Thanks to Peter Zhang of Lenovo for providing information on how to use the
GMMS method to query the tablet mode.

Signed-off-by: Benjamin Berg 
Cc: Peter FP1 Zhang 
Cc: Lyude Paul 
---
 drivers/platform/x86/thinkpad_acpi.c | 132 +++
 1 file changed, 119 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c 
b/drivers/platform/x86/thinkpad_acpi.c
index 2242d6035d9e..91fab1a13a6d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -310,8 +310,7 @@ static struct {
enum {
TP_HOTKEY_TABLET_NONE = 0,
TP_HOTKEY_TABLET_USES_MHKG,
-   /* X1 Yoga 2016, seen on BIOS N1FET44W */
-   TP_HOTKEY_TABLET_USES_CMMD,
+   TP_HOTKEY_TABLET_USES_GMMS,
} hotkey_tablet;
u32 kbdlight:1;
u32 light:1;
@@ -2044,8 +2043,28 @@ static void hotkey_poll_setup(const bool may_warn);
 
 /* HKEY.MHKG() return bits */
 #define TP_HOTKEY_TABLET_MASK (1 << 3)
-/* ThinkPad X1 Yoga (2016) */
-#define TP_EC_CMMD_TABLET_MODE 0x6
+enum {
+   TP_ACPI_MULTI_MODE_INVALID  = 0,
+   TP_ACPI_MULTI_MODE_UNKNOWN  = 1 << 0,
+   TP_ACPI_MULTI_MODE_LAPTOP   = 1 << 1,
+   TP_ACPI_MULTI_MODE_TABLET   = 1 << 2,
+   TP_ACPI_MULTI_MODE_FLAT = 1 << 3,
+   TP_ACPI_MULTI_MODE_STAND= 1 << 4,
+   TP_ACPI_MULTI_MODE_TENT = 1 << 5,
+   TP_ACPI_MULTI_MODE_STAND_TENT   = 1 << 6,
+};
+
+enum {
+   /* The following modes are considered tablet mode for the purpose of
+* reporting the status to userspace. i.e. in all these modes it makes
+* sense to disable the laptop input devices such as touchpad and
+* keyboard.
+*/
+   TP_ACPI_MULTI_MODE_TABLET_LIKE  = TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT |
+ TP_ACPI_MULTI_MODE_STAND_TENT,
+};
 
 static int hotkey_get_wlsw(void)
 {
@@ -2066,6 +2085,90 @@ static int hotkey_get_wlsw(void)
return (status) ? TPACPI_RFK_RADIO_ON : TPACPI_RFK_RADIO_OFF;
 }
 
+static int hotkey_gmms_get_tablet_mode(int s, int *has_tablet_mode)
+{
+   int type = (s >> 16) & 0x;
+   int value = s & 0x;
+   int mode = TP_ACPI_MULTI_MODE_INVALID;
+   int valid_modes = 0;
+
+   if (has_tablet_mode)
+   *has_tablet_mode = 0;
+
+   switch (type) {
+   case 1:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND_TENT;
+   break;
+   case 2:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   case 3:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT;
+   break;
+   case 4:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   case 5:
+   valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
+ TP_ACPI_MULTI_MODE_FLAT |
+ TP_ACPI_MULTI_MODE_TABLET |
+ TP_ACPI_MULTI_MODE_STAND |
+ TP_ACPI_MULTI_MODE_TENT;
+   break;
+   default:
+   pr_err("Unknown multi mode status type %d with value 0x%04X, 
please report this to %s\n",
+  type, value, TPACPI_MAIL);
+   return 0;
+   }
+
+   if (has_tablet_mode && (valid_modes & TP_ACPI_MULTI_MODE_TABLET_LIKE))
+   *has_tablet_mode = 1;
+
+   switch (value) {
+   case 1:
+   mode = TP_ACPI_MULTI_MODE_LAPTOP;
+   break;
+   case 2:
+   mode = TP_ACPI_MULTI_MODE_FLAT;
+   break;
+   case 3:
+  

Re: ath9k: Fix beacon configuration assertion failure

[Benjamin Berg]

On Fr, 2016-08-19 at 13:03 +0300, Kalle Valo wrote:
> Actually, I see two patches which might be related but not identical:
> 
> ath9k: fix client mode beacon configuration
> https://patchwork.kernel.org/patch/9247699/
> 
> ath9k: Fix beacon configuration assertion failure
> https://patchwork.kernel.org/patch/9281191/
> 
> Felix (CCed) & Benjamin: please take a look and advice which one I
> should take.

Yes, both patches are designed to fix the same issue in my patch.

Felix solution looks entirely correct to me, the second solution seems
slightly wrong because it prevents the call to ath9k_beacon_config from
happening instead of ensuring the correct parameter value.
ath9k_beacon_config needs to be called even if iter_data.beacons is
false as it disables the interrupts.

Benjamin

Re: ath9k: Fix beacon configuration assertion failure

[Benjamin Berg]

On Fr, 2016-08-19 at 13:03 +0300, Kalle Valo wrote:
> Actually, I see two patches which might be related but not identical:
> 
> ath9k: fix client mode beacon configuration
> https://patchwork.kernel.org/patch/9247699/
> 
> ath9k: Fix beacon configuration assertion failure
> https://patchwork.kernel.org/patch/9281191/
> 
> Felix (CCed) & Benjamin: please take a look and advice which one I
> should take.

Yes, both patches are designed to fix the same issue in my patch.

Felix solution looks entirely correct to me, the second solution seems
slightly wrong because it prevents the call to ath9k_beacon_config from
happening instead of ensuring the correct parameter value.
ath9k_beacon_config needs to be called even if iter_data.beacons is
false as it disables the interrupts.

Benjamin