linux-kernel - Re: [PATCH v9 2/9] Input: goodix

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] [day] [month] [year] [list]

Message-ID: <20151102101733.GB1509@lahna.fi.intel.com>
Date:	Mon, 2 Nov 2015 12:17:33 +0200
From:	"mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	"Tirdea, Irina" <irina.tirdea@...el.com>,
	Bastien Nocera <hadess@...ess.net>,
	Aleksei Mamlin <mamlinav@...il.com>,
	Karsten Merker <merker@...ian.org>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	Mark Rutland <mark.rutland@....com>,
	"Purdila, Octavian" <octavian.purdila@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"Dolca, Robert" <robert.dolca@...el.com>
Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init

On Fri, Oct 30, 2015 at 09:33:28AM -0700, Dmitry Torokhov wrote:
> cpi_dev_add_driver_gpios() does not really help with generic drivers
> (unless we keep adding more and more board specific data to them). How
> about we keep track of names used and only allow conversion for the
> first name used, like in the patch below?

That works but it misses one case: When you have optional GPIOs and not
all of them are provided. Currently it ends up the same situation
returning the same GPIO.

I've commented below how we could handle that case as well.

> -- 
> Dmitry
> 
> gpiolib: tighten up ACPI legacy gpio lookups
> 
> From: Dmitry Torokhov <dmitry.torokhov@...il.com>
> 
> We should not fall back to the legacy unnamed gpio lookup style if the
> driver requests gpios with different names, because we'll give out the same
> gpio twice. Let's keep track of the names that were used for the device and
> only do the fallback for the first name used.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> ---
>  drivers/gpio/gpiolib.c |   42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5db3445..4ae5447 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1738,6 +1738,45 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>  	return desc;
>  }
>  
> +struct acpi_gpio_lookup {
> +	struct list_head node;
> +	struct device *dev;
> +	const char *con_id;
> +};
> +
> +static DEFINE_MUTEX(acpi_gpio_lookup_lock);
> +static LIST_HEAD(acpi_gpio_lookup_list);
> +
> +static bool acpi_can_fallback_crs(struct device *dev, const char *con_id)
> +{
> +	struct acpi_gpio_lookup *l, *lookup = NULL;

I'm thinking if we here do

	struct acpi_device *adev = ACPI_COMPANION(dev);

	/* Never fallback if the device has properties */
	if (adev->data.properties || adev->driver_gpios)
		return false;

> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	list_for_each_entry(l, &acpi_gpio_lookup_list, node) {
> +		if (l->dev == dev) {
> +			lookup = l;
> +			break;
> +		}
> +	}
> +
> +	if (!lookup) {
> +		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
> +		if (lookup) {
> +			lookup->dev = dev;
> +			lookup->con_id = con_id;
> +			list_add_tail(&lookup->node, &acpi_gpio_lookup_list);
> +		}
> +	}
> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	return lookup &&
> +		((!lookup->con_id && !con_id) ||
> +		 (lookup->con_id && con_id &&
> +		  strcmp(lookup->con_id, con_id) == 0));
> +}
> +
>  static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  					unsigned int idx,
>  					enum gpio_lookup_flags *flags)
> @@ -1765,7 +1804,8 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  
>  	/* Then from plain _CRS GPIOs */
>  	if (IS_ERR(desc)) {
> -		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
> +		if (acpi_can_fallback_crs(dev, con_id))
> +			desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);

and here we could do

		if (!acpi_can_fallback_crs(dev, con_id))
			return ERR_PTR(-ENOENT);
		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);

So instead return -ENOENT so that gpiod_get_index_optional() handles the
missing optional GPIO properly.

>  		if (IS_ERR(desc))
>  			return desc;
>  	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/