diff mbox series

[2/2] (v2) Mark MCP23S08 as one that will not sleep.

Message ID 20190513120024.17026-2-joe.burmeister@devtank.co.uk
State New
Headers show
Series [1/2] (v2) Expand MCP23S08 driver for use as interrupt controller. | expand

Commit Message

Joe Burmeister May 13, 2019, noon UTC 
Though it has a 'standby' it doesn't appear to be an issue and
marking the chip with can_sleep means gpiolib.c won't allow its use
as a interrupt controller.
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexander Stein May 13, 2019, 12:59 p.m. UTC | #1 
Am Montag, 13. Mai 2019, 14:00:24 CEST schrieb Joe Burmeister:
> Though it has a 'standby' it doesn't appear to be an issue and
> marking the chip with can_sleep means gpiolib.c won't allow its use
> as a interrupt controller.
> ---
>  drivers/pinctrl/pinctrl-mcp23s08.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index 3fc63cb5b332..7334d8eb9135 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -890,7 +890,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>  		return PTR_ERR(mcp->regmap);
>  
>  	mcp->chip.base = base;
> -	mcp->chip.can_sleep = true;
> +	mcp->chip.can_sleep = false;
>  	mcp->chip.parent = dev;
>  	mcp->chip.owner = THIS_MODULE;
>  
> 

IMHO this is completly wrong, please refer to the documentation of this flag, e.g. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/gpio/driver.h#n217
It essentially means you can't use this GPIOs from atomic context, as SPI or I2C transfers block/sleep, hence the name.
In your case the IRQs are probably not requested as threaded, as stated in the link above.

Best regards,
Alexander
Joe Burmeister May 13, 2019, 2:02 p.m. UTC | #2 
Hi Alexander,

You probably noticed there is a v3, but that just adds the missing signoff.

On 13/05/2019 13:59, Alexander Stein wrote:

> Am Montag, 13. Mai 2019, 14:00:24 CEST schrieb Joe Burmeister:
>> Though it has a 'standby' it doesn't appear to be an issue and
>> marking the chip with can_sleep means gpiolib.c won't allow its use
>> as a interrupt controller.
>> ---
>>   drivers/pinctrl/pinctrl-mcp23s08.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
>> index 3fc63cb5b332..7334d8eb9135 100644
>> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
>> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
>> @@ -890,7 +890,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>>   		return PTR_ERR(mcp->regmap);
>>   
>>   	mcp->chip.base = base;
>> -	mcp->chip.can_sleep = true;
>> +	mcp->chip.can_sleep = false;
>>   	mcp->chip.parent = dev;
>>   	mcp->chip.owner = THIS_MODULE;
>>   
>>
> IMHO this is completly wrong, please refer to the documentation of this flag, e.g. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/gpio/driver.h#n217
> It essentially means you can't use this GPIOs from atomic context, as SPI or I2C transfers block/sleep, hence the name.
> In your case the IRQs are probably not requested as threaded, as stated in the link above.


I should have seen that bit of docs, sorry.

That opens a bit of a Pandora's box.

Now I look again with a better idea of what that means, I see this isn't 
the only driver that has a mutex, and sets can_sleep to true, uses 
devm_request_threaded_irq, and called 
gpiochip_set_nested_irqchip/gpiochip_set_chained_irqchip.

Now I'm confused because I can't see how you can use them as nested 
interrupt controllers.

They will all cause "you cannot have chained interrupts on a chip that 
may sleep" from gpiochip_set_cascaded_irqchip the moment you try.

I'm still reading through what I do now, but any hints or tips would be 
appreciated.


> Best regards,
> Alexander

Regards,

Joe
Alexander Stein May 13, 2019, 2:59 p.m. UTC | #3 
Am Montag, 13. Mai 2019, 16:02:18 CEST schrieb Joe Burmeister:
> Hi Alexander,
> 
> You probably noticed there is a v3, but that just adds the missing signoff.
> 
> On 13/05/2019 13:59, Alexander Stein wrote:
> 
> > Am Montag, 13. Mai 2019, 14:00:24 CEST schrieb Joe Burmeister:
> >> Though it has a 'standby' it doesn't appear to be an issue and
> >> marking the chip with can_sleep means gpiolib.c won't allow its use
> >> as a interrupt controller.
> >> ---
> >>   drivers/pinctrl/pinctrl-mcp23s08.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/
pinctrl-mcp23s08.c
> >> index 3fc63cb5b332..7334d8eb9135 100644
> >> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> >> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> >> @@ -890,7 +890,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, 
struct device *dev,
> >>   		return PTR_ERR(mcp->regmap);
> >>   
> >>   	mcp->chip.base = base;
> >> -	mcp->chip.can_sleep = true;
> >> +	mcp->chip.can_sleep = false;
> >>   	mcp->chip.parent = dev;
> >>   	mcp->chip.owner = THIS_MODULE;
> >>   
> >>
> > IMHO this is completly wrong, please refer to the documentation of this 
flag, e.g. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
tree/include/linux/gpio/driver.h#n217
> > It essentially means you can't use this GPIOs from atomic context, as SPI 
or I2C transfers block/sleep, hence the name.
> > In your case the IRQs are probably not requested as threaded, as stated in 
the link above.
> 
> 
> I should have seen that bit of docs, sorry.
> 
> That opens a bit of a Pandora's box.
> 
> Now I look again with a better idea of what that means, I see this isn't 
> the only driver that has a mutex, and sets can_sleep to true, uses 
> devm_request_threaded_irq, and called 
> gpiochip_set_nested_irqchip/gpiochip_set_chained_irqchip.
> 
> Now I'm confused because I can't see how you can use them as nested 
> interrupt controllers.
> 
> They will all cause "you cannot have chained interrupts on a chip that 
> may sleep" from gpiochip_set_cascaded_irqchip the moment you try.
> 
> I'm still reading through what I do now, but any hints or tips would be 
> appreciated.

Have a look at https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/
linux.git/tree/Documentation/driver-api/gpio/driver.rst#n309 which pretty much 
describes the types.
I think you are confusing chained and nested IRQ controllers. Using 
gpiochip_set_nested_irqchip cannot result in that error.
mcp23s08 driver is using gpiochip_set_nested_irqchip appropriately.
I'm wondering what you are trying to resolve. Are you attaching another IRQ 
controller to MCP23Sxx inputs?

Best regards,
Alexander
Joe Burmeister May 13, 2019, 3:54 p.m. UTC | #4 
Hi Alexander,

On 13/05/2019 15:59, Alexander Stein wrote:
> Am Montag, 13. Mai 2019, 16:02:18 CEST schrieb Joe Burmeister:
>> Hi Alexander,
>>
>> You probably noticed there is a v3, but that just adds the missing signoff.
>>
>> On 13/05/2019 13:59, Alexander Stein wrote:
>>
>>> Am Montag, 13. Mai 2019, 14:00:24 CEST schrieb Joe Burmeister:
>>>> Though it has a 'standby' it doesn't appear to be an issue and
>>>> marking the chip with can_sleep means gpiolib.c won't allow its use
>>>> as a interrupt controller.
>>>> ---
>>>>    drivers/pinctrl/pinctrl-mcp23s08.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/
> pinctrl-mcp23s08.c
>>>> index 3fc63cb5b332..7334d8eb9135 100644
>>>> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
>>>> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
>>>> @@ -890,7 +890,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp,
> struct device *dev,
>>>>    		return PTR_ERR(mcp->regmap);
>>>>    
>>>>    	mcp->chip.base = base;
>>>> -	mcp->chip.can_sleep = true;
>>>> +	mcp->chip.can_sleep = false;
>>>>    	mcp->chip.parent = dev;
>>>>    	mcp->chip.owner = THIS_MODULE;
>>>>    
>>>>
>>> IMHO this is completly wrong, please refer to the documentation of this
> flag, e.g. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> tree/include/linux/gpio/driver.h#n217
>>> It essentially means you can't use this GPIOs from atomic context, as SPI
> or I2C transfers block/sleep, hence the name.
>>> In your case the IRQs are probably not requested as threaded, as stated in
> the link above.
>>
>> I should have seen that bit of docs, sorry.
>>
>> That opens a bit of a Pandora's box.
>>
>> Now I look again with a better idea of what that means, I see this isn't
>> the only driver that has a mutex, and sets can_sleep to true, uses
>> devm_request_threaded_irq, and called
>> gpiochip_set_nested_irqchip/gpiochip_set_chained_irqchip.
>>
>> Now I'm confused because I can't see how you can use them as nested
>> interrupt controllers.
>>
>> They will all cause "you cannot have chained interrupts on a chip that
>> may sleep" from gpiochip_set_cascaded_irqchip the moment you try.
>>
>> I'm still reading through what I do now, but any hints or tips would be
>> appreciated.
> Have a look at https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/
> linux.git/tree/Documentation/driver-api/gpio/driver.rst#n309 which pretty much
> describes the types.

Was just reading 
https://www.kernel.org/doc/Documentation/gpio/driver.txt as this came 
through.

> I think you are confusing chained and nested IRQ controllers. Using
> gpiochip_set_nested_irqchip cannot result in that error.
> mcp23s08 driver is using gpiochip_set_nested_irqchip appropriately.

It was seeing the error was what I thought brought me to can_sleep.

I assumed about can_sleep and assumed completely wrong.

I've just removed this second patch and it all seams to still be working 
fine.

Ignore this second patch, it's clearly nonsense. Sorry I wasted your 
time, if it's any consolation, I wasted much more of mine.

Should I resubmit the series with just the first patch?


> I'm wondering what you are trying to resolve. Are you attaching another IRQ
> controller to MCP23Sxx inputs?

We are using a MCP23S017 in a Raspberry Pi Compute Module motherboard, 4 
interrupt lines go into it and 2 come out.

(Though it turns out might as well be 1 as they both go into the same 
GPIO bank on the Pi, so same interrupt regardless.)

> Best regards,
> Alexander

Regards,


Joe
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 3fc63cb5b332..7334d8eb9135 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -890,7 +890,7 @@  static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 		return PTR_ERR(mcp->regmap);
 
 	mcp->chip.base = base;
-	mcp->chip.can_sleep = true;
+	mcp->chip.can_sleep = false;
 	mcp->chip.parent = dev;
 	mcp->chip.owner = THIS_MODULE;