| Message ID | 20230718181849.3947851-19-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Rejected |
| Headers | show |
| Series | pwm: Provide devm_pwmchip_alloc() function | expand |
On Tue, Jul 18, 2023 at 8:19 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > This prepares the pwm sub-driver to further changes of the pwm core > outlined in the commit introducing devm_pwmchip_alloc(). There is no > intended semantical change and the driver should behave as before. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/gpio/gpio-mvebu.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > index a35958e7adf6..9557cac807f9 100644 > --- a/drivers/gpio/gpio-mvebu.c > +++ b/drivers/gpio/gpio-mvebu.c > @@ -98,7 +98,6 @@ struct mvebu_pwm { > u32 offset; > unsigned long clk_rate; > struct gpio_desc *gpiod; > - struct pwm_chip chip; > spinlock_t lock; > struct mvebu_gpio_chip *mvchip; > > @@ -614,7 +613,7 @@ static const struct regmap_config mvebu_gpio_regmap_config = { > */ > static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip) > { > - return container_of(chip, struct mvebu_pwm, chip); > + return pwmchip_priv(chip); > } > > static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > @@ -789,6 +788,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev, > { > struct device *dev = &pdev->dev; > struct mvebu_pwm *mvpwm; > + struct pwm_chip *chip; > void __iomem *base; > u32 offset; > u32 set; > @@ -813,9 +813,11 @@ static int mvebu_pwm_probe(struct platform_device *pdev, > if (IS_ERR(mvchip->clk)) > return PTR_ERR(mvchip->clk); > > - mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL); > - if (!mvpwm) > + chip = devm_pwmchip_alloc(dev, sizeof(struct mvebu_pwm)); > + if (!chip) > return -ENOMEM; > + mvpwm = pwmchip_priv(chip); > + > mvchip->mvpwm = mvpwm; > mvpwm->mvchip = mvchip; > mvpwm->offset = offset; > @@ -868,13 +870,12 @@ static int mvebu_pwm_probe(struct platform_device *pdev, > return -EINVAL; > } > > - mvpwm->chip.dev = dev; > - mvpwm->chip.ops = &mvebu_pwm_ops; > - mvpwm->chip.npwm = mvchip->chip.ngpio; > + chip->ops = &mvebu_pwm_ops; > + chip->npwm = mvchip->chip.ngpio; > > spin_lock_init(&mvpwm->lock); > > - return devm_pwmchip_add(dev, &mvpwm->chip); > + return devm_pwmchip_add(dev, chip); > } > > #ifdef CONFIG_DEBUG_FS > -- > 2.39.2 > Looks good to me (although I have my reservations about the concept of foo_alloc() for subsystems in the kernel...). How do you want this to go upstream? Bart
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? > How do you want this to go upstream? I haven't thought about that yet. I first will have to convince Thierry that this is a good idea I guess. This version will not be merged for sure. Best regards Uwe [1] https://static.sched.com/hosted_files/eoss2023/e3/LifecycleIssues_WolframSang_2023.pdf
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. 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 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. Bart > > How do you want this to go upstream? > > I haven't thought about that yet. I first will have to convince > Thierry that this is a good idea I guess. This version will not be > merged for sure. > > Best regards > Uwe > > [1] https://static.sched.com/hosted_files/eoss2023/e3/LifecycleIssues_WolframSang_2023.pdf > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
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: > > 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. > > 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 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. A difficulty I see is that as of now the ops-pointer is maintained in driver-allocated data. So it's not possible to call the .free callback once the driver is gone. So either unbinding the driver must be delayed until all consumers are gone, or the reference to a PWM that a consumer holds must be invalidated. Both options are not optimal. But I have to admit that I didn't grasp the device core completely (yet?), so I might well miss something. Also I like the concept of "..._alloc" and find it clear enough. I'm not aware of "ownership confusions". I'm open to hear about these if you have something to point out. Best regards Uwe
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
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)?
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
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c index a35958e7adf6..9557cac807f9 100644 --- a/drivers/gpio/gpio-mvebu.c +++ b/drivers/gpio/gpio-mvebu.c @@ -98,7 +98,6 @@ struct mvebu_pwm { u32 offset; unsigned long clk_rate; struct gpio_desc *gpiod; - struct pwm_chip chip; spinlock_t lock; struct mvebu_gpio_chip *mvchip; @@ -614,7 +613,7 @@ static const struct regmap_config mvebu_gpio_regmap_config = { */ static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip) { - return container_of(chip, struct mvebu_pwm, chip); + return pwmchip_priv(chip); } static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) @@ -789,6 +788,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev, { struct device *dev = &pdev->dev; struct mvebu_pwm *mvpwm; + struct pwm_chip *chip; void __iomem *base; u32 offset; u32 set; @@ -813,9 +813,11 @@ static int mvebu_pwm_probe(struct platform_device *pdev, if (IS_ERR(mvchip->clk)) return PTR_ERR(mvchip->clk); - mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL); - if (!mvpwm) + chip = devm_pwmchip_alloc(dev, sizeof(struct mvebu_pwm)); + if (!chip) return -ENOMEM; + mvpwm = pwmchip_priv(chip); + mvchip->mvpwm = mvpwm; mvpwm->mvchip = mvchip; mvpwm->offset = offset; @@ -868,13 +870,12 @@ static int mvebu_pwm_probe(struct platform_device *pdev, return -EINVAL; } - mvpwm->chip.dev = dev; - mvpwm->chip.ops = &mvebu_pwm_ops; - mvpwm->chip.npwm = mvchip->chip.ngpio; + chip->ops = &mvebu_pwm_ops; + chip->npwm = mvchip->chip.ngpio; spin_lock_init(&mvpwm->lock); - return devm_pwmchip_add(dev, &mvpwm->chip); + return devm_pwmchip_add(dev, chip); } #ifdef CONFIG_DEBUG_FS
This prepares the pwm sub-driver to further changes of the pwm core outlined in the commit introducing devm_pwmchip_alloc(). There is no intended semantical change and the driver should behave as before. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/gpio/gpio-mvebu.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)