lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87d2068b-2644-0307-d722-5539b1f9fb36@redhat.com>
Date:   Thu, 16 Feb 2023 14:07:53 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Orlando Chamberlain <orlandoch.dev@...il.com>
Cc:     Mark Gross <markgross@...nel.org>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        Lukas Wunner <lukas@...ner.de>,
        Seth Forshee <sforshee@...nel.org>,
        Aditya Garg <gargaditya08@...e.com>,
        Aun-Ali Zaidi <admin@...eit.net>,
        Kerem Karabay <kekrby@...il.com>
Subject: Re: [PATCH v2 3/5] apple-gmux: Use GMSP acpi method for interrupt
 clear

Hi Orlando,

Thank you for the new version patches 1 + 2 look good,
one small remark on this one.

On 2/16/23 13:23, Orlando Chamberlain wrote:
> This is needed for interrupts to be cleared correctly on MMIO based
> gmux's. It is untested if this helps/hinders other gmux types, so
> currently this is only enabled for the MMIO gmux's.
> 
> There is also a "GMLV" acpi method, and the "GMSP" method can be called
> with 1 as its argument, but the purposes of these aren't known and they
> don't seem to be needed.
> 
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@...il.com>
> ---
> v1->v2: Only enable this on MMIO gmux's
>  drivers/platform/x86/apple-gmux.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 36208e93d745..12a93fc49c36 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -76,6 +76,7 @@ struct apple_gmux_config {
>  	enum vga_switcheroo_handler_flags_t handler_flags;
>  	unsigned long resource_type;
>  	bool read_version_as_u32;
> +	bool use_acpi_gmsp;
>  	char *name;
>  };
>  
> @@ -488,6 +489,7 @@ static const struct apple_gmux_config apple_gmux_pio = {
>  	.handler_flags = VGA_SWITCHEROO_CAN_SWITCH_DDC,
>  	.resource_type = IORESOURCE_IO,
>  	.read_version_as_u32 = false,
> +	.use_acpi_gmsp = false,
>  	.name = "classic"
>  };
>  
> @@ -500,6 +502,7 @@ static const struct apple_gmux_config apple_gmux_index = {
>  	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
>  	.resource_type = IORESOURCE_IO,
>  	.read_version_as_u32 = true,
> +	.use_acpi_gmsp = false,
>  	.name = "indexed"
>  };
>  
> @@ -511,8 +514,29 @@ static const struct apple_gmux_config apple_gmux_index = {
>   * MCP79, on all following generations it's GPIO pin 6 of the Intel PCH.
>   * The GPE merely signals that an interrupt occurred, the actual type of event
>   * is identified by reading a gmux register.
> + *
> + * On MMIO gmux's, we also need to call the acpi method GMSP to properly clear
> + * interrupts.
>   */
>  
> +static int gmux_call_acpi_gmsp(struct apple_gmux_data *gmux_data, int arg)
> +{
> +	acpi_status status = AE_OK;
> +	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> +	struct acpi_object_list arg_list = { 1, &arg0 };
> +
> +	arg0.integer.value = arg;
> +
> +	status = acpi_evaluate_object(gmux_data->dhandle, "GMSP", &arg_list, NULL);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("GMSP call failed: %s\n",
> +		       acpi_format_exception(status));
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  static inline void gmux_disable_interrupts(struct apple_gmux_data *gmux_data)
>  {
>  	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_ENABLE,
> @@ -536,7 +560,11 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
>  
>  	/* to clear interrupts write back current status */
>  	status = gmux_interrupt_get_status(gmux_data);
> -	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> +	if (status) {
> +		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> +		if (gmux_data->config->use_acpi_gmsp)
> +			gmux_call_acpi_gmsp(gmux_data, 0);
> +	}

This changes the behavior on the existing supported models to
only write back status when it is non 0. This is likely fine
but given that we seem to lack testers for the old models
I would prefer to not change the behavior there.

So how about:

	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
	if (status && gmux_data->config->use_acpi_gmsp)
		gmux_call_acpi_gmsp(gmux_data, 0);

?

The 0 write to what presumably is a register with
write 1 to clear bits should be harmless.

You can test that it is harmless on the new MMIO models
and this way we don't change the behavior on the older models.

Regards,

Hans

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