linux-kernel - Re: [PATCH v2] Skip deferred request irqs for devices known to fail

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: <20190819115218.GY30120@smile.fi.intel.com>
Date:   Mon, 19 Aug 2019 14:52:18 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Ian W MORRISON <ianwmorrison@...il.com>
Cc:     benjamin.tissoires@...hat.com, hdegoede@...hat.com,
        mika.westerberg@...ux.intel.com, linus.walleij@...aro.org,
        bgolaszewski@...libre.com, linux-gpio@...r.kernel.org,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH v2] Skip deferred request irqs for devices known to fail

On Mon, Aug 19, 2019 at 09:26:37PM +1000, Ian W MORRISON wrote:
> Patch ca876c7483b6 "gpiolib-acpi: make sure we trigger edge events at
> least once on boot" causes the MINIX family of mini PCs to fail to boot
> resulting in a "black screen".
> 
> This patch excludes MINIX devices from executing this trigger in order
> to successfully boot.

Thanks for an update.

> Cc: stable@...r.kernel.org
> Signed-off-by: Ian W MORRISON <ianwmorrison@...il.com>
> Reviewed-by: Hans de Goede <hdegoede@...hat.com>

> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

Hmm... Did I really give the tag?
Too many stuff is going on, anyway, please consider more comments below.

First of all, the subject should start from "gpiolib: acpi: " prefix.

Then, Fixes tag seems to be missed.

> +/*
> + * Run deferred acpi_gpiochip_request_irqs()
> + * but exclude devices known to fail

Missed period.

> +*/

Missed leading space (the column of stars).

>  static int acpi_gpio_handle_deferred_request_irqs(void)
>  {
>  	struct acpi_gpio_chip *acpi_gpio, *tmp;
> +	const struct dmi_system_id *dmi_id;
>  
> -	mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> -	list_for_each_entry_safe(acpi_gpio, tmp,
> +	dmi_id = dmi_first_match(skip_deferred_request_irqs_table);
> +	if (dmi_id)
> +		return 0;

The idea of positive check is exactly for...

> +	else {

...getting rid of this redundant 'else' followed by unneeded level of indentation.

> +		mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> +		list_for_each_entry_safe(acpi_gpio, tmp,
>  				 &acpi_gpio_deferred_req_irqs_list,
>  				 deferred_req_irqs_list_entry)
> -		acpi_gpiochip_request_irqs(acpi_gpio);
> +			acpi_gpiochip_request_irqs(acpi_gpio);
>  
> -	acpi_gpio_deferred_req_irqs_done = true;
> -	mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> +		acpi_gpio_deferred_req_irqs_done = true;
> +		mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> +	}
>  
>  	return 0;
>  }

-- 
With Best Regards,
Andy Shevchenko