Re: [PATCH v2] pinmux: Use sequential access to access desc->pinmux data

[Mukesh Ojha <quic_mojha@xxxxxxxxxxx>]

On Sat, Sep 28, 2024 at 01:57:41AM +0530, Wasim Nazir wrote:

Hi Wasim,

Thanks for the review, there looks to be problem with your email client
while replying, please fix.

Please find my reply inline..

> From: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> 
> When two client of the same gpio call pinctrl_select_state() for the
> same functionality, we are seeing NULL pointer issue while accessing
> desc->mux_owner.
> 
> Let's say two processes A, B executing in pin_request() for the same pin
> and process A updates the desc->mux_usecount but not yet updated the
> desc->mux_owner while process B see the desc->mux_usecount which got
> updated by A path and further executes strcmp and while accessing
> desc->mux_owner it crashes with NULL pointer.
> 
> Serialize the access to mux related setting with a spin lock.
> 
> 	cpu0 (process A)			cpu1(process B)
> 
> pinctrl_select_state() {		  pinctrl_select_state() {
>   pin_request() {				pin_request() {
>   ...
> 						 ....
>     } else {
>          desc->mux_usecount++;
>     						desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
> 
>          if (desc->mux_usecount > 1)
>                return 0;
>          desc->mux_owner = owner;
> 
>   }						}
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> ---
> Changes in v2:
>  - Used scoped_guard and renamed lock name as per suggestion from Linus.W .
> 
>  drivers/pinctrl/core.c   |   3 +
>  drivers/pinctrl/core.h   |   2 +
>  drivers/pinctrl/pinmux.c | 150 +++++++++++++++++++++------------------
>  3 files changed, 86 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 4061890a1748..b00911421cf5 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -220,6 +220,9 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
>  
>  	/* Set owner */
>  	pindesc->pctldev = pctldev;
> +#ifdef CONFIG_PINMUX
> +	spin_lock_init(&pindesc->mux_lock);
> +#endif
>  
>  	/* Copy basic pin info */
>  	if (pin->name) {
> diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
> index 4e07707d2435..179e01dfacc2 100644
> --- a/drivers/pinctrl/core.h
> +++ b/drivers/pinctrl/core.h
> @@ -12,6 +12,7 @@
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  #include <linux/radix-tree.h>
> +#include <linux/spinlock.h>
>  #include <linux/types.h>
>  
>  #include <linux/pinctrl/machine.h>
> @@ -177,6 +178,7 @@ struct pin_desc {
>  	const char *mux_owner;
>  	const struct pinctrl_setting_mux *mux_setting;
>  	const char *gpio_owner;
> +	spinlock_t mux_lock;
>  #endif
>  };
>  
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index 02033ea1c643..e4d535aabbb6 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -14,6 +14,7 @@
>  
>  #include <linux/array_size.h>
>  #include <linux/ctype.h>
> +#include <linux/cleanup.h>
>  #include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -127,29 +128,31 @@ static int pin_request(struct pinctrl_dev *pctldev,
>  	dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n",
>  		pin, desc->name, owner);
>  
> -	if ((!gpio_range || ops->strict) &&
> -	    desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
> -		dev_err(pctldev->dev,
> -			"pin %s already requested by %s; cannot claim for %s\n",
> -			desc->name, desc->mux_owner, owner);
> -		goto out;
> -	}
> +	scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> > Any reason to use spinlock_irqsave and not mutex ? If spinlock is needed
> > can we guard only the mux variables and exclude the printk
> > as the same lock is used with pin_show API too.

Good point, using spinlock_irqsave can make things worse and this can
stuck while writing log to console.

I remember now, why i did this,
v3: https://lore.kernel.org/lkml/20231225082305.12343-1-quic_aiquny@xxxxxxxxxxx/

Later same patch in v4 was causing sleep while atomic issue.,
https://lore.kernel.org/lkml/8376074.NyiUUSuA9g@steina-w/

I better be correcting this to mutex here, should also have to increase
the range of this lock to cover mux-setting as well.
> 
> > Moreover, is the mux_usecount variable in pinmux_can_be_used_for_gpio()
> > needed guarding ?

It's a miss, thanks.,

> +		if ((!gpio_range || ops->strict) &&
> +		     desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
> +			dev_err(pctldev->dev,
> +				"pin %s already requested by %s; cannot claim for %s\n",
> +				desc->name, desc->mux_owner, owner);
> +			goto out;
> +		}
>  
> -	if ((gpio_range || ops->strict) && desc->gpio_owner) {
> -		dev_err(pctldev->dev,
> -			"pin %s already requested by %s; cannot claim for %s\n",
> -			desc->name, desc->gpio_owner, owner);
> -		goto out;
> -	}
> +		if ((gpio_range || ops->strict) && desc->gpio_owner) {
> +			dev_err(pctldev->dev,
> +				"pin %s already requested by %s; cannot claim for %s\n",
> +				desc->name, desc->gpio_owner, owner);
> +			goto out;
> +		}
>  
> -	if (gpio_range) {
> -		desc->gpio_owner = owner;
> -	} else {
> -		desc->mux_usecount++;
> -		if (desc->mux_usecount > 1)
> -			return 0;
> +		if (gpio_range) {
> +			desc->gpio_owner = owner;
> +		} else {
> +			desc->mux_usecount++;
> +			if (desc->mux_usecount > 1)
> +				return 0;
>  
> -		desc->mux_owner = owner;
> +			desc->mux_owner = owner;
> +		}
>  	}
>  
>  	/* Let each pin increase references to this module */
> @@ -178,12 +181,14 @@ static int pin_request(struct pinctrl_dev *pctldev,
>  
>  out_free_pin:
>  	if (status) {
> -		if (gpio_range) {
> -			desc->gpio_owner = NULL;
> -		} else {
> -			desc->mux_usecount--;
> -			if (!desc->mux_usecount)
> -				desc->mux_owner = NULL;
> +		scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> +			if (gpio_range) {
> +				desc->gpio_owner = NULL;
> +			} else {
> +				desc->mux_usecount--;
> +				if (!desc->mux_usecount)
> +					desc->mux_owner = NULL;
> +			}
>  		}
>  	}
>  out:
> @@ -223,11 +228,13 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
>  		/*
>  		 * A pin should not be freed more times than allocated.
>  		 */
> -		if (WARN_ON(!desc->mux_usecount))
> -			return NULL;
> -		desc->mux_usecount--;
> -		if (desc->mux_usecount)
> -			return NULL;
> +		scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> +			if (WARN_ON(!desc->mux_usecount))
> +				return NULL;
> +			desc->mux_usecount--;
> +			if (desc->mux_usecount)
> +				return NULL;
> +		}
>  	}
>  
>  	/*
> @@ -239,13 +246,15 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
>  	else if (ops->free)
>  		ops->free(pctldev, pin);
>  
> -	if (gpio_range) {
> -		owner = desc->gpio_owner;
> -		desc->gpio_owner = NULL;
> -	} else {
> -		owner = desc->mux_owner;
> -		desc->mux_owner = NULL;
> -		desc->mux_setting = NULL;
> +	scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> +		if (gpio_range) {
> +			owner = desc->gpio_owner;
> +			desc->gpio_owner = NULL;
> +		} else {
> +			owner = desc->mux_owner;
> +			desc->mux_owner = NULL;
> +			desc->mux_setting = NULL;
> +		}
>  	}
>  
>  	module_put(pctldev->owner);
> @@ -608,40 +617,43 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
>  		if (desc == NULL)
>  			continue;
>  
> -		if (desc->mux_owner &&
> -		    !strcmp(desc->mux_owner, pinctrl_dev_get_name(pctldev)))
> -			is_hog = true;
> -
> -		if (pmxops->strict) {
> -			if (desc->mux_owner)
> -				seq_printf(s, "pin %d (%s): device %s%s",
> -					   pin, desc->name, desc->mux_owner,
> +		scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> +			if (desc->mux_owner &&
> +			    !strcmp(desc->mux_owner, pinctrl_dev_get_name(pctldev)))
> +				is_hog = true;
> +
> +			if (pmxops->strict) {
> +				if (desc->mux_owner)
> +					seq_printf(s, "pin %d (%s): device %s%s",
> +						   pin, desc->name, desc->mux_owner,
> +						   is_hog ? " (HOG)" : "");
> +				else if (desc->gpio_owner)
> +					seq_printf(s, "pin %d (%s): GPIO %s",
> +						   pin, desc->name, desc->gpio_owner);
> +				else
> +					seq_printf(s, "pin %d (%s): UNCLAIMED",
> +						   pin, desc->name);
> +			} else {
> +				/* For non-strict controllers */
> +				seq_printf(s, "pin %d (%s): %s %s%s", pin, desc->name,
> +					   desc->mux_owner ? desc->mux_owner
> +					   : "(MUX UNCLAIMED)",
> +					   desc->gpio_owner ? desc->gpio_owner
> +					   : "(GPIO UNCLAIMED)",
>  					   is_hog ? " (HOG)" : "");
> -			else if (desc->gpio_owner)
> -				seq_printf(s, "pin %d (%s): GPIO %s",
> -					   pin, desc->name, desc->gpio_owner);
> +			}
> +
> +			/* If mux: print function+group claiming the pin */
> +			if (desc->mux_setting)
> +				seq_printf(s, " function %s group %s\n",
> +					   pmxops->get_function_name(pctldev,
> +						desc->mux_setting->func),
> +					   pctlops->get_group_name(pctldev,
> +						desc->mux_setting->group));
>  			else
> -				seq_printf(s, "pin %d (%s): UNCLAIMED",
> -					   pin, desc->name);
> -		} else {
> -			/* For non-strict controllers */
> -			seq_printf(s, "pin %d (%s): %s %s%s", pin, desc->name,
> -				   desc->mux_owner ? desc->mux_owner
> -				   : "(MUX UNCLAIMED)",
> -				   desc->gpio_owner ? desc->gpio_owner
> -				   : "(GPIO UNCLAIMED)",
> -				   is_hog ? " (HOG)" : "");
> -		}
> +				seq_putc(s, '\n');
>  
> -		/* If mux: print function+group claiming the pin */
> -		if (desc->mux_setting)
> -			seq_printf(s, " function %s group %s\n",
> -				   pmxops->get_function_name(pctldev,
> -					desc->mux_setting->func),
> -				   pctlops->get_group_name(pctldev,
> -					desc->mux_setting->group));
> -		else
> -			seq_putc(s, '\n');
> +		}
> > Since we are introducing a lock, do we need to guard mux-settings too ?

Yes, we should, I would need to take care at other places as well.

-Mukesh
>  	}
>  
>  	mutex_unlock(&pctldev->mutex);
> 
> -- 
> 2.34.1
> 
>