| Message ID | 1368690525-32252-1-git-send-email-linus.walleij@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/16/2013 01:48 AM, Linus Walleij wrote: > From: Linus Walleij <linus.walleij@linaro.org> > > The pinctrldev_list_mutex is sinked into the functions that > actually traverse the list and lock it there. The code makes > much more sense in this way. All the callers are in > non-performance critical paths and the code is way more > readable this way. > > Also refactor the function get_pinctrl_dev_from_devname() to > follow the design pattern of get_pinctrl_dev_from_of_node() > which is slightly simpler. This seems fine on the surface, but I do have one question: I think the pinctrl lock serves a couple of purposes: 1) Basic protection for accesses to the pinctrldev_list itself. This patch seems just fine w.r.t. this point. 2) Preventing pinctrl drivers from being unregistered (and their modules unloaded) when some operation is being performed on/to them. So, that means some code is written as follows: lock find pinctrl device perform operation on pinctrl device unlock // only now could the found pinctrl device be unregistered However, I think this patch changes that to the following for some operations: lock find pinctrl device unlock // now the found pinctrl device can be unregistered perform operation on pinctrl device Is this true here, or am I off-base? If this isn't an issue, then the patch is fine by me. But, how is the unregistration prevented while the device is being operated on then?
On Mon, May 20, 2013 at 10:40 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > This seems fine on the surface, but I do have one question: > > I think the pinctrl lock serves a couple of purposes: > > 1) Basic protection for accesses to the pinctrldev_list itself. > > This patch seems just fine w.r.t. this point. > > 2) Preventing pinctrl drivers from being unregistered (and their modules > unloaded) when some operation is being performed on/to them. Prevention of module unloading of pin controllers has never been working properly, as there is no way to release the pinctrl handles taken by different drivers. I think that is why most pin controller drivers are bool rather than tristate. Currently only pinctrl-single is tristate. Part of me want to change that to bool, I think it only will work when using the single with hogs (that will be properly free:ed when unloading the driver). Tony: is this really working with non-hogs? If we really want to support loading/unloading of pin controllers I think the mutex is the least of the problems, and we should probably create a separate lock for handling that instead of relying on the list lock in that case. While it's probably possible to *unload* the driver properly after some hacking like this, we get to the problem of re-probing the driver and re-associating all pinctrl handles (at that point floating in space with no driver back-end) with the driver again. I feel this needs to be driven by someone who actually need to load/unload pinctrl modules. Yours, Linus Walleij
On 05/24/2013 02:04 AM, Linus Walleij wrote: > On Mon, May 20, 2013 at 10:40 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >> This seems fine on the surface, but I do have one question: >> >> I think the pinctrl lock serves a couple of purposes: >> >> 1) Basic protection for accesses to the pinctrldev_list itself. >> >> This patch seems just fine w.r.t. this point. >> >> 2) Preventing pinctrl drivers from being unregistered (and their modules >> unloaded) when some operation is being performed on/to them. > > Prevention of module unloading of pin controllers has never > been working properly, as there is no way to release the > pinctrl handles taken by different drivers. > > I think that is why most pin controller drivers are bool rather > than tristate. Once we get to multi-platform distro kernels, we will probably want all the pinctrl drivers to be modules so only the correct one gets loaded from an initrd. Hence, we'll want to move things to tristate rather than away from it. If we know the pinctrl subsystem doesn't yet work correctly with module unloads, should we modify pinctrl_register() to simply take a lock on the driver module and never drop it, so that we guarantee we don't try to unload the module later? Or, is this effectively already in place? In other words, I can accept that we know that we can't unload pinctrl drivers, but given that, I think the kernel should make sure the user /actually/ can't unload them.
On Fri, May 24, 2013 at 5:45 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 05/24/2013 02:04 AM, Linus Walleij wrote: >> Prevention of module unloading of pin controllers has never >> been working properly, as there is no way to release the >> pinctrl handles taken by different drivers. >> >> I think that is why most pin controller drivers are bool rather >> than tristate. > > Once we get to multi-platform distro kernels, we will probably want all > the pinctrl drivers to be modules so only the correct one gets loaded > from an initrd. Hence, we'll want to move things to tristate rather than > away from it. OK ... As some kind of excuse I think the current situation is an outgrowth of the fact that all the custom set-up used to be in machines down in arch/arm/* and inevitably done at machine init. > If we know the pinctrl subsystem doesn't yet work correctly with module > unloads, should we modify pinctrl_register() to simply take a lock on > the driver module and never drop it, so that we guarantee we don't try > to unload the module later? Or, is this effectively already in place? Hm, it won't happen with anything but pinctrl-single for sure. But I know that Tony used it at one point, however I still suspect that he was only using hogs. We should maybe take the lock at the instant we instatiate a pinctrl handle from something else than a hog, so as to mark that we then have external dependencies that make unloading impossible. But it'd be even cooler to actually just iterate over the pinctrl_list och handles and orphan them, and later recouple them if a driver is loaded back in. It can surely be done, but at the cost of introducing a state lock in struct pinctrl and some pieces of hairy code in the core. Yours, Linus Walleij
On 05/25/2013 03:09 AM, Linus Walleij wrote: > On Fri, May 24, 2013 at 5:45 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 05/24/2013 02:04 AM, Linus Walleij wrote: > >>> Prevention of module unloading of pin controllers has never >>> been working properly, as there is no way to release the >>> pinctrl handles taken by different drivers. >>> >>> I think that is why most pin controller drivers are bool rather >>> than tristate. >> >> Once we get to multi-platform distro kernels, we will probably want all >> the pinctrl drivers to be modules so only the correct one gets loaded >> from an initrd. Hence, we'll want to move things to tristate rather than >> away from it. > > OK ... As some kind of excuse I think the current situation is an > outgrowth of the fact that all the custom set-up used to be in machines > down in arch/arm/* and inevitably done at machine init. > >> If we know the pinctrl subsystem doesn't yet work correctly with module >> unloads, should we modify pinctrl_register() to simply take a lock on >> the driver module and never drop it, so that we guarantee we don't try >> to unload the module later? Or, is this effectively already in place? > > Hm, it won't happen with anything but pinctrl-single for sure. > But I know that Tony used it at one point, however I still > suspect that he was only using hogs. > > We should maybe take the lock at the instant we instatiate a > pinctrl handle from something else than a hog, so as to mark > that we then have external dependencies that make unloading > impossible. Yes. > But it'd be even cooler to actually just iterate over the > pinctrl_list och handles and orphan them, and later recouple > them if a driver is loaded back in. Hmm. What happens if the client driver wants to select a different state while the pinctrl driver itself isn't loaded? I'm not sure how workable that is. I think the only way to make it work would be to cache enough information in the struct pinctrl that it could be activated even if the driver wasn't loaded. That seems a little scary. I'd be fine if there was a requirement to unload all drives that were clients of the pinctrl driver before you could unload the pinctrl driver, just like any other driver/subsystem. This would work fine at least for testing with just hogs, which while not great for full-system testing would surely be fine at least when first developing the pinctrl driver.
On Tue, May 28, 2013 at 5:14 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > [Me] >> But it'd be even cooler to actually just iterate over the >> pinctrl_list och handles and orphan them, and later recouple >> them if a driver is loaded back in. > > Hmm. What happens if the client driver wants to select a different state > while the pinctrl driver itself isn't loaded? I'm not sure how workable > that is. I think the only way to make it work would be to cache enough > information in the struct pinctrl that it could be activated even if the > driver wasn't loaded. That seems a little scary. Yes it looks like that would lead to a bit of scalability problem in the struct pinctrl. If for nothing else than for the fact that it will eat memory and such :-( > I'd be fine if there was a requirement to unload all drives that were > clients of the pinctrl driver before you could unload the pinctrl > driver, just like any other driver/subsystem. This would work fine at > least for testing with just hogs, which while not great for full-system > testing would surely be fine at least when first developing the pinctrl > driver. Yes ... so if we call get_device(pctldev->dev)/put_device(pctldev->dev) for any successful pinctrl_get() which is not a hog, we increase the refcount so that the device core will disallow unloading of the driver. Now I need to figure out how to do that. Yours, Linus Walleij
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index 5327f35..1f9608b 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -101,20 +101,23 @@ EXPORT_SYMBOL_GPL(pinctrl_dev_get_drvdata); struct pinctrl_dev *get_pinctrl_dev_from_devname(const char *devname) { struct pinctrl_dev *pctldev = NULL; - bool found = false; if (!devname) return NULL; + mutex_lock(&pinctrldev_list_mutex); + list_for_each_entry(pctldev, &pinctrldev_list, node) { if (!strcmp(dev_name(pctldev->dev), devname)) { /* Matched on device name */ - found = true; - break; + mutex_unlock(&pinctrldev_list_mutex); + return pctldev; } } - return found ? pctldev : NULL; + mutex_unlock(&pinctrldev_list_mutex); + + return NULL; } struct pinctrl_dev *get_pinctrl_dev_from_of_node(struct device_node *np) @@ -326,6 +329,8 @@ static bool pinctrl_ready_for_gpio_range(unsigned gpio) struct pinctrl_gpio_range *range = NULL; struct gpio_chip *chip = gpio_to_chip(gpio); + mutex_lock(&pinctrldev_list_mutex); + /* Loop over the pin controllers */ list_for_each_entry(pctldev, &pinctrldev_list, node) { /* Loop over the ranges */ @@ -334,9 +339,13 @@ static bool pinctrl_ready_for_gpio_range(unsigned gpio) if (range->base + range->npins - 1 < chip->base || range->base > chip->base + chip->ngpio - 1) continue; + mutex_unlock(&pinctrldev_list_mutex); return true; } } + + mutex_unlock(&pinctrldev_list_mutex); + return false; } #else @@ -408,8 +417,6 @@ struct pinctrl_dev *pinctrl_find_and_add_gpio_range(const char *devname, { struct pinctrl_dev *pctldev; - mutex_lock(&pinctrldev_list_mutex); - pctldev = get_pinctrl_dev_from_devname(devname); /* @@ -418,13 +425,10 @@ struct pinctrl_dev *pinctrl_find_and_add_gpio_range(const char *devname, * range need to defer probing. */ if (!pctldev) { - mutex_unlock(&pinctrldev_list_mutex); return ERR_PTR(-EPROBE_DEFER); } pinctrl_add_gpio_range(pctldev, range); - mutex_unlock(&pinctrldev_list_mutex); - return pctldev; } EXPORT_SYMBOL_GPL(pinctrl_find_and_add_gpio_range); @@ -517,13 +521,10 @@ int pinctrl_request_gpio(unsigned gpio) int ret; int pin; - mutex_lock(&pinctrldev_list_mutex); - ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); if (ret) { if (pinctrl_ready_for_gpio_range(gpio)) ret = 0; - mutex_unlock(&pinctrldev_list_mutex); return ret; } @@ -532,7 +533,6 @@ int pinctrl_request_gpio(unsigned gpio) ret = pinmux_request_gpio(pctldev, range, pin, gpio); - mutex_unlock(&pinctrldev_list_mutex); return ret; } EXPORT_SYMBOL_GPL(pinctrl_request_gpio); @@ -552,11 +552,8 @@ void pinctrl_free_gpio(unsigned gpio) int ret; int pin; - mutex_lock(&pinctrldev_list_mutex); - ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); if (ret) { - mutex_unlock(&pinctrldev_list_mutex); return; } mutex_lock(&pctldev->mutex); @@ -567,7 +564,6 @@ void pinctrl_free_gpio(unsigned gpio) pinmux_free_gpio(pctldev, pin, range); mutex_unlock(&pctldev->mutex); - mutex_unlock(&pinctrldev_list_mutex); } EXPORT_SYMBOL_GPL(pinctrl_free_gpio); @@ -578,11 +574,8 @@ static int pinctrl_gpio_direction(unsigned gpio, bool input) int ret; int pin; - mutex_lock(&pinctrldev_list_mutex); - ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); if (ret) { - mutex_unlock(&pinctrldev_list_mutex); return ret; } @@ -593,7 +586,6 @@ static int pinctrl_gpio_direction(unsigned gpio, bool input) ret = pinmux_gpio_direction(pctldev, range, pin, input); mutex_unlock(&pctldev->mutex); - mutex_unlock(&pinctrldev_list_mutex); return ret; }