| 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: <20170227230542.GQ21809@atomide.com>
Date: Mon, 27 Feb 2017 15:05:43 -0800
From: Tony Lindgren <tony@...mide.com>
To: Gary Bisson <gary.bisson@...ndarydevices.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
* 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 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()?
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");
goto free;
}
@@ -788,8 +788,16 @@ int imx_pinctrl_probe(struct platform_device *pdev,
goto free;
}
+ ret = pinctrl_claim_hogs(ipctl->pctl);
+ if (ret) {
+ dev_err(&pdev->dev, "could not claim hogs: %i\n", ret);
+ goto free;
+ }
+
dev_info(&pdev->dev, "initialized IMX pinctrl driver\n");
+ pinctrl_enable(ipctl->pctl);
+
return 0;
free:
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -146,7 +146,14 @@ extern int pinctrl_register_and_init(struct pinctrl_desc *pctldesc,
struct device *dev, void *driver_data,
struct pinctrl_dev **pctldev);
-/* Please use pinctrl_register_and_init() instead */
+/* Use these if dynamically allocating pins */
+extern struct pinctrl_dev *
+pinctrl_init_controller(struct pinctrl_desc *pctldesc, struct device *dev,
+ void *driver_data);
+extern int pinctrl_claim_hogs(struct pinctrl_dev *pctldev);
+extern void pinctrl_enable(struct pinctrl_dev *pctldev);
+
+/* Please use pinctrl_register_and_init() or the above instead */
extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
struct device *dev, void *driver_data);
--
2.11.1
Powered by blists - more mailing lists