| Message ID | 20230718181849.3947851-19-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| 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 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(-)