linux-kernel - Re: [REGRESSION] pinctrl, of, unable to find hogs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170228172439.GD20572@atomide.com>
Date:   Tue, 28 Feb 2017 09:24:39 -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> [170228 01:27]:
> 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>

OK good to hear.

> I made one change though, see below.

OK

> > 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?

Yeah.. pinctrl_register_and_init() reduces some boilerplate code, but it
still has the $subject issue with hogs. So I don't know if we want to
get stuck with supporting it.

> > --- 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).

Oops thanks for catching that, will fold in.

Regards,

Tony