diff mbox series

[v2] gpiolib: Show correct direction from the beginning

Message ID 20180921103604.13361-2-ricardo.ribalda@gmail.com
State New
Headers show
Series [v2] gpiolib: Show correct direction from the beginning | expand

Commit Message

Ricardo Ribalda Delgado Sept. 21, 2018, 10:36 a.m. UTC
Current code assumes that the direction is input if direction_input
function is set.
This might not be the case on GPIOs with programmable direction.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/gpio/gpiolib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Timur Tabi Sept. 21, 2018, 12:25 p.m. UTC | #1
Jeff, can you test these two patches on Amberwing to make sure that they 
don't cause an XPU violation on boot?

The call to gpiochip_line_is_valid() should return false on any GPIOs 
that aren't listed in the ACPI table.

My concern is that this patch might be calling gpiochip_line_is_valid() 
too early, before all the arrays have been set up.

Thanks.

On 9/21/18 5:36 AM, Ricardo Ribalda Delgado wrote:
> Current code assumes that the direction is input if direction_input
> function is set.
> This might not be the case on GPIOs with programmable direction.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>   drivers/gpio/gpiolib.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4b45de883ada..00c17f64d9ff 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1352,7 +1352,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
>   		 * it does, and in case chip->get_direction is not set, we may
>   		 * expose the wrong direction in sysfs.
>   		 */
> -		desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
> +		if (chip->get_direction && gpiochip_line_is_valid(chip, i))
> +			desc->flags = !chip->get_direction(chip, i) ?
> +					(1 << FLAG_IS_OUT) : 0;
> +		else
> +			desc->flags = !chip->direction_input ?
> +					(1 << FLAG_IS_OUT) : 0;
>   	}
>   
>   #ifdef CONFIG_PINCTRL
>
Stephen Boyd Sept. 27, 2018, 6:51 a.m. UTC | #2
Quoting Ricardo Ribalda Delgado (2018-09-21 03:36:04)
> Current code assumes that the direction is input if direction_input
> function is set.
> This might not be the case on GPIOs with programmable direction.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Looks OK to me visually. I haven't tested it because I don't have access
to the locked down hardware anymore.

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Timur Tabi Sept. 27, 2018, 12:19 p.m. UTC | #3
On 9/27/18 1:51 AM, Stephen Boyd wrote:
> Looks OK to me visually. I haven't tested it because I don't have access
> to the locked down hardware anymore.

Same here.  Please wait for Jeff Hugo to test it before applying.  Thanks.
Jeffrey Hugo Sept. 27, 2018, 2:04 p.m. UTC | #4
On 9/27/2018 6:19 AM, Timur Tabi wrote:
> On 9/27/18 1:51 AM, Stephen Boyd wrote:
>> Looks OK to me visually. I haven't tested it because I don't have access
>> to the locked down hardware anymore.
> 
> Same here.  Please wait for Jeff Hugo to test it before applying.  Thanks.

I guess its lucky I saw this then.

I should be able to test within a week.
Timur Tabi Sept. 27, 2018, 2:19 p.m. UTC | #5
On 9/27/18 9:04 AM, Jeffrey Hugo wrote:
> 
> I guess its lucky I saw this then.

Did you not get this email: 
https://lore.kernel.org/patchwork/patch/989545/#1173771
Jeffrey Hugo Sept. 27, 2018, 2:34 p.m. UTC | #6
On 9/27/2018 8:19 AM, Timur Tabi wrote:
> On 9/27/18 9:04 AM, Jeffrey Hugo wrote:
>>
>> I guess its lucky I saw this then.
> 
> Did you not get this email: 
> https://lore.kernel.org/patchwork/patch/989545/#1173771

Apparently I did.  I found it in my deleted items.  I must have 
accidentally done that when sorting through my backlog after Linaro 
Connect.  Sorry about that.
Jeffrey Hugo Sept. 28, 2018, 7:14 p.m. UTC | #7
On 9/27/2018 8:04 AM, Jeffrey Hugo wrote:
> On 9/27/2018 6:19 AM, Timur Tabi wrote:
>> On 9/27/18 1:51 AM, Stephen Boyd wrote:
>>> Looks OK to me visually. I haven't tested it because I don't have access
>>> to the locked down hardware anymore.
>>
>> Same here.  Please wait for Jeff Hugo to test it before applying.  
>> Thanks.
> 
> I guess its lucky I saw this then.
> 
> I should be able to test within a week.
> 

Nack.  Causes a XPU violation to the GPIO config registers.
Timur Tabi Sept. 28, 2018, 7:22 p.m. UTC | #8
On Fri, Sep 28, 2018 at 2:14 PM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> Nack.  Causes a XPU violation to the GPIO config registers.

That doesn't surprise me at all.

I believe that the problem is that gpiochip_line_is_valid() is being
called before gpiochip_irqchip_init_valid_mask() is called, which
means that gpiochip_line_is_valid() always returns true.
Ricardo Ribalda Delgado Sept. 29, 2018, 6:23 a.m. UTC | #9
Hi Timur

In fact  gpiochip_init_valid_mask is called some lines after in the
same function. We could reorder the function. Would that work for you?

The driver breaking is upstream? Is it possible to access the hardware?

Thanks

[Sorry for the two html mails in a row, I did not try to work with my
phone before :) ]
On Fri, Sep 28, 2018 at 9:23 PM Timur Tabi <timur@kernel.org> wrote:
>
> On Fri, Sep 28, 2018 at 2:14 PM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> > Nack.  Causes a XPU violation to the GPIO config registers.
>
> That doesn't surprise me at all.
>
> I believe that the problem is that gpiochip_line_is_valid() is being
> called before gpiochip_irqchip_init_valid_mask() is called, which
> means that gpiochip_line_is_valid() always returns true.
Timur Tabi Sept. 29, 2018, 1:21 p.m. UTC | #10
On 9/29/18 1:23 AM, Ricardo Ribalda Delgado wrote:
> In fact  gpiochip_init_valid_mask is called some lines after in the
> same function. We could reorder the function. Would that work for you?

It might.  It might break something else, though.

> The driver breaking is upstream?

Yes.

> Is it possible to access the hardware?

Linaro and some Linux OSVs should still have systems, but I usually just 
ask Jeff to test code for me.
Timur Tabi Sept. 29, 2018, 1:25 p.m. UTC | #11
On 9/29/18 8:21 AM, Timur Tabi wrote:
> 
>> Is it possible to access the hardware?
> 
> Linaro and some Linux OSVs should still have systems, but I usually just 
> ask Jeff to test code for me.

Alternatively, you can just add valid_mask support to your driver, and 
add a check to your get_direction() function to see if it ever is asked 
to access an invalid GPIO.
Linus Walleij Oct. 1, 2018, 11:54 a.m. UTC | #12
On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:

> How do we proceed from here? Can you fix your driver somehow to
> init the valid mask before enabling the gpio?

Just include a hunk to the qcom driver reordering this call
at the same time. No need to make it separate patches,
it need to be tested together anyways.

I guess just switch the order of these two:

        ret = gpiochip_add_data(&pctrl->chip, pctrl);
        if (ret) {
                dev_err(pctrl->dev, "Failed register gpiochip\n");
                return ret;
        }

        ret = msm_gpio_init_valid_mask(chip, pctrl);
        if (ret) {
                dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
                gpiochip_remove(&pctrl->chip);
                return ret;
        }

> Do we need to make more severe changes on the core?

Don't think so.

Yours,
Linus Walleij
Ricardo Ribalda Delgado Oct. 1, 2018, 1:36 p.m. UTC | #13
Hi Linus
On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
>
> > How do we proceed from here? Can you fix your driver somehow to
> > init the valid mask before enabling the gpio?
>
> Just include a hunk to the qcom driver reordering this call
> at the same time. No need to make it separate patches,
> it need to be tested together anyways.
>
> I guess just switch the order of these two:
>
>         ret = gpiochip_add_data(&pctrl->chip, pctrl);
>         if (ret) {
>                 dev_err(pctrl->dev, "Failed register gpiochip\n");
>                 return ret;
>         }
>
>         ret = msm_gpio_init_valid_mask(chip, pctrl);
>         if (ret) {
>                 dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
>                 gpiochip_remove(&pctrl->chip);
>                 return ret;
>         }
>

the problem is that valid_mask is not a long/integer, is a struct that
needs to be malloced, and is malloc at gpiochip_add_data :(

Maybe we need a callback from the driver to init that mask just after
the allocation?

A fast grep shows that the only driver using need_valid_mask (not for
irq) is msm:

ricardo@neopili:~/curro/kernel-upstream$ git grep "need_valid_mask ="
| grep -v irq
drivers/gpio/gpiolib.c: gpiochip->need_valid_mask = true;
drivers/pinctrl/intel/pinctrl-cherryview.c: bool need_valid_mask =
!dmi_check_system(chv_no_valid_mask);
drivers/pinctrl/qcom/pinctrl-msm.c: chip->need_valid_mask =
msm_gpio_needs_valid_mask(pctrl);

so hacking something in the driver might not be a terrible idea.



Also

> > Do we need to make more severe changes on the core?
>
> Don't think so.
>
> Yours,
> Linus Walleij
Jeffrey Hugo Oct. 1, 2018, 2:27 p.m. UTC | #14
On 10/1/2018 7:36 AM, Ricardo Ribalda Delgado wrote:
> Hi Linus
> On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
>> <ricardo.ribalda@gmail.com> wrote:
>>
>>> How do we proceed from here? Can you fix your driver somehow to
>>> init the valid mask before enabling the gpio?
>>
>> Just include a hunk to the qcom driver reordering this call
>> at the same time. No need to make it separate patches,
>> it need to be tested together anyways.
>>
>> I guess just switch the order of these two:
>>
>>          ret = gpiochip_add_data(&pctrl->chip, pctrl);
>>          if (ret) {
>>                  dev_err(pctrl->dev, "Failed register gpiochip\n");
>>                  return ret;
>>          }
>>
>>          ret = msm_gpio_init_valid_mask(chip, pctrl);
>>          if (ret) {
>>                  dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
>>                  gpiochip_remove(&pctrl->chip);
>>                  return ret;
>>          }
>>
> 
> the problem is that valid_mask is not a long/integer, is a struct that
> needs to be malloced, and is malloc at gpiochip_add_data :(
> 
> Maybe we need a callback from the driver to init that mask just after
> the allocation?
> 
> A fast grep shows that the only driver using need_valid_mask (not for
> irq) is msm:
> 
> ricardo@neopili:~/curro/kernel-upstream$ git grep "need_valid_mask ="
> | grep -v irq
> drivers/gpio/gpiolib.c: gpiochip->need_valid_mask = true;
> drivers/pinctrl/intel/pinctrl-cherryview.c: bool need_valid_mask =
> !dmi_check_system(chv_no_valid_mask);
> drivers/pinctrl/qcom/pinctrl-msm.c: chip->need_valid_mask =
> msm_gpio_needs_valid_mask(pctrl);
> 
> so hacking something in the driver might not be a terrible idea.
> 

IMO, hack up the driver and I'll test it.  We can figure out what is 
needed to work, then determine what is the proper solution for that.
Linus Walleij Oct. 1, 2018, 9:20 p.m. UTC | #15
On Mon, Oct 1, 2018 at 3:36 PM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
> > <ricardo.ribalda@gmail.com> wrote:
> >
> > > How do we proceed from here? Can you fix your driver somehow to
> > > init the valid mask before enabling the gpio?
> >
> > Just include a hunk to the qcom driver reordering this call
> > at the same time. No need to make it separate patches,
> > it need to be tested together anyways.
> >
> > I guess just switch the order of these two:
> >
> >         ret = gpiochip_add_data(&pctrl->chip, pctrl);
> >         if (ret) {
> >                 dev_err(pctrl->dev, "Failed register gpiochip\n");
> >                 return ret;
> >         }
> >
> >         ret = msm_gpio_init_valid_mask(chip, pctrl);
> >         if (ret) {
> >                 dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
> >                 gpiochip_remove(&pctrl->chip);
> >                 return ret;
> >         }
> >
>
> the problem is that valid_mask is not a long/integer, is a struct that
> needs to be malloced, and is malloc at gpiochip_add_data :(

I don't get it, but maybe I'm not smart enough.

gpiochip_add_data() doesn't allocate anything, it
just adds a already allocated (or static!) gpio_chip
to the gpiolib subsystem.

In fact I think it is wrong to set up the mask after
calling gpiolob_add_data(), because of exactly the
type of problem you're seeing.

Don't get confused by the &pctrl->chip
vs just chip variables, it's just some sloppiness.

Yours,
Linus Walleij
Ricardo Ribalda Delgado Oct. 2, 2018, 7:15 a.m. UTC | #16
Hi
On Mon, Oct 1, 2018 at 11:20 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Oct 1, 2018 at 3:36 PM Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
> > On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
> > > <ricardo.ribalda@gmail.com> wrote:
> > >
> > > > How do we proceed from here? Can you fix your driver somehow to
> > > > init the valid mask before enabling the gpio?
> > >
> > > Just include a hunk to the qcom driver reordering this call
> > > at the same time. No need to make it separate patches,
> > > it need to be tested together anyways.
> > >
> > > I guess just switch the order of these two:
> > >
> > >         ret = gpiochip_add_data(&pctrl->chip, pctrl);
> > >         if (ret) {
> > >                 dev_err(pctrl->dev, "Failed register gpiochip\n");
> > >                 return ret;
> > >         }
> > >
> > >         ret = msm_gpio_init_valid_mask(chip, pctrl);
> > >         if (ret) {
> > >                 dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
> > >                 gpiochip_remove(&pctrl->chip);
> > >                 return ret;
> > >         }
> > >
> >
> > the problem is that valid_mask is not a long/integer, is a struct that
> > needs to be malloced, and is malloc at gpiochip_add_data :(
>
> I don't get it, but maybe I'm not smart enough.

Dont take my job, I am the not smart of the conversation :P

>
> gpiochip_add_data() doesn't allocate anything, it
> just adds a already allocated (or static!) gpio_chip
> to the gpiolib subsystem.
>

Take a look to gpiochip_add_data_with_key()
  ->gpiochip_init_valid_mask()
       -> gpiochip->valid_mask = gpiochip_allocate_mask(gpiochip);



> In fact I think it is wrong to set up the mask after
> calling gpiolob_add_data(), because of exactly the
> type of problem you're seeing.

I agree, and I believe that the cleaner way would be to add a function
pointer that replaces:

gpiochip_allocate_mask()
  bitmap_fill(p, chip->ngpio);

with a proper initalization from the driver

But as today the only driver that seems to be using valid_mask is msm,
so perhaps a hack is something better and then when we have a second
driver that requires it we figure out the real requirements. But it is
definately your decision ;)


>
> Don't get confused by the &pctrl->chip
> vs just chip variables, it's just some sloppiness.
>
> Yours,
> Linus Walleij

Thanks!
Linus Walleij Oct. 2, 2018, 7:38 a.m. UTC | #17
On Tue, Oct 2, 2018 at 9:15 AM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> On Mon, Oct 1, 2018 at 11:20 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > gpiochip_add_data() doesn't allocate anything, it
> > just adds a already allocated (or static!) gpio_chip
> > to the gpiolib subsystem.
>
> Take a look to gpiochip_add_data_with_key()
>   ->gpiochip_init_valid_mask()
>        -> gpiochip->valid_mask = gpiochip_allocate_mask(gpiochip);

Aha I see...

> > In fact I think it is wrong to set up the mask after
> > calling gpiolob_add_data(), because of exactly the
> > type of problem you're seeing.
>
> I agree, and I believe that the cleaner way would be to add a function
> pointer that replaces:
>
> gpiochip_allocate_mask()
>   bitmap_fill(p, chip->ngpio);
>
> with a proper initalization from the driver

OK

> But as today the only driver that seems to be using valid_mask is msm,
> so perhaps a hack is something better and then when we have a second
> driver that requires it we figure out the real requirements. But it is
> definately your decision ;)

I would just add some exported function to gpiolib to do what you
need so you can set up the valid_mask before calling
gpiochip_add*.

Yours,
Linus Walleij
Timur Tabi Oct. 2, 2018, 12:26 p.m. UTC | #18
On 10/2/18 2:38 AM, Linus Walleij wrote:
>> But as today the only driver that seems to be using valid_mask is msm,
>> so perhaps a hack is something better and then when we have a second
>> driver that requires it we figure out the real requirements. But it is
>> definately your decision;)

Please note that MSM is supposed to be the *first* driver, not the only, 
driver that needs valid_mask.  So let's not make any code changes that 
limit this feature to the MSM driver.

> I would just add some exported function to gpiolib to do what you
> need so you can set up the valid_mask before calling
> gpiochip_add*.

I think that should be okay.  Drivers should know pretty early whether 
they need valid_mask or not.
Linus Walleij Oct. 2, 2018, 12:51 p.m. UTC | #19
On Tue, Oct 2, 2018 at 2:26 PM Timur Tabi <timur@kernel.org> wrote:
> On 10/2/18 2:38 AM, Linus Walleij wrote:
> >> But as today the only driver that seems to be using valid_mask is msm,
> >> so perhaps a hack is something better and then when we have a second
> >> driver that requires it we figure out the real requirements. But it is
> >> definately your decision;)
>
> Please note that MSM is supposed to be the *first* driver, not the only,
> driver that needs valid_mask.  So let's not make any code changes that
> limit this feature to the MSM driver.

I just recently encouraged Thierry to reuse this for the Tegra186
driver:
https://marc.info/?l=linux-gpio&m=153787164826821&w=2

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4b45de883ada..00c17f64d9ff 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1352,7 +1352,12 @@  int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
 		 * it does, and in case chip->get_direction is not set, we may
 		 * expose the wrong direction in sysfs.
 		 */
-		desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
+		if (chip->get_direction && gpiochip_line_is_valid(chip, i))
+			desc->flags = !chip->get_direction(chip, i) ?
+					(1 << FLAG_IS_OUT) : 0;
+		else
+			desc->flags = !chip->direction_input ?
+					(1 << FLAG_IS_OUT) : 0;
 	}
 
 #ifdef CONFIG_PINCTRL