Re: [PATCH] pcmcia: cs: Replace manual mutex locking with guard(mutex)

[Dominik Brodowski]

Am Wed, Jun 03, 2026 at 01:27:12AM -0500 schrieb Maxwell Doose:
> The current code uses manual mutex locking. Replace it with the RAII
> guard(mutex) alongside some helpers to help condense some sections and
> increase readability.

As the PCMCIA subsystem is in "odd fixes" mode, I don't see real advantages
for this patch.

Thanks,
	Dominik


> Signed-off-by: Maxwell Doose <m32285159@xxxxxxxxx>
> ---
>  drivers/pcmcia/cs.c | 208 ++++++++++++++++++++++----------------------
>  1 file changed, 104 insertions(+), 104 deletions(-)
> 
> diff --git a/drivers/pcmcia/cs.c b/drivers/pcmcia/cs.c
> index adbc486af2ea..200167eb863f 100644
> --- a/drivers/pcmcia/cs.c
> +++ b/drivers/pcmcia/cs.c
> @@ -159,9 +159,8 @@ int pcmcia_register_socket(struct pcmcia_socket *socket)
>  	spin_lock_init(&socket->thread_lock);
>  
>  	if (socket->resource_ops->init) {
> -		mutex_lock(&socket->ops_mutex);
> -		ret = socket->resource_ops->init(socket);
> -		mutex_unlock(&socket->ops_mutex);
> +		scoped_guard(mutex, &socket->ops_mutex)
> +			ret = socket->resource_ops->init(socket);
>  		if (ret)
>  			goto err;
>  	}
> @@ -220,9 +219,9 @@ void pcmcia_unregister_socket(struct pcmcia_socket *socket)
>  
>  	/* wait for sysfs to drop all references */
>  	if (socket->resource_ops->exit) {
> -		mutex_lock(&socket->ops_mutex);
> +		guard(mutex)(&socket->ops_mutex);
> +
>  		socket->resource_ops->exit(socket);
> -		mutex_unlock(&socket->ops_mutex);
>  	}
>  	wait_for_completion(&socket->socket_released);
>  } /* pcmcia_unregister_socket */
> @@ -259,6 +258,24 @@ static int socket_reset(struct pcmcia_socket *skt)
>  	return -ETIMEDOUT;
>  }
>  
> +static void socket_shutdown_helper(struct pcmcia_socket *s)
> +{
> +	guard(mutex)(&s->ops_mutex);
> +
> +	s->state &= SOCKET_INUSE | SOCKET_PRESENT;
> +	msleep(shutdown_delay * 10);
> +	s->state &= SOCKET_INUSE;
> +
> +	/* Blank out the socket state */
> +	s->socket = dead_socket;
> +	s->ops->init(s);
> +	s->ops->set_socket(s, &s->socket);
> +	s->lock_count = 0;
> +	kfree(s->fake_cis);
> +	s->fake_cis = NULL;
> +	s->functions = 0;
> +}
> +
>  /*
>   * socket_setup() and socket_shutdown() are called by the main event handler
>   * when card insertion and removal events are received.
> @@ -274,27 +291,13 @@ static void socket_shutdown(struct pcmcia_socket *s)
>  	if (s->callback)
>  		s->callback->remove(s);
>  
> -	mutex_lock(&s->ops_mutex);
> -	s->state &= SOCKET_INUSE | SOCKET_PRESENT;
> -	msleep(shutdown_delay * 10);
> -	s->state &= SOCKET_INUSE;
> -
> -	/* Blank out the socket state */
> -	s->socket = dead_socket;
> -	s->ops->init(s);
> -	s->ops->set_socket(s, &s->socket);
> -	s->lock_count = 0;
> -	kfree(s->fake_cis);
> -	s->fake_cis = NULL;
> -	s->functions = 0;
> -
>  	/* From here on we can be sure that only we (that is, the
>  	 * pccardd thread) accesses this socket, and all (16-bit)
>  	 * PCMCIA interactions are gone. Therefore, release
>  	 * ops_mutex so that we don't get a sysfs-related lockdep
>  	 * warning.
>  	 */
> -	mutex_unlock(&s->ops_mutex);
> +	socket_shutdown_helper(s);
>  
>  #ifdef CONFIG_CARDBUS
>  	cb_free(s);
> @@ -396,6 +399,10 @@ static int socket_insert(struct pcmcia_socket *skt)
>  
>  	dev_dbg(&skt->dev, "insert\n");
>  
> +	/*
> +	 * NOTE: Converting to guard(mutex) may require refactoring some of this
> +	 * code so not converted.
> +	 */
>  	mutex_lock(&skt->ops_mutex);
>  	if (skt->state & SOCKET_INUSE) {
>  		mutex_unlock(&skt->ops_mutex);
> @@ -435,7 +442,7 @@ static int socket_suspend(struct pcmcia_socket *skt)
>  	if ((skt->state & SOCKET_SUSPEND) && !(skt->state & SOCKET_IN_RESUME))
>  		return -EBUSY;
>  
> -	mutex_lock(&skt->ops_mutex);
> +	guard(mutex)(&skt->ops_mutex);
>  	/* store state on first suspend, but not after spurious wakeups */
>  	if (!(skt->state & SOCKET_IN_RESUME))
>  		skt->suspended_state = skt->state;
> @@ -446,20 +453,21 @@ static int socket_suspend(struct pcmcia_socket *skt)
>  		skt->ops->suspend(skt);
>  	skt->state |= SOCKET_SUSPEND;
>  	skt->state &= ~SOCKET_IN_RESUME;
> -	mutex_unlock(&skt->ops_mutex);
> +
>  	return 0;
>  }
>  
>  static int socket_early_resume(struct pcmcia_socket *skt)
>  {
> -	mutex_lock(&skt->ops_mutex);
> +	guard(mutex)(&skt->ops_mutex);
> +
>  	skt->socket = dead_socket;
>  	skt->ops->init(skt);
>  	skt->ops->set_socket(skt, &skt->socket);
>  	if (skt->state & SOCKET_PRESENT)
>  		skt->resume_status = socket_setup(skt, resume_delay);
>  	skt->state |= SOCKET_IN_RESUME;
> -	mutex_unlock(&skt->ops_mutex);
> +
>  	return 0;
>  }
>  
> @@ -467,9 +475,8 @@ static int socket_late_resume(struct pcmcia_socket *skt)
>  {
>  	int ret = 0;
>  
> -	mutex_lock(&skt->ops_mutex);
> -	skt->state &= ~(SOCKET_SUSPEND | SOCKET_IN_RESUME);
> -	mutex_unlock(&skt->ops_mutex);
> +	scoped_guard(mutex, &skt->ops_mutex)
> +		skt->state &= ~(SOCKET_SUSPEND | SOCKET_IN_RESUME);
>  
>  	if (!(skt->state & SOCKET_PRESENT)) {
>  		ret = socket_insert(skt);
> @@ -572,6 +579,48 @@ static void socket_detect_change(struct pcmcia_socket *skt)
>  	}
>  }
>  
> +static int pccardd_helper(struct pcmcia_socket *skt, unsigned int events,
> +			  unsigned int sysfs_events)
> +{
> +	int ret;
> +
> +	guard(mutex)(&skt->skt_mutex);
> +
> +	if (events & SS_DETECT)
> +		socket_detect_change(skt);
> +
> +	if (sysfs_events) {
> +		if (sysfs_events & PCMCIA_UEVENT_EJECT)
> +			socket_remove(skt);
> +		if (sysfs_events & PCMCIA_UEVENT_INSERT)
> +			socket_insert(skt);
> +		if ((sysfs_events & PCMCIA_UEVENT_SUSPEND) &&
> +		    !(skt->state & SOCKET_CARDBUS)) {
> +			if (skt->callback)
> +				ret = skt->callback->suspend(skt);
> +			else
> +				ret = 0;
> +			if (!ret) {
> +				socket_suspend(skt);
> +				msleep(100);
> +			}
> +		}
> +		if ((sysfs_events & PCMCIA_UEVENT_RESUME) &&
> +		    !(skt->state & SOCKET_CARDBUS)) {
> +			ret = socket_resume(skt);
> +			if (!ret && skt->callback)
> +				skt->callback->resume(skt);
> +		}
> +		if ((sysfs_events & PCMCIA_UEVENT_REQUERY) &&
> +		    !(skt->state & SOCKET_CARDBUS)) {
> +			if (!ret && skt->callback)
> +				skt->callback->requery(skt);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int pccardd(void *__skt)
>  {
>  	struct pcmcia_socket *skt = __skt;
> @@ -613,39 +662,7 @@ static int pccardd(void *__skt)
>  		skt->sysfs_events = 0;
>  		spin_unlock_irqrestore(&skt->thread_lock, flags);
>  
> -		mutex_lock(&skt->skt_mutex);
> -		if (events & SS_DETECT)
> -			socket_detect_change(skt);
> -
> -		if (sysfs_events) {
> -			if (sysfs_events & PCMCIA_UEVENT_EJECT)
> -				socket_remove(skt);
> -			if (sysfs_events & PCMCIA_UEVENT_INSERT)
> -				socket_insert(skt);
> -			if ((sysfs_events & PCMCIA_UEVENT_SUSPEND) &&
> -				!(skt->state & SOCKET_CARDBUS)) {
> -				if (skt->callback)
> -					ret = skt->callback->suspend(skt);
> -				else
> -					ret = 0;
> -				if (!ret) {
> -					socket_suspend(skt);
> -					msleep(100);
> -				}
> -			}
> -			if ((sysfs_events & PCMCIA_UEVENT_RESUME) &&
> -				!(skt->state & SOCKET_CARDBUS)) {
> -				ret = socket_resume(skt);
> -				if (!ret && skt->callback)
> -					skt->callback->resume(skt);
> -			}
> -			if ((sysfs_events & PCMCIA_UEVENT_REQUERY) &&
> -				!(skt->state & SOCKET_CARDBUS)) {
> -				if (!ret && skt->callback)
> -					skt->callback->requery(skt);
> -			}
> -		}
> -		mutex_unlock(&skt->skt_mutex);
> +		ret = pccardd_helper(skt, events, sysfs_events);
>  
>  		if (events || sysfs_events)
>  			continue;
> @@ -663,9 +680,9 @@ static int pccardd(void *__skt)
>  
>  	/* shut down socket, if a device is still present */
>  	if (skt->state & SOCKET_PRESENT) {
> -		mutex_lock(&skt->skt_mutex);
> +		guard(mutex)(&skt->skt_mutex);
> +
>  		socket_remove(skt);
> -		mutex_unlock(&skt->skt_mutex);
>  	}
>  
>  	/* remove from the device core */
> @@ -722,17 +739,13 @@ EXPORT_SYMBOL(pcmcia_parse_uevents);
>  /* register pcmcia_callback */
>  int pccard_register_pcmcia(struct pcmcia_socket *s, struct pcmcia_callback *c)
>  {
> -	int ret = 0;
> -
>  	/* s->skt_mutex also protects s->callback */
> -	mutex_lock(&s->skt_mutex);
> +	guard(mutex)(&s->skt_mutex);
>  
>  	if (c) {
>  		/* registration */
> -		if (s->callback) {
> -			ret = -EBUSY;
> -			goto err;
> -		}
> +		if (s->callback)
> +			return -EBUSY;
>  
>  		s->callback = c;
>  
> @@ -740,10 +753,8 @@ int pccard_register_pcmcia(struct pcmcia_socket *s, struct pcmcia_callback *c)
>  			s->callback->add(s);
>  	} else
>  		s->callback = NULL;
> - err:
> -	mutex_unlock(&s->skt_mutex);
>  
> -	return ret;
> +	return 0;
>  }
>  EXPORT_SYMBOL(pccard_register_pcmcia);
>  
> @@ -759,35 +770,27 @@ int pcmcia_reset_card(struct pcmcia_socket *skt)
>  
>  	dev_dbg(&skt->dev, "resetting socket\n");
>  
> -	mutex_lock(&skt->skt_mutex);
> -	do {
> -		if (!(skt->state & SOCKET_PRESENT)) {
> -			dev_dbg(&skt->dev, "can't reset, not present\n");
> -			ret = -ENODEV;
> -			break;
> -		}
> -		if (skt->state & SOCKET_SUSPEND) {
> -			dev_dbg(&skt->dev, "can't reset, suspended\n");
> -			ret = -EBUSY;
> -			break;
> -		}
> -		if (skt->state & SOCKET_CARDBUS) {
> -			dev_dbg(&skt->dev, "can't reset, is cardbus\n");
> -			ret = -EPERM;
> -			break;
> -		}
> +	guard(mutex)(&skt->skt_mutex);
>  
> -		if (skt->callback)
> -			skt->callback->suspend(skt);
> -		mutex_lock(&skt->ops_mutex);
> +	if (!(skt->state & SOCKET_PRESENT)) {
> +		dev_dbg(&skt->dev, "can't reset, not present\n");
> +		return -ENODEV;
> +	}
> +	if (skt->state & SOCKET_SUSPEND) {
> +		dev_dbg(&skt->dev, "can't reset, suspended\n");
> +		return -EBUSY;
> +	}
> +	if (skt->state & SOCKET_CARDBUS) {
> +		dev_dbg(&skt->dev, "can't reset, is cardbus\n");
> +		return -EPERM;
> +	}
> +
> +	if (skt->callback)
> +		skt->callback->suspend(skt);
> +	scoped_guard(mutex, &skt->ops_mutex)
>  		ret = socket_reset(skt);
> -		mutex_unlock(&skt->ops_mutex);
> -		if ((ret == 0) && (skt->callback))
> -			skt->callback->resume(skt);
> -
> -		ret = 0;
> -	} while (0);
> -	mutex_unlock(&skt->skt_mutex);
> +	if ((ret == 0) && (skt->callback))
> +		skt->callback->resume(skt);
>  
>  	return ret;
>  } /* reset_card */
> @@ -820,13 +823,10 @@ static int __pcmcia_pm_op(struct device *dev,
>  			  int (*callback) (struct pcmcia_socket *skt))
>  {
>  	struct pcmcia_socket *s = container_of(dev, struct pcmcia_socket, dev);
> -	int ret;
>  
> -	mutex_lock(&s->skt_mutex);
> -	ret = callback(s);
> -	mutex_unlock(&s->skt_mutex);
> +	guard(mutex)(&s->skt_mutex);
>  
> -	return ret;
> +	return callback(s);
>  }
>  
>  static int pcmcia_socket_dev_suspend_noirq(struct device *dev)
> -- 
> 2.54.0
> 
>