| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170228092538.gne33rswfnlhasrk@t450s.lan>
Date: Tue, 28 Feb 2017 10:25:38 +0100
From: Gary Bisson <gary.bisson@...ndarydevices.com>
To: Tony Lindgren <tony@...mide.com>
Cc: Mika Penttilä <mika.penttila@...tfour.com>,
LKML <linux-kernel@...r.kernel.org>, linus.walleij@...aro.org
Subject: Re: [REGRESSION] pinctrl, of, unable to find hogs
Hi Tony,
On Mon, Feb 27, 2017 at 03:05:43PM -0800, Tony Lindgren wrote:
> * Gary Bisson <gary.bisson@...ndarydevices.com> [170227 13:08]:
> > Hi Tony,
> >
> > On Mon, Feb 27, 2017 at 10:45:35AM -0800, Tony Lindgren wrote:
> > > * Tony Lindgren <tony@...mide.com> [170227 09:37]:
> > > > * Gary Bisson <gary.bisson@...ndarydevices.com> [170227 08:42]:
> > > > > > Not sure how to fix it though since we can't move the dt probing before
> > > > > > radix tree init.
> > > >
> > > > Yup looks like we still have an issue with pinctrl driver functions
> > > > getting called before driver probe has completed.
> > > >
> > > > How about we introduce something like:
> > > >
> > > > int pinctrl_claim_hogs(struct pinctrl_dev *pctldev);
> > > >
> > > > Then the drivers can call that at the end of the probe after
> > > > the pins have been parsed?
> > > >
> > > > This should be safe as no other driver can claim the pins either
> > > > before the pins have been parsed :)
> > >
> > > Below is an initial take on this solution. I've only briefly tested
> > > it so far but maybe give it a try and see if it helps.
> > >
> > > I'll take a look if we can make the error handling better for
> > > pinctrl_register_and_init().
> >
> > I'll try that tomorrow morning and let you know.
>
> Actually that one is not considering that it's OK to return -ENODEV
> for hogs if not found. Below is what I think is a better version,
> compile tested only for imx6 though. I boot tested the similar changes
> with pinctrl-single with an additional patch.
>
> It just splits up things further and exports these for pin controller
> drivers to use:
>
> pinctrl_init_controller
> pinctrl_claim_hogs
> pinctrl_enable
>
> Splitting things that way allows parsing the pins dynamically like
> you do. And that can be then used later on for other pin controller
> drivers to simplify things further.
I tested your patch and confirm it works.
Tested-by: Gary Bisson <gary.bisson@...ndarydevices.com>
I made one change though, see below.
> I wonder if we should drop the pinctrl_register_and_init() we recently
> introduced in favor of init + claim_hogs + enable. Linus, what's your
> preference, keep or drop pinctrl_register_and_init()?
Indeed it doesn't strike me as really necessary. But I guess the
question is now: which option is the best/acceptable for 4.11?
> Regards,
>
> Tony
>
> 8< ---------------------
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -2009,32 +2009,50 @@ struct pinctrl_dev *pinctrl_init_controller(struct pinctrl_desc *pctldesc,
> kfree(pctldev);
> return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL_GPL(pinctrl_init_controller);
>
> -static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
> +int pinctrl_claim_hogs(struct pinctrl_dev *pctldev)
> {
> pctldev->p = create_pinctrl(pctldev->dev, pctldev);
> - if (!IS_ERR(pctldev->p)) {
> - kref_get(&pctldev->p->users);
> - 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");
> - }
> + if (PTR_ERR(pctldev->p) == -ENODEV) {
> + dev_dbg(pctldev->dev, "no hogs found\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");
> + return 0;
> + }
> +
> + if (IS_ERR(pctldev->p)) {
> + dev_err(pctldev->dev, "error claiming hogs: %li\n",
> + PTR_ERR(pctldev->p));
> +
> + return PTR_ERR(pctldev->p);
> }
>
> + kref_get(&pctldev->p->users);
> + 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");
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pinctrl_claim_hogs);
> +
> +void pinctrl_enable(struct pinctrl_dev *pctldev)
> +{
> mutex_lock(&pinctrldev_list_mutex);
> list_add_tail(&pctldev->node, &pinctrldev_list);
> mutex_unlock(&pinctrldev_list_mutex);
> @@ -2043,6 +2061,24 @@ static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(pinctrl_enable);
> +
> +static int pinctrl_create_and_start(struct pinctrl_dev *pctldev)
> +{
> + int error;
> +
> + error = pinctrl_claim_hogs(pctldev);
> + if (error) {
> + dev_err(pctldev->dev, "could not claim hogs: %i\n",
> + error);
> +
> + return error;
> + }
> +
> + pinctrl_enable(pctldev);
> +
> + return 0;
> +}
>
> /**
> * pinctrl_register() - register a pin controller device
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -774,10 +774,10 @@ int imx_pinctrl_probe(struct platform_device *pdev,
> ipctl->info = info;
> ipctl->dev = info->dev;
> platform_set_drvdata(pdev, ipctl);
> - ret = devm_pinctrl_register_and_init(&pdev->dev,
> - imx_pinctrl_desc, ipctl,
> - &ipctl->pctl);
> - if (ret) {
> +
> + ipctl->pctl = pinctrl_init_controller(imx_pinctrl_desc, &pdev->dev,
> + ipctl);
> + if (IS_ERR(ipctl->pctl)) {
> dev_err(&pdev->dev, "could not register IMX pinctrl driver\n");
Here you need to add:
ret = PTR_ERR(ipctl->pctl);
Otherwise the return value will be undetermined (and a warning shows
up).
Regards,
Gary
Powered by blists - more mailing lists