[PATCH] extcon: usbc-tusb320: make sure the state is initialized on probe

[PATCH] extcon: usbc-tusb320: make sure the state is initialized on probe

[Peter Rosin]

When the port is connected at boot, there is not necessarily
an interrupt flagged in the interrupt status register, causing
the IRQ handler to bail out early without reading the state when
it is invoked directly from probe.

Add a flag that overrides the interrupt status register and reads
the state regardless during probe.

Fixes: 06bc4ca115cd ("extcon: Add driver for TI TUSB320")
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/extcon/extcon-usbc-tusb320.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c
index 6ba3d89b106d..bd3645ae0d52 100644
--- a/drivers/extcon/extcon-usbc-tusb320.c
+++ b/drivers/extcon/extcon-usbc-tusb320.c
@@ -55,6 +55,7 @@ struct tusb320_priv {
 	struct extcon_dev *edev;
 	struct tusb320_ops *ops;
 	enum tusb320_attached_state state;
+	bool initialized;
 };
 
 static const char * const tusb_attached_states[] = {
@@ -195,7 +196,7 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
 		return IRQ_NONE;
 	}
 
-	if (!(reg & TUSB320_REG9_INTERRUPT_STATUS))
+	if (priv->initialized && !(reg & TUSB320_REG9_INTERRUPT_STATUS))
 		return IRQ_NONE;
 
 	state = (reg >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
@@ -297,6 +298,8 @@ static int tusb320_extcon_probe(struct i2c_client *client,
 		 */
 		tusb320_irq_handler(client->irq, priv);
 
+	priv->initialized = true;
+
 	ret = devm_request_threaded_irq(priv->dev, client->irq, NULL,
 					tusb320_irq_handler,
 					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-- 
2.20.1

Re: [PATCH] extcon: usbc-tusb320: make sure the state is initialized on probe

[Chanwoo Choi]

On 22. 6. 23. 20:21, Peter Rosin wrote:
> When the port is connected at boot, there is not necessarily
> an interrupt flagged in the interrupt status register, causing
> the IRQ handler to bail out early without reading the state when
> it is invoked directly from probe.
> 
> Add a flag that overrides the interrupt status register and reads
> the state regardless during probe.
> 
> Fixes: 06bc4ca115cd ("extcon: Add driver for TI TUSB320")
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/extcon/extcon-usbc-tusb320.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c
> index 6ba3d89b106d..bd3645ae0d52 100644
> --- a/drivers/extcon/extcon-usbc-tusb320.c
> +++ b/drivers/extcon/extcon-usbc-tusb320.c
> @@ -55,6 +55,7 @@ struct tusb320_priv {
>  	struct extcon_dev *edev;
>  	struct tusb320_ops *ops;
>  	enum tusb320_attached_state state;
> +	bool initialized;
>  };
>  
>  static const char * const tusb_attached_states[] = {
> @@ -195,7 +196,7 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
>  		return IRQ_NONE;
>  	}
>  
> -	if (!(reg & TUSB320_REG9_INTERRUPT_STATUS))
> +	if (priv->initialized && !(reg & TUSB320_REG9_INTERRUPT_STATUS))
>  		return IRQ_NONE;
>  
>  	state = (reg >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
> @@ -297,6 +298,8 @@ static int tusb320_extcon_probe(struct i2c_client *client,
>  		 */
>  		tusb320_irq_handler(client->irq, priv);
>  
> +	priv->initialized = true;
> +

After initializing as 'priv->initialized = true', 
tusb320_irq_handler() is not anymore detecting the external connector changes.

If external connector is detached after finished kernel boot,
how to change the state of external connector by using extcon_set_state()?

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH] extcon: usbc-tusb320: make sure the state is initialized on probe

[Peter Rosin]

Hi!

2022-06-29 at 21:54, Chanwoo Choi wrote:
> On 22. 6. 23. 20:21, Peter Rosin wrote:
>> When the port is connected at boot, there is not necessarily
>> an interrupt flagged in the interrupt status register, causing
>> the IRQ handler to bail out early without reading the state when
>> it is invoked directly from probe.
>>
>> Add a flag that overrides the interrupt status register and reads
>> the state regardless during probe.
>>
>> Fixes: 06bc4ca115cd ("extcon: Add driver for TI TUSB320")
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/extcon/extcon-usbc-tusb320.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c
>> index 6ba3d89b106d..bd3645ae0d52 100644
>> --- a/drivers/extcon/extcon-usbc-tusb320.c
>> +++ b/drivers/extcon/extcon-usbc-tusb320.c
>> @@ -55,6 +55,7 @@ struct tusb320_priv {
>>  	struct extcon_dev *edev;
>>  	struct tusb320_ops *ops;
>>  	enum tusb320_attached_state state;
>> +	bool initialized;
>>  };
>>  
>>  static const char * const tusb_attached_states[] = {
>> @@ -195,7 +196,7 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
>>  		return IRQ_NONE;
>>  	}
>>  
>> -	if (!(reg & TUSB320_REG9_INTERRUPT_STATUS))
>> +	if (priv->initialized && !(reg & TUSB320_REG9_INTERRUPT_STATUS))
>>  		return IRQ_NONE;

Do not return early if priv->initialized is false. Behave as before if
priv->initialized is true.

>>  
>>  	state = (reg >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
>> @@ -297,6 +298,8 @@ static int tusb320_extcon_probe(struct i2c_client *client,
>>  		 */
>>  		tusb320_irq_handler(client->irq, priv);
>>  
>> +	priv->initialized = true;
>> +
> 
> After initializing as 'priv->initialized = true', 
> tusb320_irq_handler() is not anymore detecting the external connector changes.

Have you tested the patch and observed the trouble you are reporting, or
have you simply misread the patch?

> 
> If external connector is detached after finished kernel boot,
> how to change the state of external connector by using extcon_set_state()?
> 

If you did test this and there actually is a problem, maybe there should be
READ_ONCE in the irq handler when checking and WRITE_ONCE when assigning
priv->initialized. But if that's really what's going on I'd be surprised
when it's a variable that changes *once* before the interrupt has even
been requested.

Cheers,
Peter

Re: [PATCH] extcon: usbc-tusb320: make sure the state is initialized on probe

[Peter Rosin]

Hi!

2022-06-30 at 08:03, Peter Rosin wrote:
> Hi!
> 
> 2022-06-29 at 21:54, Chanwoo Choi wrote:
>> On 22. 6. 23. 20:21, Peter Rosin wrote:
>>> When the port is connected at boot, there is not necessarily
>>> an interrupt flagged in the interrupt status register, causing
>>> the IRQ handler to bail out early without reading the state when
>>> it is invoked directly from probe.
>>>
>>> Add a flag that overrides the interrupt status register and reads
>>> the state regardless during probe.
>>>
>>> Fixes: 06bc4ca115cd ("extcon: Add driver for TI TUSB320")
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>>  drivers/extcon/extcon-usbc-tusb320.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c
>>> index 6ba3d89b106d..bd3645ae0d52 100644
>>> --- a/drivers/extcon/extcon-usbc-tusb320.c
>>> +++ b/drivers/extcon/extcon-usbc-tusb320.c
>>> @@ -55,6 +55,7 @@ struct tusb320_priv {
>>>  	struct extcon_dev *edev;
>>>  	struct tusb320_ops *ops;
>>>  	enum tusb320_attached_state state;
>>> +	bool initialized;
>>>  };
>>>  
>>>  static const char * const tusb_attached_states[] = {
>>> @@ -195,7 +196,7 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
>>>  		return IRQ_NONE;
>>>  	}
>>>  
>>> -	if (!(reg & TUSB320_REG9_INTERRUPT_STATUS))
>>> +	if (priv->initialized && !(reg & TUSB320_REG9_INTERRUPT_STATUS))
>>>  		return IRQ_NONE;
> 
> Do not return early if priv->initialized is false. Behave as before if
> priv->initialized is true.
> 
>>>  
>>>  	state = (reg >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
>>> @@ -297,6 +298,8 @@ static int tusb320_extcon_probe(struct i2c_client *client,
>>>  		 */
>>>  		tusb320_irq_handler(client->irq, priv);
>>>  
>>> +	priv->initialized = true;
>>> +
>>
>> After initializing as 'priv->initialized = true', 
>> tusb320_irq_handler() is not anymore detecting the external connector changes.
> 
> Have you tested the patch and observed the trouble you are reporting, or
> have you simply misread the patch?
> 
>>
>> If external connector is detached after finished kernel boot,
>> how to change the state of external connector by using extcon_set_state()?
>>
> 
> If you did test this and there actually is a problem, maybe there should be
> READ_ONCE in the irq handler when checking and WRITE_ONCE when assigning
> priv->initialized. But if that's really what's going on I'd be surprised
> when it's a variable that changes *once* before the interrupt has even
> been requested.
> 
> Cheers,
> Peter

Ping?

Cheers,
Peter