Re: [PATCH 1/1] platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 5/12/21 2:55 PM, Hans de Goede wrote:
> Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement
> irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to
> the parents IRQ because this was breaking suspend (causing immediate
> wakeups) on an Asus E202SA.
> 
> This workaround for the Asus E202SA is causing wakeup by USB keyboard to
> not work on other devices with Airmont CPU cores such as the Medion Akoya
> E1239T. In hindsight the problem with the Asus E202SA has nothing to do
> with Silvermont vs Airmont CPU cores, so the differentiation between the
> 2 types of CPU cores introduced by the previous fix is wrong.
> 
> The real issue at hand is s2idle vs S3 suspend where the suspend is
> mostly handled by firmware. The parent IRQ for the INT0002 device is shared
> with the ACPI SCI and the real problem is that the INT0002 code should not
> be messing with the wakeup settings of that IRQ when suspend/resume is
> being handled by the firmware.
> 
> Note that on systems which support both s2idle and S3 suspend, which
> suspend method to use can be changed at runtime.
> 
> This patch fixes both the Asus E202SA spurious wakeups issue as well as
> the wakeup by USB keyboard not working on the Medion Akoya E1239T issue.
> 
> These are both fixed by replacing the old workaround with delaying the
> enable_irq_wake(parent_irq) call till system-suspend time and protecting
> it with a !pm_suspend_via_firmware() check so that we still do not call
> it on devices using firmware-based (S3) suspend such as the Asus E202SA.
> 
> Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds
> a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose
> is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes
> no sense.
> 
> Cc: Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail")
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>

I've added this to my review-hans branch now and I will also include this
in the next pdx86-fixes pull-req for 5.13.

Regards,

Hans



> ---
>  drivers/platform/x86/Kconfig               |  2 +-
>  drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++-------
>  2 files changed, 57 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 4b67e74a747b..c2f608d5f1b7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -713,7 +713,7 @@ config INTEL_HID_EVENT
>  
>  config INTEL_INT0002_VGPIO
>  	tristate "Intel ACPI INT0002 Virtual GPIO driver"
> -	depends on GPIOLIB && ACPI
> +	depends on GPIOLIB && ACPI && PM_SLEEP
>  	select GPIOLIB_IRQCHIP
>  	help
>  	  Some peripherals on Bay Trail and Cherry Trail platforms signal a
> diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c
> index 289c6655d425..569342aa8926 100644
> --- a/drivers/platform/x86/intel_int0002_vgpio.c
> +++ b/drivers/platform/x86/intel_int0002_vgpio.c
> @@ -51,6 +51,12 @@
>  #define GPE0A_STS_PORT			0x420
>  #define GPE0A_EN_PORT			0x428
>  
> +struct int0002_data {
> +	struct gpio_chip chip;
> +	int parent_irq;
> +	int wake_enable_count;
> +};
> +
>  /*
>   * As this is not a real GPIO at all, but just a hack to model an event in
>   * ACPI the get / set functions are dummy functions.
> @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data)
>  static int int0002_irq_set_wake(struct irq_data *data, unsigned int on)
>  {
>  	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> -	struct platform_device *pdev = to_platform_device(chip->parent);
> -	int irq = platform_get_irq(pdev, 0);
> +	struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip);
>  
> -	/* Propagate to parent irq */
> +	/*
> +	 * Applying of the wakeup flag to our parent IRQ is delayed till system
> +	 * suspend, because we only want to do this when using s2idle.
> +	 */
>  	if (on)
> -		enable_irq_wake(irq);
> +		int0002->wake_enable_count++;
>  	else
> -		disable_irq_wake(irq);
> +		int0002->wake_enable_count--;
>  
>  	return 0;
>  }
> @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data)
>  	return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT);
>  }
>  
> -static struct irq_chip int0002_byt_irqchip = {
> +static struct irq_chip int0002_irqchip = {
>  	.name			= DRV_NAME,
>  	.irq_ack		= int0002_irq_ack,
>  	.irq_mask		= int0002_irq_mask,
> @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = {
>  	.irq_set_wake		= int0002_irq_set_wake,
>  };
>  
> -static struct irq_chip int0002_cht_irqchip = {
> -	.name			= DRV_NAME,
> -	.irq_ack		= int0002_irq_ack,
> -	.irq_mask		= int0002_irq_mask,
> -	.irq_unmask		= int0002_irq_unmask,
> -	/*
> -	 * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI
> -	 * and we don't want to mess with the ACPI SCI irq settings.
> -	 */
> -	.flags			= IRQCHIP_SKIP_SET_WAKE,
> -};
> -
>  static const struct x86_cpu_id int0002_cpu_ids[] = {
> -	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT,	&int0002_byt_irqchip),
> -	X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT,	&int0002_cht_irqchip),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL),
>  	{}
>  };
>  
> @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	const struct x86_cpu_id *cpu_id;
> -	struct gpio_chip *chip;
> +	struct int0002_data *int0002;
>  	struct gpio_irq_chip *girq;
> +	struct gpio_chip *chip;
>  	int irq, ret;
>  
>  	/* Menlow has a different INT0002 device? <sigh> */
> @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return irq;
>  
> -	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> -	if (!chip)
> +	int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL);
> +	if (!int0002)
>  		return -ENOMEM;
>  
> +	int0002->parent_irq = irq;
> +
> +	chip = &int0002->chip;
>  	chip->label = DRV_NAME;
>  	chip->parent = dev;
>  	chip->owner = THIS_MODULE;
> @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev)
>  	}
>  
>  	girq = &chip->irq;
> -	girq->chip = (struct irq_chip *)cpu_id->driver_data;
> +	girq->chip = &int0002_irqchip;
>  	/* This let us handle the parent IRQ in the driver */
>  	girq->parent_handler = NULL;
>  	girq->num_parents = 0;
> @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev)
>  
>  	acpi_register_wakeup_handler(irq, int0002_check_wake, NULL);
>  	device_init_wakeup(dev, true);
> +	dev_set_drvdata(dev, int0002);
>  	return 0;
>  }
>  
> @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int int0002_suspend(struct device *dev)
> +{
> +	struct int0002_data *int0002 = dev_get_drvdata(dev);
> +
> +	/*
> +	 * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't
> +	 * muck with it when firmware based suspend is used, otherwise we may
> +	 * cause spurious wakeups from firmware managed suspend.
> +	 */
> +	if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
> +		enable_irq_wake(int0002->parent_irq);
> +
> +	return 0;
> +}
> +
> +static int int0002_resume(struct device *dev)
> +{
> +	struct int0002_data *int0002 = dev_get_drvdata(dev);
> +
> +	if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
> +		disable_irq_wake(int0002->parent_irq);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops int0002_pm_ops = {
> +	.suspend = int0002_suspend,
> +	.resume = int0002_resume,
> +};
> +
>  static const struct acpi_device_id int0002_acpi_ids[] = {
>  	{ "INT0002", 0 },
>  	{ },
> @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = {
>  	.driver = {
>  		.name			= DRV_NAME,
>  		.acpi_match_table	= int0002_acpi_ids,
> +		.pm			= &int0002_pm_ops,
>  	},
>  	.probe	= int0002_probe,
>  	.remove	= int0002_remove,
> 




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux