[PATCH v3 1/2] usb: typec: tcpci: fix NULL pointer issue on shared irq case

[PATCH v3 1/2] usb: typec: tcpci: fix NULL pointer issue on shared irq case

[Xu Yang]

The tcpci_irq() may meet below NULL pointer dereference issue:

[    2.641851] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
[    2.641951] status 0x1, 0x37f
[    2.650659] Mem abort info:
[    2.656490]   ESR = 0x0000000096000004
[    2.660230]   EC = 0x25: DABT (current EL), IL = 32 bits
[    2.665532]   SET = 0, FnV = 0
[    2.668579]   EA = 0, S1PTW = 0
[    2.671715]   FSC = 0x04: level 0 translation fault
[    2.676584] Data abort info:
[    2.679459]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[    2.684936]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[    2.689980]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[    2.695284] [0000000000000010] user address but active_mm is swapper
[    2.701632] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[    2.707883] Modules linked in:
[    2.710936] CPU: 1 UID: 0 PID: 87 Comm: irq/111-2-0051 Not tainted 6.12.0-rc6-06316-g7f63786ad3d1-dirty #4
[    2.720570] Hardware name: NXP i.MX93 11X11 EVK board (DT)
[    2.726040] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    2.732989] pc : tcpci_irq+0x38/0x318
[    2.736647] lr : _tcpci_irq+0x14/0x20
[    2.740295] sp : ffff80008324bd30
[    2.743597] x29: ffff80008324bd70 x28: ffff800080107894 x27: ffff800082198f70
[    2.750721] x26: ffff0000050e6680 x25: ffff000004d172ac x24: ffff0000050f0000
[    2.757845] x23: ffff000004d17200 x22: 0000000000000001 x21: ffff0000050f0000
[    2.764969] x20: ffff000004d17200 x19: 0000000000000000 x18: 0000000000000001
[    2.772093] x17: 0000000000000000 x16: ffff80008183d8a0 x15: ffff00007fbab040
[    2.779217] x14: ffff00007fb918c0 x13: 0000000000000000 x12: 000000000000017a
[    2.786341] x11: 0000000000000001 x10: 0000000000000a90 x9 : ffff80008324bd00
[    2.793465] x8 : ffff0000050f0af0 x7 : ffff00007fbaa840 x6 : 0000000000000031
[    2.800589] x5 : 000000000000017a x4 : 0000000000000002 x3 : 0000000000000002
[    2.807713] x2 : ffff80008324bd3a x1 : 0000000000000010 x0 : 0000000000000000
[    2.814838] Call trace:
[    2.817273]  tcpci_irq+0x38/0x318
[    2.820583]  _tcpci_irq+0x14/0x20
[    2.823885]  irq_thread_fn+0x2c/0xa8
[    2.827456]  irq_thread+0x16c/0x2f4
[    2.830940]  kthread+0x110/0x114
[    2.834164]  ret_from_fork+0x10/0x20
[    2.837738] Code: f9426420 f9001fe0 d2800000 52800201 (f9400a60)

This may happen on shared irq case. Such as two Type-C ports share one
irq. After the first port finished tcpci_register_port(), it may trigger
interrupt. However, if the interrupt comes by chance the 2nd port finishes
devm_request_threaded_irq(), the 2nd port interrupt handler may be run at
first. Then the above issue may happen.

  devm_request_threaded_irq()
				<-- port1 irq comes
  disable_irq(client->irq);
  tcpci_register_port()

This will restore the logic to the state before commit (77e85107a771 "usb:
typec: tcpci: support edge irq").

Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
Cc: stable@vger.kernel.org
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v3:
 = no changes
Changes in v2:
 - no changes
---
 drivers/usb/typec/tcpm/tcpci.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 2f15734a5043..db42f4bf3632 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -923,22 +923,20 @@ static int tcpci_probe(struct i2c_client *client)
 
 	chip->data.set_orientation = err;
 
+	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
+	if (IS_ERR(chip->tcpci))
+		return PTR_ERR(chip->tcpci);
+
 	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
 					_tcpci_irq,
 					IRQF_SHARED | IRQF_ONESHOT,
 					dev_name(&client->dev), chip);
