| Message ID | 20161025210221.9150-2-tony@atomide.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 25, 2016 at 11:02 PM, Tony Lindgren <tony@atomide.com> wrote: > Having the pin control framework call pin controller functions > before it's probe has finished is not nice as the pin controller > device driver does not yet have struct pinctrl_dev handle. > > Let's fix this issue by adding deferred work for hogs. This is > needed to be able to add pinctrl generic helper functions. > > Note that the pinctrl functions already take care of the necessary > locking. > > Signed-off-by: Tony Lindgren <tony@atomide.com> I don't see why this is necessary? The hogging was placed inside pinctrl_register() so that any hogs would be taken before it returns, so nothing else can take it before the controller itself has the first chance. This semantic needs to be preserved I think. > + schedule_delayed_work(&pctldev->hog_work, > + msecs_to_jiffies(100)); If we arbitrarily delay, something else can go in and take the pins used by the hogs before the pinctrl core? That is what we want to avoid. Hm, 100ms seems arbitrarily chosen BTW. Can it be 1 ms? 1 ns? I'm pretty sure that whatever it is that needs to happen before the hog work runs can race with this delayed work under some circumstances (such as slow external expanders on i2c). It should be impossible for that to happen and I don't think it is? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Linus Walleij <linus.walleij@linaro.org> [161111 12:17]: > On Tue, Oct 25, 2016 at 11:02 PM, Tony Lindgren <tony@atomide.com> wrote: > > > Having the pin control framework call pin controller functions > > before it's probe has finished is not nice as the pin controller > > device driver does not yet have struct pinctrl_dev handle. > > > > Let's fix this issue by adding deferred work for hogs. This is > > needed to be able to add pinctrl generic helper functions. > > > > Note that the pinctrl functions already take care of the necessary > > locking. > > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > I don't see why this is necessary? It's needed because the pin controller driver has not yet finished it's probe at this point. We end up calling functions in the device driver where no struct pinctrl_dev is yet known to the driver. Asking a device driver to do something before it's probe is done does not quite follow the Linux driver model :) > The hogging was placed inside pinctrl_register() so that any hogs > would be taken before it returns, so nothing else can take it > before the controller itself has the first chance. This semantic > needs to be preserved I think. > > > + schedule_delayed_work(&pctldev->hog_work, > > + msecs_to_jiffies(100)); > > If we arbitrarily delay, something else can go in and take the > pins used by the hogs before the pinctrl core? That is what > we want to avoid. > > Hm, 100ms seems arbitrarily chosen BTW. Can it be 1 ms? > 1 ns? Yeah well seems like it should not matter but the race we need to remove somehow. > I'm pretty sure that whatever it is that needs to happen before > the hog work runs can race with this delayed work under > some circumstances (such as slow external expanders > on i2c). It should be impossible for that to happen > and I don't think it is? Yes it's totally possible even with delay set to 0. Maybe we could add some trigger on the first consumer request and if that does not happen use the timer? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Tony Lindgren <tony@atomide.com> [161111 12:27]: > * Linus Walleij <linus.walleij@linaro.org> [161111 12:17]: > > On Tue, Oct 25, 2016 at 11:02 PM, Tony Lindgren <tony@atomide.com> wrote: > > > > > Having the pin control framework call pin controller functions > > > before it's probe has finished is not nice as the pin controller > > > device driver does not yet have struct pinctrl_dev handle. > > > > > > Let's fix this issue by adding deferred work for hogs. This is > > > needed to be able to add pinctrl generic helper functions. > > > > > > Note that the pinctrl functions already take care of the necessary > > > locking. > > > > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > > > I don't see why this is necessary? > > It's needed because the pin controller driver has not yet > finished it's probe at this point. We end up calling functions > in the device driver where no struct pinctrl_dev is yet known > to the driver. Asking a device driver to do something before > it's probe is done does not quite follow the Linux driver model :) To clarify, that's an issue with multiple instances of the same driver probing as there's no static pointer to driver specific data. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Tony Lindgren <tony@atomide.com> [161111 12:32]: > * Tony Lindgren <tony@atomide.com> [161111 12:27]: > > * Linus Walleij <linus.walleij@linaro.org> [161111 12:17]: > > > On Tue, Oct 25, 2016 at 11:02 PM, Tony Lindgren <tony@atomide.com> wrote: > > > > > > > Having the pin control framework call pin controller functions > > > > before it's probe has finished is not nice as the pin controller > > > > device driver does not yet have struct pinctrl_dev handle. > > > > > > > > Let's fix this issue by adding deferred work for hogs. This is > > > > needed to be able to add pinctrl generic helper functions. > > > > > > > > Note that the pinctrl functions already take care of the necessary > > > > locking. > > > > > > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > > > > > I don't see why this is necessary? > > > > It's needed because the pin controller driver has not yet > > finished it's probe at this point. We end up calling functions > > in the device driver where no struct pinctrl_dev is yet known > > to the driver. Asking a device driver to do something before > > it's probe is done does not quite follow the Linux driver model :) > > To clarify, that's an issue with multiple instances of the same > driver probing as there's no static pointer to driver specific > data. To clarify even more, the following patches in this series need struct pinctrl_dev to pass to the generic functions :) Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -1738,6 +1738,35 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev) } /** + * pinctrl_hog_work() - delayed work to set pincontroller self-claimed pins + * @work: work struct + */ +static void pinctrl_hog_work(struct work_struct *work) +{ + struct pinctrl_dev *pctldev; + + pctldev = container_of(work, struct pinctrl_dev, hog_work.work); + + pctldev->p = pinctrl_get(pctldev->dev); + if (IS_ERR(pctldev->p)) + return; + + pctldev->hog_default = + pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT); + if (IS_ERR(pctldev->hog_default)) { + dev_dbg(pctldev->dev, "failed to lookup the default state\n"); + } else { + if (pinctrl_select_state(pctldev->p, pctldev->hog_default)) + dev_err(pctldev->dev, "failed to select default state\n"); + } + + pctldev->hog_sleep = pinctrl_lookup_state(pctldev->p, + PINCTRL_STATE_SLEEP); + if (IS_ERR(pctldev->hog_sleep)) + dev_dbg(pctldev->dev, "failed to lookup the sleep state\n"); +} + +/** * pinctrl_register() - register a pin controller device * @pctldesc: descriptor for this pin controller * @dev: parent device for this pin controller @@ -1766,6 +1795,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc, pctldev->driver_data = driver_data; INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL); INIT_LIST_HEAD(&pctldev->gpio_ranges); + INIT_DELAYED_WORK(&pctldev->hog_work, pinctrl_hog_work); pctldev->dev = dev; mutex_init(&pctldev->mutex); @@ -1804,26 +1834,8 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc, list_add_tail(&pctldev->node, &pinctrldev_list); mutex_unlock(&pinctrldev_list_mutex); - pctldev->p = pinctrl_get(pctldev->dev); - - if (!IS_ERR(pctldev->p)) { - pctldev->hog_default = - pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT); - if (IS_ERR(pctldev->hog_default)) { - dev_dbg(dev, "failed to lookup the default state\n"); - } else { - if (pinctrl_select_state(pctldev->p, - pctldev->hog_default)) - dev_err(dev, - "failed to select default state\n"); - } - - pctldev->hog_sleep = - pinctrl_lookup_state(pctldev->p, - PINCTRL_STATE_SLEEP); - if (IS_ERR(pctldev->hog_sleep)) - dev_dbg(dev, "failed to lookup the sleep state\n"); - } + schedule_delayed_work(&pctldev->hog_work, + msecs_to_jiffies(100)); pinctrl_init_device_debugfs(pctldev); @@ -1848,6 +1860,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev) if (pctldev == NULL) return; + cancel_delayed_work_sync(&pctldev->hog_work); mutex_lock(&pctldev->mutex); pinctrl_remove_device_debugfs(pctldev); mutex_unlock(&pctldev->mutex); diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h --- a/drivers/pinctrl/core.h +++ b/drivers/pinctrl/core.h @@ -33,6 +33,7 @@ struct pinctrl_gpio_range; * @p: result of pinctrl_get() for this device * @hog_default: default state for pins hogged by this device * @hog_sleep: sleep state for pins hogged by this device + * @hog_work: delayed work for pin controller device claimed hog pins * @mutex: mutex taken on each pin controller specific action * @device_root: debugfs root for this device */ @@ -47,6 +48,7 @@ struct pinctrl_dev { struct pinctrl *p; struct pinctrl_state *hog_default; struct pinctrl_state *hog_sleep; + struct delayed_work hog_work; struct mutex mutex; #ifdef CONFIG_DEBUG_FS struct dentry *device_root;
Having the pin control framework call pin controller functions before it's probe has finished is not nice as the pin controller device driver does not yet have struct pinctrl_dev handle. Let's fix this issue by adding deferred work for hogs. This is needed to be able to add pinctrl generic helper functions. Note that the pinctrl functions already take care of the necessary locking. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/pinctrl/core.c | 53 +++++++++++++++++++++++++++++++------------------- drivers/pinctrl/core.h | 2 ++ 2 files changed, 35 insertions(+), 20 deletions(-)