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: 4600 bytes --]

Hello Bart,

On Sun, Jul 30, 2023 at 12:07:33PM +0200, Bartosz Golaszewski wrote:
> On Sat, Jul 29, 2023 at 11:37 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello Bartosz,
> >
> > On Sat, Jul 29, 2023 at 04:09:40PM +0200, Bartosz Golaszewski wrote:
> > > Looks good to me (although I have my reservations about the concept of
> > > foo_alloc() for subsystems in the kernel...).
> >
> > Wolfram's EOSS talk[1] mentioned "__cleanup__ + kref as suggested by Bartosz?
> > Paradigm shift, probably looong way to go". I guess that's what you'd
> > prefer? Do you have a link for me to read about this?
> >
> 
> For now I prefer the gpiolib model. One structure allocated and
> controlled by the driver (struct gpio_chip) which needs to live only
> as long as the device is bound to a driver and a second structure
> private to the subsystem, allocated and controlled by the subsystem
> (struct gpio_device) which also contains the referenced counted struct
> device and is only released by the device's release callback.

The issue I want to fix for pwm (but don't know yet how to do) is: What
should happen to PWMs that are requested by a consumer when the PWM
driver goes away.

I looked into how gpio does it, and I think the "solution" there is:

	dev_crit(&gdev->dev,
		 "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");

introduced in e1db1706c86e ("gpio: gpiolib: set gpiochip_remove retval
to void"). (But the problem is actually older because returning -EBUSY
as done before is bad, too) I'd hope this could be done better?!

While trying to understand how gpio works, I found a few issues that are
(I think) fixable with the gpiolib model:

 - gpiochip_add_data_with_key() calls device_initialize(&gdev->dev) and
   has later error paths that don't do device_put() but kfree gdev.

 - 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.

 - there is a race condition: gpiochip_remove() takes &gdev->sem when
   invalidating gdev->chip and calling gpiochip_set_data(), but the
   various gpio API functions calling VALIDATE_DESC_VOID don't hold
   &gdev->sem, so gpiochip_remove() might clean gdev->chip just between
   a consumer calling VALIDATE_DESC_VOID(desc) and
   WARN_ON(desc->gdev->chip->can_sleep) (e.g. in gpiod_set_value).

> IMO there shouldn't be any need for PWM drivers to dereference struct
> device held by struct pwm_chip. If anything - it should be passed to
> the drivers in subsystem callbacks.

I don't understand this. I think we agree that a PWM driver shouldn't
have to care about the devices's lifetimes. It's difficult enough to get
this right on the subsystem level.

> I may be wrong of course, I don't know this subsystem very well but it
> seems to follow a pattern that's pretty common in the kernel and
> causes ownership confusion.

Yes that's common. I think another thing that's common though is that
device lifetime isn't properly handled, and while I don't consider
myself as an expert here, the above makes me consider that gpio is no
exception here. So I doubt it serves as a good example to copy from.

Having said that I think the ..._alloc approach is easy enough for
subsystem drivers. Also for pwm we only need a devm_... variant, so
getting the driver part right is really easy.

And given that ..._alloc makes it easier for a subsystem core to do
things right (as it only has to handle a single data structure that
lives long enough) that's what I did here.

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 --]