linux-kernel - Re: [PATCH v4 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC

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]

Date:   Thu, 7 Nov 2019 17:36:08 +0800
From:   "Tanwar, Rahul" <rahul.tanwar@...ux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     linus.walleij@...aro.org, robh+dt@...nel.org, mark.rutland@....com,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, robh@...nel.org, qi-ming.wu@...el.com,
        yixin.zhu@...ux.intel.com, cheol.yong.kim@...el.com
Subject: Re: [PATCH v4 1/2] pinctrl: Add pinmux & GPIO controller driver for a
 new SoC


Hi Andy,

On 7/11/2019 5:07 PM, Andy Shevchenko wrote:
> On Thu, Nov 07, 2019 at 03:36:44PM +0800, Rahul Tanwar wrote:
> +static void eqbr_gpio_mask_ack_irq(struct irq_data *d)
>> +{
>> +	eqbr_gpio_disable_irq(d);
>> +	eqbr_gpio_ack_irq(d);
> Potential race?
>
>> +}
>> +static int eqbr_pinmux_set_mux(struct pinctrl_dev *pctldev,
>> +			       unsigned int selector, unsigned int group)
>> +{
>> +	struct eqbr_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev);
>> +	struct function_desc *func;
>> +	struct group_desc *grp;
>> +	unsigned int *pinmux;
>> +	int i;
>> +
>> +	func = pinmux_generic_get_function(pctldev, selector);
>> +	if (!func)
>> +		return -EINVAL;
>> +
>> +	grp = pinctrl_generic_get_group(pctldev, group);
>> +	if (!grp)
>> +		return -EINVAL;
>> +
>> +	pinmux = grp->data;
>> +	for (i = 0; i < grp->num_pins; i++)
>> +		eqbr_set_pin_mux(pctl, pinmux[i], grp->pins[i]);
> What if in the middle of the loop mux of one of the pins be changed by parallel
> thread?

These are all ops called back from the core pinctrl framework.
My understanding is that multi-threading serialization is provided by the
pinctrl framework using mutex's. Drivers don't have to worry about that.
Drivers only have to worry about multi-core serialization. I checked
many other existing pinctrl drivers & all of them seem to use spin lock
only for register accesses (not for serializing the ops itself).

Is this understanding incorrect ?


>
>> +	/* 0 mux is reserved for GPIO */
> Perhaps
>
> #define EQBR_GPIO_MODE	0
>
> ?

I had first used #define for this but removed it based on Rob Herring
review feedback. I will add it back here but keep it as 0 in dt bindings..

>> +	return eqbr_set_pin_mux(pctl, 0, pin);
>> +}
>> +	for (i = 0; i < npins; i++) {
>> +		ret = eqbr_pinconf_set(pctldev, pins[i], configs, num_configs);
>> +		if (ret)
>> +			return ret;
> What if in the middle of the loop settings of one of the pins be changed by
> parallel thread?

Same comments as above..

>
>> +		group.pins = devm_kcalloc(dev, group.num_pins,
>> +					  sizeof(*(group.pins)), GFP_KERNEL);
>> +		pinmux = devm_kcalloc(dev, group.num_pins,
>> +				      sizeof(*pinmux), GFP_KERNEL);
> These can be rearranged.

Lost you here. Please elaborate more on what you mean by rearranging. Thanks.

Regards,
Rahul