Re: [PATCH 18/18] gpio: mvebu: Make use of devm_pwmchip_alloc() function

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2176 bytes --]

Hello Andy,

On Thu, Aug 03, 2023 at 02:51:40PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:42:12AM +0200, Uwe Kleine-König wrote:
> > On Sun, Jul 30, 2023 at 12:07:33PM +0200, Bartosz Golaszewski wrote:
> 
> ...
> 
> >  - the locking scheme in gpiod_request_commit() looks strange. gpio_lock
> >    is released and retaken possibly several times. I wonder what it
> >    actually protects there. Maybe doing
> > 
> > 	diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > 	index edab00c9cb3c..496b1cebba58 100644
> > 	--- a/drivers/gpio/gpiolib.c
> > 	+++ b/drivers/gpio/gpiolib.c
> > 	@@ -2064,13 +2064,11 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> > 				goto out_free_unlock;
> > 			}
> > 		}
> > 	+	spin_unlock_irqrestore(&gpio_lock, flags);
> > 		if (gc->get_direction) {
> > 			/* gc->get_direction may sleep */
> > 	-		spin_unlock_irqrestore(&gpio_lock, flags);
> > 			gpiod_get_direction(desc);
> > 	-		spin_lock_irqsave(&gpio_lock, flags);
> > 		}
> > 	-	spin_unlock_irqrestore(&gpio_lock, flags);
> > 		return 0;
> > 	 
> > 	 out_free_unlock:
> > 
> >    simplifies the code and given that gpiod_get_direction() rechecks
> >    gc->get_direction unlocked I don't think we'd loose anything here.
> 
> Wouldn't this break sleeping bus accesses (I2C gpio expanders, etc)?

This question is too short for me to understand what you think. The
only difference my patch does is that gc->get_direction is checked
without holding the lock and a lock+unlock pair. I don't see how this is
relevant to sleeping bus accesses.

	lock()
	...
	if (A) {
		unlock()
		something()
		lock()
	}
	unlock()

is nearly identical to:

	lock()
	...
	unlock()
	if (A) {
		something()
	}
	lock()
	unlock()

which in turn is nearly identical to

	lock()
	...
	unlock()
	if (A) {
		something()
	}

. But I might well miss something, as the "nearly"s above sometimes are
relevant.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]