| Message ID | 20231121134901.208535-101-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | pwm: Fix lifetime issues for pwm_chips | expand |
On Tue, Nov 21, 2023 at 2:52 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 | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > index a13f3c18ccd4..02c8382b4dd2 100644 > --- a/drivers/gpio/gpio-mvebu.c > +++ b/drivers/gpio/gpio-mvebu.c > @@ -99,7 +99,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; > > @@ -615,7 +614,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) > - return -ENOMEM; > + chip = devm_pwmchip_alloc(dev, mvchip->chip.ngpio, sizeof(struct mvebu_pwm)); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + mvpwm = pwmchip_priv(chip); > + > mvchip->mvpwm = mvpwm; > mvpwm->mvchip = mvchip; > mvpwm->offset = offset; > @@ -868,13 +870,11 @@ 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; > > spin_lock_init(&mvpwm->lock); > > - return devm_pwmchip_add(dev, &mvpwm->chip); > + return devm_pwmchip_add(dev, chip); > } > > #ifdef CONFIG_DEBUG_FS > -- > 2.42.0 > Eh... I had a talk at LPC where I explained why I really dislike this approach but I guess this ship has sailed now and it's not a subsystem where I have any say anyway. It's not clear in the cover letter - are these patches supposed to go through their respective subsystem trees? Bart
Hello Bart, On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote: > Eh... I had a talk at LPC where I explained why I really dislike this > approach but I guess this ship has sailed now and it's not a subsystem > where I have any say anyway. Is there a record of your talk? I'm open to hear your arguments. > It's not clear in the cover letter - are these patches supposed to go > through their respective subsystem trees? This patch can only go in once patch #37 is in. So for now the options are: - Wait until devm_pwmchip_alloc() is in the mainline and apply this patch then via the gpio tree - Ack it and let it go in via the pwm tree with the other patches. I'm not sure how quick this series will go in, so there is no rush. Best regards Uwe
Hello Bart, On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote: > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote: > > Eh... I had a talk at LPC where I explained why I really dislike this > > approach but I guess this ship has sailed now and it's not a subsystem > > where I have any say anyway. > > Is there a record of your talk? I'm open to hear your arguments. I found your slides at https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf The main critic as I understand it about the "alloc_foo() + register_foo()" approach is: "Breaks life-time logic - the driver allocates the object but is not responsible for freeing it". Yes, the driver allocates the object (via a subsystem helper). It is not responsible for freeing the object, but the driver must drop its reference to this object when going away. So foo_alloc() is paired by foo_put(). The solution you present as the good way has the struct device in the foo_wrapper. In GPIO land that's struct gpio_device, right? gpiochip_add_data_with_key() allocates that using kzalloc() and "frees" it with gpio_device_put() right? So your approach suffers from the same inconsistency, the only upside is that you do that once at the subsystem level instead of in each driver. (And in return you have two allocations (priv + foo_wrapper) while the "alloc_foo() + register_foo()" approach only needs one.) Let's just rename foo_alloc() to foo_get_new() and the problem is gone? In the implementation of foo_get_new() kzalloc() is still paired with put_device() in foo_put(), but IMHO that's fine. The responsibility to kfree() is traded to the struct device with device_initialize() in return for a reference to the device. That's something you won't get rid of while keeping the concept of reference counting. Best regards Uwe
On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Bart, > > On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote: > > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote: > > > Eh... I had a talk at LPC where I explained why I really dislike this > > > approach but I guess this ship has sailed now and it's not a subsystem > > > where I have any say anyway. > > > > Is there a record of your talk? I'm open to hear your arguments. > > I found your slides at > https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf > My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s > The main critic as I understand it about the "alloc_foo() + > register_foo()" approach is: "Breaks life-time logic - the driver > allocates the object but is not responsible for freeing it". > > Yes, the driver allocates the object (via a subsystem helper). It is not > responsible for freeing the object, but the driver must drop its > reference to this object when going away. So foo_alloc() is paired by > foo_put(). > Is it though? I don't see any pwmchip_put() being called in this patch. I assume it's done implicitly but that's just confusing and does break the scope. > The solution you present as the good way has the struct device in the > foo_wrapper. In GPIO land that's struct gpio_device, right? Exactly. > gpiochip_add_data_with_key() allocates that using kzalloc() and "frees" > it with gpio_device_put() right? So your approach suffers from the same No, the structure is allocated by kzalloc() but it's life-time is tied with the struct device embedded in it and it's freed in the device's .release() callback when the last reference is dropped. > inconsistency, the only upside is that you do that once at the subsystem > level instead of in each driver. (And in return you have two allocations > (priv + foo_wrapper) while the "alloc_foo() + register_foo()" approach > only needs one.) Memory is cheap and this is not a hot path, so it isn't a big deal. > > Let's just rename foo_alloc() to foo_get_new() and the problem is gone? > Nope, because from a quick glance at PWM code, I'm convinced it will suffer from the same hot-unplug problem I described in my talk. In which case this rework will not fix all the issues. > In the implementation of foo_get_new() kzalloc() is still paired with > put_device() in foo_put(), but IMHO that's fine. The responsibility to > kfree() is traded to the struct device with device_initialize() in > return for a reference to the device. That's something you won't get rid > of while keeping the concept of reference counting. > But if the PWM driver is unbound with users still holding references - do you have a mechanism to handle that? Bart > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Bart, On Wed, Nov 22, 2023 at 11:36:19AM +0100, Bartosz Golaszewski wrote: > On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote: > > > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote: > > > > Eh... I had a talk at LPC where I explained why I really dislike this > > > > approach but I guess this ship has sailed now and it's not a subsystem > > > > where I have any say anyway. > > > > > > Is there a record of your talk? I'm open to hear your arguments. > > > > I found your slides at > > https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf > > > > My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s > > > The main critic as I understand it about the "alloc_foo() + > > register_foo()" approach is: "Breaks life-time logic - the driver > > allocates the object but is not responsible for freeing it". > > > > Yes, the driver allocates the object (via a subsystem helper). It is not > > responsible for freeing the object, but the driver must drop its > > reference to this object when going away. So foo_alloc() is paired by > > foo_put(). > > > > Is it though? I don't see any pwmchip_put() being called in this > patch. It's not in this patch. Up to patch #103 I'm preparing drivers and the code that is moved into the core isn't better than what was done before in each driver. Look at patch #106 which does the relevant conversion in pwmchip_alloc(). When unbinding the mvebu gpio driver the necessary pwmchip_put() is triggered by the devm cleanup registered in devm_pwmchip_alloc(). > I assume it's done implicitly but that's just confusing and > does break the scope. > > > The solution you present as the good way has the struct device in the > > foo_wrapper. In GPIO land that's struct gpio_device, right? > > Exactly. > > > gpiochip_add_data_with_key() allocates that using kzalloc() and "frees" > > it with gpio_device_put() right? So your approach suffers from the same > > No, the structure is allocated by kzalloc() but it's life-time is tied > with the struct device embedded in it and it's freed in the device's > .release() callback when the last reference is dropped. With the complete series applied a pwmchip is allocated by pwmchip_alloc() and it's life-time is tied with the struct device embedded in it and it's freed in the device's .release() callback when the last reference is dropped. In this respect I see a certain similarity between your gpio approach and mine for pwm. So either I don't understand your critic on my patch set, or I don't see why it shouldn't apply to your approach, too. Yes, gpio drivers look fine having only ..._alloc() paired with ..._free() and ..._get() with ..._put(). But that's only because you moved that "inconsistency" of kzalloc() <-> put_device() into the gpio core, while I kept it in the drivers. Renaming pwmchip_alloc() to pwmchip_get_new() was a honest suggestion that moves that inconsistency to the core, too. > > inconsistency, the only upside is that you do that once at the subsystem > > level instead of in each driver. (And in return you have two allocations > > (priv + foo_wrapper) while the "alloc_foo() + register_foo()" approach > > only needs one.) > > Memory is cheap and this is not a hot path, so it isn't a big deal. It's not only about wasting memory and the time needed to dereference pointers. It's also about complexity that has to be grasped by humans. Also not being in a hot path doesn't mean it's bad to pick the faster approach. Having said that I'm not sure if the hot paths (e.g. gpiod_set_value()) really don't suffer from having two separate allocations. But I guess we're both biased here to our own approach because that's what each of us thought about in detail. > > Let's just rename foo_alloc() to foo_get_new() and the problem is gone? > > Nope, because from a quick glance at PWM code, I'm convinced it will > suffer from the same hot-unplug problem I described in my talk. In > which case this rework will not fix all the issues. Please look at the state after patch #107. If you spot an issue there, please tell me. > > In the implementation of foo_get_new() kzalloc() is still paired with > > put_device() in foo_put(), but IMHO that's fine. The responsibility to > > kfree() is traded to the struct device with device_initialize() in > > return for a reference to the device. That's something you won't get rid > > of while keeping the concept of reference counting. > > But if the PWM driver is unbound with users still holding references - > do you have a mechanism to handle that? Yes, should be fine starting with patch #107. In my tests (on top of patch #108) it works fine. I held /dev/pwmchipX open and unbound the lowlevel driver. The ioctls are caught in the core then and yield an error and the kfree of the pwmchip struct is delayed until /dev/pwmchipX is closed. Best regards Uwe
On Wed, Nov 22, 2023 at 11:36:19AM +0100, Bartosz Golaszewski wrote: > On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > Hello Bart, > > > > On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote: > > > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote: > > > > Eh... I had a talk at LPC where I explained why I really dislike this > > > > approach but I guess this ship has sailed now and it's not a subsystem > > > > where I have any say anyway. > > > > > > Is there a record of your talk? I'm open to hear your arguments. > > > > I found your slides at > > https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf > > > > My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s I've been watching this along with Laurent's talk from last year (and I guess I should probably also go through Wolfram's patch from earlier this year) and I really like what you presented. It also sounds like there was a lot of support across various audience members, so I think it'd be good to rally around such a common pattern so we can start to improve things on a more wide basis. Given that this wasn't very long ago, I wouldn't expect that much work has happened yet on the resmgr library. However, I think it would fit very well both with how PWM works today and with what Uwe has in mind for the character device support. Thierry
On Fri, Nov 24, 2023 at 1:14 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Nov 22, 2023 at 11:36:19AM +0100, Bartosz Golaszewski wrote: > > On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > Hello Bart, > > > > > > On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote: > > > > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote: > > > > > Eh... I had a talk at LPC where I explained why I really dislike this > > > > > approach but I guess this ship has sailed now and it's not a subsystem > > > > > where I have any say anyway. > > > > > > > > Is there a record of your talk? I'm open to hear your arguments. > > > > > > I found your slides at > > > https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf > > > > > > > My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s > > I've been watching this along with Laurent's talk from last year (and I > guess I should probably also go through Wolfram's patch from earlier > this year) and I really like what you presented. It also sounds like > there was a lot of support across various audience members, so I think > it'd be good to rally around such a common pattern so we can start to > improve things on a more wide basis. > > Given that this wasn't very long ago, I wouldn't expect that much work > has happened yet on the resmgr library. However, I think it would fit > very well both with how PWM works today and with what Uwe has in mind > for the character device support. > > Thierry Hi Thierry, Thanks for the kind words. No work has been done so far other than thinking about the possible API. I'm currently in the process of trying to fix the object life-time and concurrent access in GPIO - mostly improving the dire locking situation. My goal is to implement all I spoke about in GPIO first and then try to generalize it to some other subsystem like what Greg KH suggested. I've already got support from Wolfram on that and we of course could use any help we can get. I admit I've been quite busy but I do plan on going through Uwe's series next week and maybe running tests similar to what I have for GPIO on it. I'm quite certain (correct me if I'm wrong) that this series doesn't improve the locking (specifically hot-unplug events during API calls). I think that my proposal has the advantage of having the pointer to the implementation in the "wrapper" which can be easily protected with RCU. Uwe: do you have a solution for device removal concurrent with API calls when using your approach? Bart
Hello Bart, On Fri, Nov 24, 2023 at 10:16:40PM +0100, Bartosz Golaszewski wrote: > On Fri, Nov 24, 2023 at 1:14 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Nov 22, 2023 at 11:36:19AM +0100, Bartosz Golaszewski wrote: > > > On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote: > > > > > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote: > > > > > > Eh... I had a talk at LPC where I explained why I really dislike this > > > > > > approach but I guess this ship has sailed now and it's not a subsystem > > > > > > where I have any say anyway. > > > > > > > > > > Is there a record of your talk? I'm open to hear your arguments. > > > > > > > > I found your slides at > > > > https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf > > > > > > > > > > My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s > > > > I've been watching this along with Laurent's talk from last year (and I > > guess I should probably also go through Wolfram's patch from earlier > > this year) and I really like what you presented. It also sounds like > > there was a lot of support across various audience members, so I think > > it'd be good to rally around such a common pattern so we can start to > > improve things on a more wide basis. > > > > Given that this wasn't very long ago, I wouldn't expect that much work > > has happened yet on the resmgr library. However, I think it would fit > > very well both with how PWM works today and with what Uwe has in mind > > for the character device support. > > > > Thierry > > Hi Thierry, > > Thanks for the kind words. No work has been done so far other than > thinking about the possible API. I'm currently in the process of > trying to fix the object life-time and concurrent access in GPIO - > mostly improving the dire locking situation. My goal is to implement > all I spoke about in GPIO first and then try to generalize it to some > other subsystem like what Greg KH suggested. > > I've already got support from Wolfram on that and we of course could > use any help we can get. > > I admit I've been quite busy but I do plan on going through Uwe's > series next week and maybe running tests similar to what I have for > GPIO on it. I'm quite certain (correct me if I'm wrong) that this > series doesn't improve the locking (specifically hot-unplug events > during API calls). I think that my proposal has the advantage of > having the pointer to the implementation in the "wrapper" which can be > easily protected with RCU. Maybe I didn't understand the problem yet, but I think hotplugging isn't a problem for my approach. The hardware accesses in the lowlevel driver (probably) fail then but that's something you cannot prevent. And because pwmchip->lock is held during calls in the lowlevel driver, removal of the driver is delayed accordingly. Best regards Uwe
Hello Bartosz, On Fri, Nov 24, 2023 at 10:16:40PM +0100, Bartosz Golaszewski wrote: > I admit I've been quite busy but I do plan on going through Uwe's > series next week and maybe running tests similar to what I have for > GPIO on it. That's great. If you want to do that on my tree that already saw a few improvements compared to what I sent out, get it at https://git.pengutronix.de/git/ukl/linux pwm-lifetime-tracking . The improvements are only on the driver level, so unless you're using one of the improved drivers, the difference wouldn't be that big I guess. For (maybe) quicker feedback loops, you can find me on irc (e.g. on libera's #linux-pwm) if that's a communication channel you like. I look forward to your findings, Uwe
On Mon, Nov 27, 2023 at 11:58 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Bartosz, > > On Fri, Nov 24, 2023 at 10:16:40PM +0100, Bartosz Golaszewski wrote: > > I admit I've been quite busy but I do plan on going through Uwe's > > series next week and maybe running tests similar to what I have for > > GPIO on it. > > That's great. If you want to do that on my tree that already saw a few > improvements compared to what I sent out, get it at > > https://git.pengutronix.de/git/ukl/linux pwm-lifetime-tracking > > . The improvements are only on the driver level, so unless you're using > one of the improved drivers, the difference wouldn't be that big I > guess. For (maybe) quicker feedback loops, you can find me on irc (e.g. > on libera's #linux-pwm) if that's a communication channel you like. > > I look forward to your findings, > Uwe I don't see anything obviously wrong with the approach. I see the chip->operational field that is set to false on release. In my version, we just use a NULL-pointer to carry the same information. Interestingly you DO have a pwm_device and pwm_chip structures. I'd say it would be more logical to have the pwm_device embed struct device. My approach is more about maintaining the logical scope and not changing the ownership of objects allocated in the driver. I also don't see a reason to expose the internals of the subsystem (struct device) to the provider drivers other than in callbacks where it is relevant. Subsystems should handle as much as possible and any data structures not relevant to what the driver does should be hidden from it. Bart
Hello Bart, On Mon, Nov 27, 2023 at 09:22:48PM +0100, Bartosz Golaszewski wrote: > On Mon, Nov 27, 2023 at 11:58 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Fri, Nov 24, 2023 at 10:16:40PM +0100, Bartosz Golaszewski wrote: > > > I admit I've been quite busy but I do plan on going through Uwe's > > > series next week and maybe running tests similar to what I have for > > > GPIO on it. > > > > That's great. If you want to do that on my tree that already saw a few > > improvements compared to what I sent out, get it at > > > > https://git.pengutronix.de/git/ukl/linux pwm-lifetime-tracking > > > > . The improvements are only on the driver level, so unless you're using > > one of the improved drivers, the difference wouldn't be that big I > > guess. For (maybe) quicker feedback loops, you can find me on irc (e.g. > > on libera's #linux-pwm) if that's a communication channel you like. > > I don't see anything obviously wrong with the approach. Is this the result of "running tests similar to what I have for GPIO on it" or did you only find the time for some high-level code inspection? > I see the > chip->operational field that is set to false on release. In my > version, we just use a NULL-pointer to carry the same information. Yup, sounds obvious. Your usage of "just" sounds as if your variant was better. To give the alternative view where the suggested approach sounds better would be: You need a pointer and I "just" a bool that even has a name implying its function. You need to dereference the pointer in several places as the needed information is distributed over two structures while it's all together in a single struct for the usual foo_alloc() + foo_register() approach. > Interestingly you DO have a pwm_device and pwm_chip structures. I'd > say it would be more logical to have the pwm_device embed struct > device. A pwm_chip represents a piece of hardware that provides (possibly) several PWM lines. A pwm_device is the abstraction for a single PWM line. So that's two different concepts and I wonder why you find it interesting that we have two different structures for it. Today the pwm framework already has a struct device for the pwm_chip that appears in /sys/class/pwm/pwmchipX. If a PWM line is exported in sysfs, another struct containing a struct device is allocated (struct pwm_export) to manage /sys/class/pwm/pwmchipX/pwmY/. I think it's good to have a struct device in the gpio_chip. I'd be open to put a struct device into pwm_device (unconditionally, not only when it's exported), but that's a change that is out of scope for this series. Also note that this would change the behaviour of /sys/class/pwm/ which I'd like to prevent (at least today until the character support is established, available for some time and known to be in use). > My approach is more about maintaining the logical scope and not > changing the ownership of objects allocated in the driver. I also > don't see a reason to expose the internals of the subsystem (struct > device) to the provider drivers other than in callbacks where it is > relevant. Subsystems should handle as much as possible and any data > structures not relevant to what the driver does should be hidden from > it. Drivers see struct pwm_chip today and IMHO that's fine. I also feel little incentive to hide something from the driver in .probe() and then have to expose (more of) it in .apply() anyhow. Also I don't think the series would benefit from putting yet more changes into it. Struct pwm_chip currently contains the following members: struct device dev; struct cdev cdev; const struct pwm_ops *ops; struct module *owner; unsigned int id; unsigned int npwm; struct pwm_device * (*of_xlate)(struct pwm_chip *chip, const struct of_phandle_args *args); unsigned int of_pwm_n_cells; /* only used internally by the PWM framework */ struct mutex lock; bool uses_pwmchip_alloc; bool operational; void *drvdata; struct pwm_device pwms[] __counted_by(npwm); Some of them should be moved below the "only used internally" comment. (i.e. dev, cdev, owner, id). For me this is "hidden" good enough then. Best regards Uwe
On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > [snip] > > > I see the > > chip->operational field that is set to false on release. In my > > version, we just use a NULL-pointer to carry the same information. > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was > better. To give the alternative view where the suggested approach sounds > better would be: > > You need a pointer and I "just" a bool that even has a name implying its > function. You need to dereference the pointer in several places as the > needed information is distributed over two structures while it's all > together in a single struct for the usual foo_alloc() + foo_register() > approach. > There's another reason we do that. I'm no longer sure if I mentioned it in my talk (I meant to anyway). In GPIO we have API functions that may be called from any context - thus needing spinlocks for locking - but also driver callbacks that may use mutexes internally or otherwise sleep. I don't know if this is the case for PWM too but in GPIO we may end up in a situation where if we used a spinlock to protect some kind of an "is_operational" field, we'd end up sleeping with a spinlock taken and if we used a mutex, we couldn't use API function from atomic contexts. This is the reason behind locking being so broken in GPIO at the moment and why I'm trying to fix it this release cycle. Splitting the implementation into two structures and protecting the pointer to the provider structure with SRCU has the benefit of not limiting us in what locks we use underneath. Every subsystem has its own issues and we need to find something generic enough to cover them all (or most of them anyway). I don't think having a single structure cuts it. Bart [snip]
On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote: > On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > [snip] > > > > > > I see the > > > chip->operational field that is set to false on release. In my > > > version, we just use a NULL-pointer to carry the same information. > > > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was > > better. To give the alternative view where the suggested approach sounds > > better would be: > > > > You need a pointer and I "just" a bool that even has a name implying its > > function. You need to dereference the pointer in several places as the > > needed information is distributed over two structures while it's all > > together in a single struct for the usual foo_alloc() + foo_register() > > approach. > > > > There's another reason we do that. I'm no longer sure if I mentioned > it in my talk (I meant to anyway). > > In GPIO we have API functions that may be called from any context - > thus needing spinlocks for locking - but also driver callbacks that > may use mutexes internally or otherwise sleep. I don't know if this is > the case for PWM too but in GPIO we may end up in a situation where if > we used a spinlock to protect some kind of an "is_operational" field, > we'd end up sleeping with a spinlock taken and if we used a mutex, we > couldn't use API function from atomic contexts. > > This is the reason behind locking being so broken in GPIO at the > moment and why I'm trying to fix it this release cycle. > > Splitting the implementation into two structures and protecting the > pointer to the provider structure with SRCU has the benefit of not > limiting us in what locks we use underneath. > > Every subsystem has its own issues and we need to find something > generic enough to cover them all (or most of them anyway). I don't > think having a single structure cuts it. I'm convinced it works. I introduced a wrapper pwmchip_lock() that for now uses a mutex and once we have fast pwm_chips it uses a mutex for sleeping pwm_chips and a spinlock for the fast ones. That's similar to how struct irq_chip::irq_bus_lock works. For sleeping chips that callback uses a mutex, for fast chips a spinlock. Best regards Uwe
On Sat, Dec 2, 2023 at 1:43 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote: > > On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > [snip] > > > > > > > > > I see the > > > > chip->operational field that is set to false on release. In my > > > > version, we just use a NULL-pointer to carry the same information. > > > > > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was > > > better. To give the alternative view where the suggested approach sounds > > > better would be: > > > > > > You need a pointer and I "just" a bool that even has a name implying its > > > function. You need to dereference the pointer in several places as the > > > needed information is distributed over two structures while it's all > > > together in a single struct for the usual foo_alloc() + foo_register() > > > approach. > > > > > > > There's another reason we do that. I'm no longer sure if I mentioned > > it in my talk (I meant to anyway). > > > > In GPIO we have API functions that may be called from any context - > > thus needing spinlocks for locking - but also driver callbacks that > > may use mutexes internally or otherwise sleep. I don't know if this is > > the case for PWM too but in GPIO we may end up in a situation where if > > we used a spinlock to protect some kind of an "is_operational" field, > > we'd end up sleeping with a spinlock taken and if we used a mutex, we > > couldn't use API function from atomic contexts. > > > > This is the reason behind locking being so broken in GPIO at the > > moment and why I'm trying to fix it this release cycle. > > > > Splitting the implementation into two structures and protecting the > > pointer to the provider structure with SRCU has the benefit of not > > limiting us in what locks we use underneath. > > > > Every subsystem has its own issues and we need to find something > > generic enough to cover them all (or most of them anyway). I don't > > think having a single structure cuts it. > > I'm convinced it works. I introduced a wrapper pwmchip_lock() that for > now uses a mutex and once we have fast pwm_chips it uses a mutex for > sleeping pwm_chips and a spinlock for the fast ones. > > That's similar to how struct irq_chip::irq_bus_lock works. For sleeping > chips that callback uses a mutex, for fast chips a spinlock. > Fair enough. I'd love to see a benchmark of what's faster one day though: two structures with dereferencing and SRCU or one structure with mutex/spinlock. By "fair enough" I mean: I still don't like it for the reasons I mentioned before but I cannot point out anything technically wrong. Bart > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Dec 4, 2023 at 9:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Sat, Dec 2, 2023 at 1:43 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote: > > > On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > > > [snip] > > > > > > > > > > > > I see the > > > > > chip->operational field that is set to false on release. In my > > > > > version, we just use a NULL-pointer to carry the same information. > > > > > > > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was > > > > better. To give the alternative view where the suggested approach sounds > > > > better would be: > > > > > > > > You need a pointer and I "just" a bool that even has a name implying its > > > > function. You need to dereference the pointer in several places as the > > > > needed information is distributed over two structures while it's all > > > > together in a single struct for the usual foo_alloc() + foo_register() > > > > approach. > > > > > > > > > > There's another reason we do that. I'm no longer sure if I mentioned > > > it in my talk (I meant to anyway). > > > > > > In GPIO we have API functions that may be called from any context - > > > thus needing spinlocks for locking - but also driver callbacks that > > > may use mutexes internally or otherwise sleep. I don't know if this is > > > the case for PWM too but in GPIO we may end up in a situation where if > > > we used a spinlock to protect some kind of an "is_operational" field, > > > we'd end up sleeping with a spinlock taken and if we used a mutex, we > > > couldn't use API function from atomic contexts. > > > > > > This is the reason behind locking being so broken in GPIO at the > > > moment and why I'm trying to fix it this release cycle. > > > > > > Splitting the implementation into two structures and protecting the > > > pointer to the provider structure with SRCU has the benefit of not > > > limiting us in what locks we use underneath. > > > > > > Every subsystem has its own issues and we need to find something > > > generic enough to cover them all (or most of them anyway). I don't > > > think having a single structure cuts it. > > > > I'm convinced it works. I introduced a wrapper pwmchip_lock() that for > > now uses a mutex and once we have fast pwm_chips it uses a mutex for > > sleeping pwm_chips and a spinlock for the fast ones. > > > > That's similar to how struct irq_chip::irq_bus_lock works. For sleeping > > chips that callback uses a mutex, for fast chips a spinlock. > > > > Fair enough. I'd love to see a benchmark of what's faster one day > though: two structures with dereferencing and SRCU or one structure > with mutex/spinlock. > Actually there is one thing that - while not technically wrong - makes the split solution better. In case of your abstracted lock, you find yourself in a very all-or-nothing locking situation, where all of the structure is locked or none is. With SRCU protecting just the pointer to implementation, we can easily factor that part out and leave whatever fine-grained locking is required to the subsystem. Additionally: the pointer to implementation has many readers but only one writer. I believe this to be the same for your "operational" field. I don't know the PWM code very well but I can only guess that the situation is similar, where subsystem data structures are read more often than they are modified and multiple readers could access the structure at the same time lowering latencies. Just another 2 cents. Bart > By "fair enough" I mean: I still don't like it for the reasons I > mentioned before but I cannot point out anything technically wrong. > > Bart > > > Best regards > > Uwe > > > > -- > > Pengutronix e.K. | Uwe Kleine-König | > > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello, On Mon, Dec 04, 2023 at 10:28:15PM +0100, Bartosz Golaszewski wrote: > On Mon, Dec 4, 2023 at 9:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > On Sat, Dec 2, 2023 at 1:43 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote: > > > > On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > I see the > > > > > > chip->operational field that is set to false on release. In my > > > > > > version, we just use a NULL-pointer to carry the same information. > > > > > > > > > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was > > > > > better. To give the alternative view where the suggested approach sounds > > > > > better would be: > > > > > > > > > > You need a pointer and I "just" a bool that even has a name implying its > > > > > function. You need to dereference the pointer in several places as the > > > > > needed information is distributed over two structures while it's all > > > > > together in a single struct for the usual foo_alloc() + foo_register() > > > > > approach. > > > > > > > > > > > > > There's another reason we do that. I'm no longer sure if I mentioned > > > > it in my talk (I meant to anyway). > > > > > > > > In GPIO we have API functions that may be called from any context - > > > > thus needing spinlocks for locking - but also driver callbacks that > > > > may use mutexes internally or otherwise sleep. I don't know if this is > > > > the case for PWM too but in GPIO we may end up in a situation where if > > > > we used a spinlock to protect some kind of an "is_operational" field, > > > > we'd end up sleeping with a spinlock taken and if we used a mutex, we > > > > couldn't use API function from atomic contexts. > > > > > > > > This is the reason behind locking being so broken in GPIO at the > > > > moment and why I'm trying to fix it this release cycle. > > > > > > > > Splitting the implementation into two structures and protecting the > > > > pointer to the provider structure with SRCU has the benefit of not > > > > limiting us in what locks we use underneath. > > > > > > > > Every subsystem has its own issues and we need to find something > > > > generic enough to cover them all (or most of them anyway). I don't > > > > think having a single structure cuts it. > > > > > > I'm convinced it works. I introduced a wrapper pwmchip_lock() that for > > > now uses a mutex and once we have fast pwm_chips it uses a mutex for > > > sleeping pwm_chips and a spinlock for the fast ones. > > > > > > That's similar to how struct irq_chip::irq_bus_lock works. For sleeping > > > chips that callback uses a mutex, for fast chips a spinlock. > > > > > > > Fair enough. I'd love to see a benchmark of what's faster one day > > though: two structures with dereferencing and SRCU or one structure > > with mutex/spinlock. I think until the day has come that we have a SRCU+two and mutex/spinlock+one implementation for one framework it's moot to discuss which one is the faster, so I suggest we stop here. (Note, you can already do mutex/spinlock+two already now. That's what I do for the non-pure PWM drivers in the next iteration. Preview at https://lore.kernel.org/linux-pwm/20231124215208.616551-4-u.kleine-koenig@pengutronix.de/T/#u) For me it's not so clear that SRCU is the winner. Also the winner might vary depending on questions like: - How many PWM (or GPIO) lines does the chip in question expose? - Does the implementation of the callbacks need serialisation (because the bits for different lines are in common registers)? - Usage pattern (influencing the contention of the locks) (But I adhere to my suggestion to stop now :-) > Actually there is one thing that - while not technically wrong - makes > the split solution better. In case of your abstracted lock, you find > yourself in a very all-or-nothing locking situation, where all of the > structure is locked or none is. With SRCU protecting just the pointer > to implementation, we can easily factor that part out and leave > whatever fine-grained locking is required to the subsystem. The fine-grainedness of the locking scheme isn't fixed with my approach. In fact you could just not use the offer to handle framework struct and driver private data in a single memory chunk (and/or stop using the knowledge that it is (or can be) a single chunk) and then the two approaches are not fundamentally different and you can use the same locking mechanisms. The biggest difference between our approaches is that I handle allocation of the framework struct and its registration in two steps in the drivers while you do that in a single one. My approach has the advantage that it allows to handle private data in the same allocation and that the driver can fill in the framework struct without the need for copying or pointer dereferencing if the framework needs the driver provided information. Yours has the advantage that drivers see less of the framework and so are better separated from the core. How you weight the different advantages is very subjective. So if we rule out the subjective metrics we're left with: Both approaches solve the technical challenge at hand (i.e. ensure unloading a driver doesn't make the kernel crash if a character device is still open) and my approach already exists for pwm. > Additionally: the pointer to implementation has many readers but only > one writer. I believe this to be the same for your "operational" > field. I don't know the PWM code very well but I can only guess that > the situation is similar, where subsystem data structures are read > more often than they are modified and multiple readers could access > the structure at the same time lowering latencies. The lock serves to serialize access to .operational and ensures that the driver doesn't go away until all callbacks have completed. Is this serialized in your approach, too? (If you don't, I wonder if you should. And if you do, I think this better matches the use case spinlocks and mutexes are optimized for compared to SRCU.) Best regards Uwe
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c index a13f3c18ccd4..02c8382b4dd2 100644 --- a/drivers/gpio/gpio-mvebu.c +++ b/drivers/gpio/gpio-mvebu.c @@ -99,7 +99,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; @@ -615,7 +614,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) - return -ENOMEM; + chip = devm_pwmchip_alloc(dev, mvchip->chip.ngpio, sizeof(struct mvebu_pwm)); + if (IS_ERR(chip)) + return PTR_ERR(chip); + mvpwm = pwmchip_priv(chip); + mvchip->mvpwm = mvpwm; mvpwm->mvchip = mvchip; mvpwm->offset = offset; @@ -868,13 +870,11 @@ 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; 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 | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)