-	if (err < 0)
+	if (err < 0) {
+		tcpci_unregister_port(chip->tcpci);
 		return err;
+	}
 
-	/*
-	 * Disable irq while registering port. If irq is configured as an edge
-	 * irq this allow to keep track and process the irq as soon as it is enabled.
-	 */
-	disable_irq(client->irq);
-	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
-	enable_irq(client->irq);
-
-	return PTR_ERR_OR_ZERO(chip->tcpci);
+	return 0;
 }
 
 static void tcpci_remove(struct i2c_client *client)
-- 
2.34.1

[PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Xu Yang]

With edge irq support, the ALERT event may be missed currently. The reason
is that ALERT_MASK register is written before devm_request_threaded_irq().
If ALERT event happens in this time gap, it will be missed and ALERT line
will not recover to high level. However, we don't meet this issue with
level irq. To avoid the issue, this will set ALERT_MASK register after
devm_request_threaded_irq() return.

Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
Cc: stable@vger.kernel.org
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v3:
 - remove set_alert_mask flag
Changes in v2:
 - new patch
---
 drivers/usb/typec/tcpm/tcpci.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index db42f4bf3632..48762508cc86 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -700,7 +700,7 @@ static int tcpci_init(struct tcpc_dev *tcpc)
 
 	tcpci->alert_mask = reg;
 
-	return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
+	return 0;
 }
 
 irqreturn_t tcpci_irq(struct tcpci *tcpci)
@@ -931,12 +931,19 @@ static int tcpci_probe(struct i2c_client *client)
 					_tcpci_irq,
 					IRQF_SHARED | IRQF_ONESHOT,
 					dev_name(&client->dev), chip);
-	if (err < 0) {
-		tcpci_unregister_port(chip->tcpci);
-		return err;
-	}
+	if (err < 0)
+		goto unregister_port;
+
+	/* Enable chip interrupts at last */
+	err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, chip->tcpci->alert_mask);
+	if (err < 0)
+		goto unregister_port;
 
 	return 0;
+
+unregister_port:
+	tcpci_unregister_port(chip->tcpci);
+	return err;
 }
 
 static void tcpci_remove(struct i2c_client *client)
-- 
2.34.1

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Francesco Dolcini]

On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
> With edge irq support, the ALERT event may be missed currently. The reason
> is that ALERT_MASK register is written before devm_request_threaded_irq().
> If ALERT event happens in this time gap, it will be missed and ALERT line
> will not recover to high level. However, we don't meet this issue with
> level irq. To avoid the issue, this will set ALERT_MASK register after
> devm_request_threaded_irq() return.
> 
> Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v3:
>  - remove set_alert_mask flag
> Changes in v2:
>  - new patch
> ---
>  drivers/usb/typec/tcpm/tcpci.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index db42f4bf3632..48762508cc86 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -700,7 +700,7 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>  
>  	tcpci->alert_mask = reg;
>  
> -	return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> +	return 0;

Should we set the alert mask to 0 at the beginning of tcpci_init() ?

Just wondering if some bind/unbind or module reload use case would need
it.

Francesco

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Xu Yang]

On Tue, Dec 17, 2024 at 10:29:05AM +0100, Francesco Dolcini wrote:
> On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
> > With edge irq support, the ALERT event may be missed currently. The reason
> > is that ALERT_MASK register is written before devm_request_threaded_irq().
> > If ALERT event happens in this time gap, it will be missed and ALERT line
> > will not recover to high level. However, we don't meet this issue with
> > level irq. To avoid the issue, this will set ALERT_MASK register after
> > devm_request_threaded_irq() return.
> > 
> > Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > 
> > ---
> > Changes in v3:
> >  - remove set_alert_mask flag
> > Changes in v2:
> >  - new patch
> > ---
> >  drivers/usb/typec/tcpm/tcpci.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > index db42f4bf3632..48762508cc86 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.c
> > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > @@ -700,7 +700,7 @@ static int tcpci_init(struct tcpc_dev *tcpc)
> >  
> >  	tcpci->alert_mask = reg;
> >  
> > -	return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> > +	return 0;
> 
> Should we set the alert mask to 0 at the beginning of tcpci_init() ?
> 
> Just wondering if some bind/unbind or module reload use case would need
> it.

