Re: [PATCH v2 2/4] gpiolib: acpi: Rework honor_wakeup option into an ignore_wake option

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Mar 02, 2020 at 12:12:23PM +0100, Hans de Goede wrote:
> Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
> quirk mechanism") was added to deal with spurious wakeups on one specific
> model of the HP x2 10 series.
> 
> The approach taken there was to add a bool controlling wakeup support for
> all ACPI GPIO events. This was sufficient for the specific HP x2 10 model
> the commit was trying to fix, but in the mean time other models have
> turned up which need a similar workaround to avoid spurious wakeups from
> suspend, but only for one of the pins on which the ACPI tables request
> ACPI GPIO events.
> 
> Since the honor_wakeup option was added to be able to ignore wake events,
> the name was perhaps not the best, this commit renames it to ignore_wake
> and changes it to a string with the following format:
> gpiolib_acpi.ignore_wake=controller@pin[,controller@pin[,...]]
> 
> This allows working around spurious wakeup issues on a per pin basis.
> 
> This commit also reworks the existing quirk for the HP x2 10 so that
> it functions as before.
> 
> Note:
> -This removes the honor_wakeup parameter. This has only been upstream for
>  a short time and to the best of my knowledge there are no users using
>  this module parameter.
> 
> -The controller@pin[,controller@pin[,...]] syntax is based on an existing
>  kernel module parameter using the same controller@pin format. That version
>  uses ';' as separator, but in practice that is problematic because grub2
>  cannot handle this without taking special care to escape the ';', so here
>  we are using a ',' as separator instead which does not have this issue.
> 

All are fine to me.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> Changes in v2:
> -Rewrite to use a much more generic string based format to describe
>  the controller@pin combination(s) on which to ignore wakeups
> ---
>  drivers/gpio/gpiolib-acpi.c | 96 +++++++++++++++++++++++++++++--------
>  1 file changed, 76 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index a77edd31dd60..87bb0d3c69da 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -21,18 +21,21 @@
>  #include "gpiolib.h"
>  #include "gpiolib-acpi.h"
>  
> -#define QUIRK_NO_EDGE_EVENTS_ON_BOOT		0x01l
> -#define QUIRK_NO_WAKEUP				0x02l
> -
>  static int run_edge_events_on_boot = -1;
>  module_param(run_edge_events_on_boot, int, 0444);
>  MODULE_PARM_DESC(run_edge_events_on_boot,
>  		 "Run edge _AEI event-handlers at boot: 0=no, 1=yes, -1=auto");
>  
> -static int honor_wakeup = -1;
> -module_param(honor_wakeup, int, 0444);
> -MODULE_PARM_DESC(honor_wakeup,
> -		 "Honor the ACPI wake-capable flag: 0=no, 1=yes, -1=auto");
> +static char *ignore_wake;
> +module_param(ignore_wake, charp, 0444);
> +MODULE_PARM_DESC(ignore_wake,
> +		 "controller@pin combos on which to ignore the ACPI wake flag "
> +		 "ignore_wake=controller@pin[,controller@pin[,...]]");
> +
> +struct acpi_gpiolib_dmi_quirk {
> +	bool no_edge_events_on_boot;
> +	char *ignore_wake;
> +};
>  
>  /**
>   * struct acpi_gpio_event - ACPI GPIO event handler data
> @@ -202,6 +205,57 @@ static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
>  		acpi_gpiochip_request_irq(acpi_gpio, event);
>  }
>  
> +static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)
> +{
> +	const char *controller, *pin_str;
> +	int len, pin;
> +	char *endp;
> +
> +	controller = ignore_wake;
> +	while (controller) {
> +		pin_str = strchr(controller, '@');
> +		if (!pin_str)
> +			goto err;
> +
> +		len = pin_str - controller;
> +		if (len == strlen(controller_in) &&
> +		    strncmp(controller, controller_in, len) == 0) {
> +			pin = simple_strtoul(pin_str + 1, &endp, 10);
> +			if (*endp != 0 && *endp != ',')
> +				goto err;
> +
> +			if (pin == pin_in)
> +				return true;
> +		}
> +
> +		controller = strchr(controller, ',');
> +		if (controller)
> +			controller++;
> +	}
> +
> +	return false;
> +err:
> +	pr_err_once("Error invalid value for gpiolib_acpi.ignore_wake: %s\n",
> +		    ignore_wake);
> +	return false;
> +}
> +
> +static bool acpi_gpio_irq_is_wake(struct device *parent,
> +				  struct acpi_resource_gpio *agpio)
> +{
> +	int pin = agpio->pin_table[0];
> +
> +	if (agpio->wake_capable != ACPI_WAKE_CAPABLE)
> +		return false;
> +
> +	if (acpi_gpio_in_ignore_list(dev_name(parent), pin)) {
> +		dev_info(parent, "Ignoring wakeup on pin %d\n", pin);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  /* Always returns AE_OK so that we keep looping over the resources */
>  static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
>  					     void *context)
> @@ -289,7 +343,7 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
>  	event->handle = evt_handle;
>  	event->handler = handler;
>  	event->irq = irq;
> -	event->irq_is_wake = honor_wakeup && agpio->wake_capable == ACPI_WAKE_CAPABLE;
> +	event->irq_is_wake = acpi_gpio_irq_is_wake(chip->parent, agpio);
>  	event->pin = pin;
>  	event->desc = desc;
>  
> @@ -1328,7 +1382,9 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "MINIX"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
>  		},
> -		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
> +		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
> +			.no_edge_events_on_boot = true,
> +		},
>  	},
>  	{
>  		/*
> @@ -1341,7 +1397,9 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Wortmann_AG"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "TERRA_PAD_1061"),
>  		},
> -		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
> +		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
> +			.no_edge_events_on_boot = true,
> +		},
>  	},
>  	{
>  		/*
> @@ -1360,33 +1418,31 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "HP x2 Detachable 10-p0XX"),
>  		},
> -		.driver_data = (void *)QUIRK_NO_WAKEUP,
> +		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
> +			.ignore_wake = "INT33FF:01@0,INT0002:00@2",
> +		},
>  	},
>  	{} /* Terminating entry */
>  };
>  
>  static int acpi_gpio_setup_params(void)
>  {
> +	const struct acpi_gpiolib_dmi_quirk *quirk = NULL;
>  	const struct dmi_system_id *id;
> -	long quirks = 0;
>  
>  	id = dmi_first_match(gpiolib_acpi_quirks);
>  	if (id)
> -		quirks = (long)id->driver_data;
> +		quirk = id->driver_data;
>  
>  	if (run_edge_events_on_boot < 0) {
> -		if (quirks & QUIRK_NO_EDGE_EVENTS_ON_BOOT)
> +		if (quirk && quirk->no_edge_events_on_boot)
>  			run_edge_events_on_boot = 0;
>  		else
>  			run_edge_events_on_boot = 1;
>  	}
>  
> -	if (honor_wakeup < 0) {
> -		if (quirks & QUIRK_NO_WAKEUP)
> -			honor_wakeup = 0;
> -		else
> -			honor_wakeup = 1;
> -	}
> +	if (ignore_wake == NULL && quirk && quirk->ignore_wake)
> +		ignore_wake = quirk->ignore_wake;
>  
>  	return 0;
>  }
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko