gpiolib: Fix logic for driver override

Message ID 20180503221118.20754-1-chris.lesiak@licor.com
State New
Headers show
Series
  • gpiolib: Fix logic for driver override
Related show

Commit Message

Chris Lesiak May 3, 2018, 10:11 p.m.
Fix incorrect logic in gpiochip_irqchip_add_key().
A driver needs to override both irq_request_resources and
irq_request_resources, otherwise gpiochip_irq_reqres and
gpiochip_irq_relres should be used.

Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Phil Reid May 4, 2018, 12:55 a.m. | #1
On 4/05/2018 06:11, Chris Lesiak wrote:
> Fix incorrect logic in gpiochip_irqchip_add_key().
> A driver needs to override both irq_request_resources and
> irq_request_resources, otherwise gpiochip_irq_reqres and
irq_request_resources repeated.

irq_release_resources?

> gpiochip_irq_relres should be used.
> 
> Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>
> ---
>   drivers/gpio/gpiolib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bdd68ff197dc..3e441879fea7 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1831,7 +1831,7 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
>   	 * It is possible for a driver to override this, but only if the
>   	 * alternative functions are both implemented.
>   	 */
> -	if (!irqchip->irq_request_resources &&
> +	if (!irqchip->irq_request_resources ||
>   	    !irqchip->irq_release_resources) {
>   		irqchip->irq_request_resources = gpiochip_irq_reqres;
>   		irqchip->irq_release_resources = gpiochip_irq_relres;
>
Linus Walleij May 16, 2018, 12:49 p.m. | #2
On Fri, May 4, 2018 at 12:11 AM, Chris Lesiak <chris.lesiak@licor.com> wrote:

> Fix incorrect logic in gpiochip_irqchip_add_key().
> A driver needs to override both irq_request_resources and
> irq_request_resources, otherwise gpiochip_irq_reqres and
> gpiochip_irq_relres should be used.
>
> Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>
> ---
>  drivers/gpio/gpiolib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bdd68ff197dc..3e441879fea7 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1831,7 +1831,7 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
>          * It is possible for a driver to override this, but only if the
>          * alternative functions are both implemented.
>          */
> -       if (!irqchip->irq_request_resources &&
> +       if (!irqchip->irq_request_resources ||
>             !irqchip->irq_release_resources) {
>                 irqchip->irq_request_resources = gpiochip_irq_reqres;
>                 irqchip->irq_release_resources = gpiochip_irq_relres;

This code was refactored by Thierry, but the logic is from
Rabin's patch
commit 8b67a1f0ad1f260f1a4032d5f7b032ac113bfa7d
"gpio: don't override irq_*_resources() callbacks"

I think the intention was to only allow gpiolib's implementation
to kick in if the driver provided no callbacks at all.

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
Chris Lesiak May 16, 2018, 1:34 p.m. | #3
On 05/16/2018 07:49 AM, Linus Walleij wrote:

> On Fri, May 4, 2018 at 12:11 AM, Chris Lesiak <chris.lesiak@licor.com> wrote:
>
>> Fix incorrect logic in gpiochip_irqchip_add_key().
>> A driver needs to override both irq_request_resources and
>> irq_request_resources, otherwise gpiochip_irq_reqres and
>> gpiochip_irq_relres should be used.
>>
>> Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>
>> ---
>>   drivers/gpio/gpiolib.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index bdd68ff197dc..3e441879fea7 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1831,7 +1831,7 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
>>           * It is possible for a driver to override this, but only if the
>>           * alternative functions are both implemented.
>>           */
>> -       if (!irqchip->irq_request_resources &&
>> +       if (!irqchip->irq_request_resources ||
>>              !irqchip->irq_release_resources) {
>>                  irqchip->irq_request_resources = gpiochip_irq_reqres;
>>                  irqchip->irq_release_resources = gpiochip_irq_relres;
> This code was refactored by Thierry, but the logic is from
> Rabin's patch
> commit 8b67a1f0ad1f260f1a4032d5f7b032ac113bfa7d
> "gpio: don't override irq_*_resources() callbacks"
>
> I think the intention was to only allow gpiolib's implementation
> to kick in if the driver provided no callbacks at all.

I agree that is what Rabin's patch implemented, but it doesn't match the 
patches comment.

The code in kernel/irq/manage.c is certainly happy of one (or both) of 
irqchip->irq_request_resources and irqchip->irq_release_resources are 
null.  But it seems likely that a driver, if needing to override at all, 
would need to override the pair.  The existing drivers follow that pattern.

As such, I think Rabin's comment was correct, but his code wrong.
Thierry Reding May 16, 2018, 1:43 p.m. | #4
On Wed, May 16, 2018 at 08:34:36AM -0500, Chris Lesiak wrote:
> On 05/16/2018 07:49 AM, Linus Walleij wrote:
> 
> > On Fri, May 4, 2018 at 12:11 AM, Chris Lesiak <chris.lesiak@licor.com> wrote:
> > 
> > > Fix incorrect logic in gpiochip_irqchip_add_key().
> > > A driver needs to override both irq_request_resources and
> > > irq_request_resources, otherwise gpiochip_irq_reqres and
> > > gpiochip_irq_relres should be used.
> > > 
> > > Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>
> > > ---
> > >   drivers/gpio/gpiolib.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index bdd68ff197dc..3e441879fea7 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -1831,7 +1831,7 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
> > >           * It is possible for a driver to override this, but only if the
> > >           * alternative functions are both implemented.
> > >           */
> > > -       if (!irqchip->irq_request_resources &&
> > > +       if (!irqchip->irq_request_resources ||
> > >              !irqchip->irq_release_resources) {
> > >                  irqchip->irq_request_resources = gpiochip_irq_reqres;
> > >                  irqchip->irq_release_resources = gpiochip_irq_relres;
> > This code was refactored by Thierry, but the logic is from
> > Rabin's patch
> > commit 8b67a1f0ad1f260f1a4032d5f7b032ac113bfa7d
> > "gpio: don't override irq_*_resources() callbacks"
> > 
> > I think the intention was to only allow gpiolib's implementation
> > to kick in if the driver provided no callbacks at all.
> 
> I agree that is what Rabin's patch implemented, but it doesn't match the
> patches comment.
> 
> The code in kernel/irq/manage.c is certainly happy of one (or both) of
> irqchip->irq_request_resources and irqchip->irq_release_resources are null. 
> But it seems likely that a driver, if needing to override at all, would need
> to override the pair.  The existing drivers follow that pattern.
> 
> As such, I think Rabin's comment was correct, but his code wrong.

I disagree. I don't think the core should be overriding anything that
the driver has explicitly set up. I don't see why a driver shouldn't be
allowed to set up only one of them. What if ->irq_release_resources()
doesn't need to do anything for a specific driver? Should the driver be
required to provide an empty dummy just so the core doesn't override an
implementation of ->irq_request_resources() that the driver specified?

Thierry
Chris Lesiak May 16, 2018, 2:12 p.m. | #5
On 05/16/2018 08:43 AM, Thierry Reding wrote:

> On Wed, May 16, 2018 at 08:34:36AM -0500, Chris Lesiak wrote:
>> On 05/16/2018 07:49 AM, Linus Walleij wrote:
>>
>>> On Fri, May 4, 2018 at 12:11 AM, Chris Lesiak <chris.lesiak@licor.com> wrote:
>>>
>>>> Fix incorrect logic in gpiochip_irqchip_add_key().
>>>> A driver needs to override both irq_request_resources and
>>>> irq_request_resources, otherwise gpiochip_irq_reqres and
>>>> gpiochip_irq_relres should be used.
>>>>
>>>> Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>
>>>> ---
>>>>    drivers/gpio/gpiolib.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>>> index bdd68ff197dc..3e441879fea7 100644
>>>> --- a/drivers/gpio/gpiolib.c
>>>> +++ b/drivers/gpio/gpiolib.c
>>>> @@ -1831,7 +1831,7 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
>>>>            * It is possible for a driver to override this, but only if the
>>>>            * alternative functions are both implemented.
>>>>            */
>>>> -       if (!irqchip->irq_request_resources &&
>>>> +       if (!irqchip->irq_request_resources ||
>>>>               !irqchip->irq_release_resources) {
>>>>                   irqchip->irq_request_resources = gpiochip_irq_reqres;
>>>>                   irqchip->irq_release_resources = gpiochip_irq_relres;
>>> This code was refactored by Thierry, but the logic is from
>>> Rabin's patch
>>> commit 8b67a1f0ad1f260f1a4032d5f7b032ac113bfa7d
>>> "gpio: don't override irq_*_resources() callbacks"
>>>
>>> I think the intention was to only allow gpiolib's implementation
>>> to kick in if the driver provided no callbacks at all.
>> I agree that is what Rabin's patch implemented, but it doesn't match the
>> patches comment.
>>
>> The code in kernel/irq/manage.c is certainly happy of one (or both) of
>> irqchip->irq_request_resources and irqchip->irq_release_resources are null.
>> But it seems likely that a driver, if needing to override at all, would need
>> to override the pair.  The existing drivers follow that pattern.
>>
>> As such, I think Rabin's comment was correct, but his code wrong.
>>

To correct the record, I see that the comment was actually added to 
Rabin's patch by Linus.

> I disagree. I don't think the core should be overriding anything that
> the driver has explicitly set up. I don't see why a driver shouldn't be
> allowed to set up only one of them. What if ->irq_release_resources()
> doesn't need to do anything for a specific driver? Should the driver be
> required to provide an empty dummy just so the core doesn't override an
> implementation of ->irq_request_resources() that the driver specified?

A reasonable argument.


--
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 May 23, 2018, 11:43 a.m. | #6
On Wed, May 16, 2018 at 4:12 PM, Chris Lesiak <chris.lesiak@licor.com> wrote:
> On 05/16/2018 08:43 AM, Thierry Reding wrote:

> To correct the record, I see that the comment was actually added to Rabin's
> patch by Linus.

Oh I take the full blame.

>> I disagree. I don't think the core should be overriding anything that
>> the driver has explicitly set up. I don't see why a driver shouldn't be
>> allowed to set up only one of them. What if ->irq_release_resources()
>> doesn't need to do anything for a specific driver? Should the driver be
>> required to provide an empty dummy just so the core doesn't override an
>> implementation of ->irq_request_resources() that the driver specified?
>
> A reasonable argument.

The driver this was added for (ETRAX) is now gone.

I looked around and AFAICT only one other driver actually uses
this facility to override .irq_request/release_resources():
drivers/pinctrl/pinctrl-st.c

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

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bdd68ff197dc..3e441879fea7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1831,7 +1831,7 @@  int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 	 * It is possible for a driver to override this, but only if the
 	 * alternative functions are both implemented.
 	 */
-	if (!irqchip->irq_request_resources &&
+	if (!irqchip->irq_request_resources ||
 	    !irqchip->irq_release_resources) {
 		irqchip->irq_request_resources = gpiochip_irq_reqres;
 		irqchip->irq_release_resources = gpiochip_irq_relres;