Maybe not needed.

tcpci = devm_kzalloc(dev, sizeof(*tcpci), GFP_KERNEL);

tcpci will be reset to all 0 when allocate the memory. So alert_mask is 0
by default.

Thanks,
Xu Yang

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Francesco Dolcini]

On Tue, Dec 17, 2024 at 05:41:17PM +0800, Xu Yang wrote:
> On Tue, Dec 17, 2024 at 10:29:05AM +0100, Francesco Dolcini wrote:
> > On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
> > > With edge irq support, the ALERT event may be missed currently. The reason
> > > is that ALERT_MASK register is written before devm_request_threaded_irq().
> > > If ALERT event happens in this time gap, it will be missed and ALERT line
> > > will not recover to high level. However, we don't meet this issue with
> > > level irq. To avoid the issue, this will set ALERT_MASK register after
> > > devm_request_threaded_irq() return.
> > > 
> > > Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > 
> > > ---
> > > Changes in v3:
> > >  - remove set_alert_mask flag
> > > Changes in v2:
> > >  - new patch
> > > ---
> > >  drivers/usb/typec/tcpm/tcpci.c | 17 ++++++++++++-----
> > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > > index db42f4bf3632..48762508cc86 100644
> > > --- a/drivers/usb/typec/tcpm/tcpci.c
> > > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > > @@ -700,7 +700,7 @@ static int tcpci_init(struct tcpc_dev *tcpc)
> > >  
> > >  	tcpci->alert_mask = reg;
> > >  
> > > -	return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> > > +	return 0;
> > 
> > Should we set the alert mask to 0 at the beginning of tcpci_init() ?
> > 
> > Just wondering if some bind/unbind or module reload use case would need
> > it.
> 
> Maybe not needed.
> 
> tcpci = devm_kzalloc(dev, sizeof(*tcpci), GFP_KERNEL);
> 
> tcpci will be reset to all 0 when allocate the memory. So alert_mask is 0
> by default.

I meant

  tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);

in tcpci_init().

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Xu Yang]

On Tue, Dec 17, 2024 at 10:45:53AM +0100, Francesco Dolcini wrote:
> On Tue, Dec 17, 2024 at 05:41:17PM +0800, Xu Yang wrote:
> > On Tue, Dec 17, 2024 at 10:29:05AM +0100, Francesco Dolcini wrote:
> > > On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
> > > > With edge irq support, the ALERT event may be missed currently. The reason
> > > > is that ALERT_MASK register is written before devm_request_threaded_irq().
> > > > If ALERT event happens in this time gap, it will be missed and ALERT line
> > > > will not recover to high level. However, we don't meet this issue with
> > > > level irq. To avoid the issue, this will set ALERT_MASK register after
> > > > devm_request_threaded_irq() return.
> > > > 
> > > > Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > 
> > > > ---
> > > > Changes in v3:
> > > >  - remove set_alert_mask flag
> > > > Changes in v2:
> > > >  - new patch
> > > > ---
> > > >  drivers/usb/typec/tcpm/tcpci.c | 17 ++++++++++++-----
> > > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > > > index db42f4bf3632..48762508cc86 100644
> > > > --- a/drivers/usb/typec/tcpm/tcpci.c
> > > > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > > > @@ -700,7 +700,7 @@ static int tcpci_init(struct tcpc_dev *tcpc)
> > > >  
> > > >  	tcpci->alert_mask = reg;
> > > >  
> > > > -	return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> > > > +	return 0;
> > > 
> > > Should we set the alert mask to 0 at the beginning of tcpci_init() ?
> > > 
> > > Just wondering if some bind/unbind or module reload use case would need
> > > it.
> > 
> > Maybe not needed.
> > 
> > tcpci = devm_kzalloc(dev, sizeof(*tcpci), GFP_KERNEL);
> > 
> > tcpci will be reset to all 0 when allocate the memory. So alert_mask is 0
> > by default.
> 
> I meant
> 
>   tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
> 
> in tcpci_init().
> 

Still seems not needed.

  err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val, sizeof(u16));

ALERT_MASK will be set to 0 at the start of tcpci_probe()

Thanks,
Xu Yang

Re: [PATCH v3 1/2] usb: typec: tcpci: fix NULL pointer issue on shared irq case

[Dan Carpenter]

On Tue, Dec 17, 2024 at 05:12:07PM +0800, Xu Yang wrote:
> The tcpci_irq() may meet below NULL pointer dereference issue:
> 
> [    2.641851] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> [    2.641951] status 0x1, 0x37f
> [    2.650659] Mem abort info:
> [    2.656490]   ESR = 0x0000000096000004
> [    2.660230]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    2.665532]   SET = 0, FnV = 0
> [    2.668579]   EA = 0, S1PTW = 0
> [    2.671715]   FSC = 0x04: level 0 translation fault
> [    2.676584] Data abort info:
> [    2.679459]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> [    2.684936]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [    2.689980]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [    2.695284] [0000000000000010] user address but active_mm is swapper
> [    2.701632] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [    2.707883] Modules linked in:
> [    2.710936] CPU: 1 UID: 0 PID: 87 Comm: irq/111-2-0051 Not tainted 6.12.0-rc6-06316-g7f63786ad3d1-dirty #4
> [    2.720570] Hardware name: NXP i.MX93 11X11 EVK board (DT)
> [    2.726040] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    2.732989] pc : tcpci_irq+0x38/0x318
> [    2.736647] lr : _tcpci_irq+0x14/0x20
> [    2.740295] sp : ffff80008324bd30
> [    2.743597] x29: ffff80008324bd70 x28: ffff800080107894 x27: ffff800082198f70
> [    2.750721] x26: ffff0000050e6680 x25: ffff000004d172ac x24: ffff0000050f0000
> [    2.757845] x23: ffff000004d17200 x22: 0000000000000001 x21: ffff0000050f0000
> [    2.764969] x20: ffff000004d17200 x19: 0000000000000000 x18: 0000000000000001
> [    2.772093] x17: 0000000000000000 x16: ffff80008183d8a0 x15: ffff00007fbab040
> [    2.779217] x14: ffff00007fb918c0 x13: 0000000000000000 x12: 000000000000017a
> [    2.786341] x11: 0000000000000001 x10: 0000000000000a90 x9 : ffff80008324bd00
> [    2.793465] x8 : ffff0000050f0af0 x7 : ffff00007fbaa840 x6 : 0000000000000031
> [    2.800589] x5 : 000000000000017a x4 : 0000000000000002 x3 : 0000000000000002
> [    2.807713] x2 : ffff80008324bd3a x1 : 0000000000000010 x0 : 0000000000000000
> [    2.814838] Call trace:
> [    2.817273]  tcpci_irq+0x38/0x318
> [    2.820583]  _tcpci_irq+0x14/0x20
> [    2.823885]  irq_thread_fn+0x2c/0xa8
> [    2.827456]  irq_thread+0x16c/0x2f4
> [    2.830940]  kthread+0x110/0x114
> [    2.834164]  ret_from_fork+0x10/0x20
> [    2.837738] Code: f9426420 f9001fe0 d2800000 52800201 (f9400a60)
> 
> This may happen on shared irq case. Such as two Type-C ports share one
> irq. After the first port finished tcpci_register_port(), it may trigger
> interrupt. However, if the interrupt comes by chance the 2nd port finishes
> devm_request_threaded_irq(), the 2nd port interrupt handler may be run at
> first. Then the above issue may happen.
> 
>   devm_request_threaded_irq()
> 				<-- port1 irq comes
>   disable_irq(client->irq);
>   tcpci_register_port()
> 
> This will restore the logic to the state before commit (77e85107a771 "usb:
> typec: tcpci: support edge irq").
> 
> Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 

This commit message was not super clear to me.  It didn't mention that
it's tcpci->regmap which is the NULL pointer.  (Obviously there are a
lot of other NULL pointers, but that's the one which will crash).

Here is something you could add to the commit message:

   We cannot register the IRQ handler until after
   tcpci_register_port() is called.  Otherwise tcpci->regmap and
   so many other tcpci pointers are not set up and it leads to
   a NULL dereference in tcpci_irq().

regards,
dan carpenter

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Dan Carpenter]

On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
> With edge irq support, the ALERT event may be missed currently. The reason
> is that ALERT_MASK register is written before devm_request_threaded_irq().
> If ALERT event happens in this time gap, it will be missed and ALERT line
> will not recover to high level. However, we don't meet this issue with
> level irq. To avoid the issue, this will set ALERT_MASK register after
> devm_request_threaded_irq() return.
> 
> Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")

I agree with Francesco that this was introduced by patch 1 so the
patches need to be folded together into one patch.  The commit
message would be something like:

    The ordering of setting up the IRQs is not correct here.
    We need to call tcpci_register_port() devm_request_threaded_irq().
    Otherwise if we recieve an IRQ before tcpci_register_port() has
    completed it leads to a NULL dereference in tcpci_irq() because
    tcpci->regmap and other pointers are NULL.

    However, moving tcpci_register_port() earlier creates a problem
    of its own because there is a potential that tcpci_init() will be
    called before devm_request_threaded_irq().  The tcpci_init()
    writes the ALERT_MASK to the hardware to tell it to start
    generating interrupts but we're not ready to deal with them yet.

    Move the ALERT_MASK stuff until after the call to
    devm_request_threaded_irq() has finished.

Something like that.

regards,
dan carpenter

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Francesco Dolcini]

On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
> With edge irq support, the ALERT event may be missed currently. The reason
> is that ALERT_MASK register is written before devm_request_threaded_irq().
> If ALERT event happens in this time gap, it will be missed and ALERT line
> will not recover to high level. However, we don't meet this issue with
> level irq. To avoid the issue, this will set ALERT_MASK register after
> devm_request_threaded_irq() return.
> 
> Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

I had an offline chat with a Emanuele (in Cc:) that worked on this a few
weeks ago and he remember that he already tried a similar approach, but
for some reason he did not work.

He should be able to try this patch in a few days, but with the upcoming
winter holidays he might not be super responsive.

I wonder if we could wait a little before merging this to allow this
testing to happen. Or maybe you can just test if this is working on your
setup using edge interrupts (you would need to use only one TCPCI, for
the test).

Francesco

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Emanuele Ghidoli]

On 17/12/2024 17:35, Francesco Dolcini wrote:
> On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
>> With edge irq support, the ALERT event may be missed currently. The reason
>> is that ALERT_MASK register is written before devm_request_threaded_irq().
>> If ALERT event happens in this time gap, it will be missed and ALERT line
>> will not recover to high level. However, we don't meet this issue with
>> level irq. To avoid the issue, this will set ALERT_MASK register after
>> devm_request_threaded_irq() return.
>>
>> Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> I had an offline chat with a Emanuele (in Cc:) that worked on this a few
> weeks ago and he remember that he already tried a similar approach, but
> for some reason he did not work.
> 
> He should be able to try this patch in a few days, but with the upcoming
> winter holidays he might not be super responsive.
> 
> I wonder if we could wait a little before merging this to allow this
> testing to happen. Or maybe you can just test if this is working on your
> setup using edge interrupts (you would need to use only one TCPCI, for
> the test).
> 
> Francesco
> 

Hi all,

I was curious, so I tested the two patches. I can confirm that if both are applied, 
edge interrupts still work correctly.
However, with only the first patch applied, it does not work.

I hope this helps.

Emanuele

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Xu Yang]

On Tue, Dec 17, 2024 at 09:29:36PM +0100, Emanuele Ghidoli wrote:
> 
> 
> On 17/12/2024 17:35, Francesco Dolcini wrote:
> > On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
> >> With edge irq support, the ALERT event may be missed currently. The reason
> >> is that ALERT_MASK register is written before devm_request_threaded_irq().
> >> If ALERT event happens in this time gap, it will be missed and ALERT line
> >> will not recover to high level. However, we don't meet this issue with
> >> level irq. To avoid the issue, this will set ALERT_MASK register after
> >> devm_request_threaded_irq() return.
> >>
> >> Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > 
> > I had an offline chat with a Emanuele (in Cc:) that worked on this a few
> > weeks ago and he remember that he already tried a similar approach, but
> > for some reason he did not work.
> > 
> > He should be able to try this patch in a few days, but with the upcoming
> > winter holidays he might not be super responsive.
> > 
> > I wonder if we could wait a little before merging this to allow this
> > testing to happen. Or maybe you can just test if this is working on your
> > setup using edge interrupts (you would need to use only one TCPCI, for
> > the test).
> > 
> > Francesco
> > 
> 
> Hi all,
> 
> I was curious, so I tested the two patches. I can confirm that if both are applied, 
> edge interrupts still work correctly.
> However, with only the first patch applied, it does not work.

Yes. This is an expected results. So could this be regarded as a tested-by?
I have tested edge irq before sending out the patches too.

Thanks,
Xu Yang

Re: [PATCH v3 1/2] usb: typec: tcpci: fix NULL pointer issue on shared irq case

[Xu Yang]

On Tue, Dec 17, 2024 at 12:56:03PM +0300, Dan Carpenter wrote:
> On Tue, Dec 17, 2024 at 05:12:07PM +0800, Xu Yang wrote:
> > The tcpci_irq() may meet below NULL pointer dereference issue:
> > 
> > [    2.641851] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> > [    2.641951] status 0x1, 0x37f
> > [    2.650659] Mem abort info:
> > [    2.656490]   ESR = 0x0000000096000004
> > [    2.660230]   EC = 0x25: DABT (current EL), IL = 32 bits
> > [    2.665532]   SET = 0, FnV = 0
> > [    2.668579]   EA = 0, S1PTW = 0
> > [    2.671715]   FSC = 0x04: level 0 translation fault
> > [    2.676584] Data abort info:
> > [    2.679459]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> > [    2.684936]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> > [    2.689980]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> > [    2.695284] [0000000000000010] user address but active_mm is swapper
> > [    2.701632] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> > [    2.707883] Modules linked in:
> > [    2.710936] CPU: 1 UID: 0 PID: 87 Comm: irq/111-2-0051 Not tainted 6.12.0-rc6-06316-g7f63786ad3d1-dirty #4
> > [    2.720570] Hardware name: NXP i.MX93 11X11 EVK board (DT)
> > [    2.726040] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [    2.732989] pc : tcpci_irq+0x38/0x318
> > [    2.736647] lr : _tcpci_irq+0x14/0x20
> > [    2.740295] sp : ffff80008324bd30
> > [    2.743597] x29: ffff80008324bd70 x28: ffff800080107894 x27: ffff800082198f70
> > [    2.750721] x26: ffff0000050e6680 x25: ffff000004d172ac x24: ffff0000050f0000
> > [    2.757845] x23: ffff000004d17200 x22: 0000000000000001 x21: ffff0000050f0000
> > [    2.764969] x20: ffff000004d17200 x19: 0000000000000000 x18: 0000000000000001
> > [    2.772093] x17: 0000000000000000 x16: ffff80008183d8a0 x15: ffff00007fbab040
> > [    2.779217] x14: ffff00007fb918c0 x13: 0000000000000000 x12: 000000000000017a
> > [    2.786341] x11: 0000000000000001 x10: 0000000000000a90 x9 : ffff80008324bd00
> > [    2.793465] x8 : ffff0000050f0af0 x7 : ffff00007fbaa840 x6 : 0000000000000031
> > [    2.800589] x5 : 000000000000017a x4 : 0000000000000002 x3 : 0000000000000002
> > [    2.807713] x2 : ffff80008324bd3a x1 : 0000000000000010 x0 : 0000000000000000
> > [    2.814838] Call trace:
> > [    2.817273]  tcpci_irq+0x38/0x318
> > [    2.820583]  _tcpci_irq+0x14/0x20
> > [    2.823885]  irq_thread_fn+0x2c/0xa8
> > [    2.827456]  irq_thread+0x16c/0x2f4
> > [    2.830940]  kthread+0x110/0x114
> > [    2.834164]  ret_from_fork+0x10/0x20
> > [    2.837738] Code: f9426420 f9001fe0 d2800000 52800201 (f9400a60)
> > 
> > This may happen on shared irq case. Such as two Type-C ports share one
> > irq. After the first port finished tcpci_register_port(), it may trigger
> > interrupt. However, if the interrupt comes by chance the 2nd port finishes
> > devm_request_threaded_irq(), the 2nd port interrupt handler may be run at
> > first. Then the above issue may happen.
> > 
> >   devm_request_threaded_irq()
> > 				<-- port1 irq comes
> >   disable_irq(client->irq);
> >   tcpci_register_port()
> > 
> > This will restore the logic to the state before commit (77e85107a771 "usb:
> > typec: tcpci: support edge irq").
> > 
> > Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > 
> 
> This commit message was not super clear to me.  It didn't mention that
> it's tcpci->regmap which is the NULL pointer.  (Obviously there are a
> lot of other NULL pointers, but that's the one which will crash).

Ahh, actually tcpci itself is the NULL pointer and kernel will crash when
get regmap. Because if tcpci is a valid pointer, all of its members will
be valid too. Anyway, I will add such info to commit message.

> 
> Here is something you could add to the commit message:
> 
>    We cannot register the IRQ handler until after
>    tcpci_register_port() is called.  Otherwise tcpci->regmap and
>    so many other tcpci pointers are not set up and it leads to
>    a NULL dereference in tcpci_irq().

Thanks,
Xu Yang

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Xu Yang]

On Tue, Dec 17, 2024 at 01:45:08PM +0300, Dan Carpenter wrote:
> On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
> > With edge irq support, the ALERT event may be missed currently. The reason
> > is that ALERT_MASK register is written before devm_request_threaded_irq().
> > If ALERT event happens in this time gap, it will be missed and ALERT line
> > will not recover to high level. However, we don't meet this issue with
> > level irq. To avoid the issue, this will set ALERT_MASK register after
> > devm_request_threaded_irq() return.
> > 
> > Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> 
> I agree with Francesco that this was introduced by patch 1 so the
> patches need to be folded together into one patch.  The commit
> message would be something like:
> 
>     The ordering of setting up the IRQs is not correct here.
>     We need to call tcpci_register_port() devm_request_threaded_irq().
>     Otherwise if we recieve an IRQ before tcpci_register_port() has
>     completed it leads to a NULL dereference in tcpci_irq() because
>     tcpci->regmap and other pointers are NULL.
> 
>     However, moving tcpci_register_port() earlier creates a problem
>     of its own because there is a potential that tcpci_init() will be
>     called before devm_request_threaded_irq().  The tcpci_init()
>     writes the ALERT_MASK to the hardware to tell it to start
>     generating interrupts but we're not ready to deal with them yet.
> 
>     Move the ALERT_MASK stuff until after the call to
>     devm_request_threaded_irq() has finished.
> 
> Something like that.
> 

Okay.

Thanks,
Xu Yang

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Emanuele Ghidoli]

On 18/12/2024 06:31, Xu Yang wrote:
> On Tue, Dec 17, 2024 at 09:29:36PM +0100, Emanuele Ghidoli wrote:
>>
>>
>> On 17/12/2024 17:35, Francesco Dolcini wrote:
>>> On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
>>>> With edge irq support, the ALERT event may be missed currently. The reason
>>>> is that ALERT_MASK register is written before devm_request_threaded_irq().
>>>> If ALERT event happens in this time gap, it will be missed and ALERT line
>>>> will not recover to high level. However, we don't meet this issue with
>>>> level irq. To avoid the issue, this will set ALERT_MASK register after
>>>> devm_request_threaded_irq() return.
>>>>
>>>> Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>>>
>>> I had an offline chat with a Emanuele (in Cc:) that worked on this a few
>>> weeks ago and he remember that he already tried a similar approach, but
>>> for some reason he did not work.
>>>
>>> He should be able to try this patch in a few days, but with the upcoming
>>> winter holidays he might not be super responsive.
>>>
>>> I wonder if we could wait a little before merging this to allow this
>>> testing to happen. Or maybe you can just test if this is working on your
>>> setup using edge interrupts (you would need to use only one TCPCI, for
>>> the test).
>>>
>>> Francesco
>>>
>>
>> Hi all,
>>
>> I was curious, so I tested the two patches. I can confirm that if both are applied, 
>> edge interrupts still work correctly.
>> However, with only the first patch applied, it does not work.
> 
> Yes. This is an expected results. So could this be regarded as a tested-by?
> I have tested edge irq before sending out the patches too.
> 
> Thanks,
> Xu Yang

Hello Xu,
I confirmed that the first patch introduces a regression, 
so I agree with Francesco and Dan about merging the two patches.

Anyway, I tested it.

Tested-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

Regards,
Emanuele

Re: [PATCH v3 2/2] usb: typec: tcpci: set ALERT_MASK register after devm_request_threaded_irq()

[Xu Yang]

On Wed, Dec 18, 2024 at 07:41:49AM +0100, Emanuele Ghidoli wrote:
> 
> 
> On 18/12/2024 06:31, Xu Yang wrote:
> > On Tue, Dec 17, 2024 at 09:29:36PM +0100, Emanuele Ghidoli wrote:
> >>
> >>
> >> On 17/12/2024 17:35, Francesco Dolcini wrote:
> >>> On Tue, Dec 17, 2024 at 05:12:08PM +0800, Xu Yang wrote:
> >>>> With edge irq support, the ALERT event may be missed currently. The reason
> >>>> is that ALERT_MASK register is written before devm_request_threaded_irq().
> >>>> If ALERT event happens in this time gap, it will be missed and ALERT line
> >>>> will not recover to high level. However, we don't meet this issue with
> >>>> level irq. To avoid the issue, this will set ALERT_MASK register after
> >>>> devm_request_threaded_irq() return.
> >>>>
> >>>> Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> >>>> Cc: stable@vger.kernel.org
> >>>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >>>
> >>> I had an offline chat with a Emanuele (in Cc:) that worked on this a few
> >>> weeks ago and he remember that he already tried a similar approach, but
> >>> for some reason he did not work.
> >>>
> >>> He should be able to try this patch in a few days, but with the upcoming
> >>> winter holidays he might not be super responsive.
> >>>
> >>> I wonder if we could wait a little before merging this to allow this
> >>> testing to happen. Or maybe you can just test if this is working on your
> >>> setup using edge interrupts (you would need to use only one TCPCI, for
> >>> the test).
> >>>
> >>> Francesco
> >>>
> >>
> >> Hi all,
> >>
> >> I was curious, so I tested the two patches. I can confirm that if both are applied, 
> >> edge interrupts still work correctly.
> >> However, with only the first patch applied, it does not work.
> > 
> > Yes. This is an expected results. So could this be regarded as a tested-by?
> > I have tested edge irq before sending out the patches too.
> > 
> > Thanks,
> > Xu Yang
> 
> Hello Xu,
> I confirmed that the first patch introduces a regression, 
> so I agree with Francesco and Dan about merging the two patches.

Okay.

> 
> Anyway, I tested it.
> 
> Tested-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

Good to know.

Thanks,
Xu Yang

Re: [PATCH v3 1/2] usb: typec: tcpci: fix NULL pointer issue on shared irq case

[Dan Carpenter]

On Wed, Dec 18, 2024 at 01:45:47PM +0800, Xu Yang wrote:
> > > Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > 
> > 
> > This commit message was not super clear to me.  It didn't mention that
> > it's tcpci->regmap which is the NULL pointer.  (Obviously there are a
> > lot of other NULL pointers, but that's the one which will crash).
> 
> Ahh, actually tcpci itself is the NULL pointer and kernel will crash when
> get regmap. Because if tcpci is a valid pointer, all of its members will
> be valid too. Anyway, I will add such info to commit message.

Thanks!

regards,
dan carpenter