[PATCH 0/4] Add Polling support for role detection with HD3SS3220

[PATCH 0/4] Add Polling support for role detection with HD3SS3220

[Biju Das]

This patch series aims to add polling support for role detection
with HD3SS3220.

Whilst it also fix a null kernel crash and also sort the headers.

Biju Das (4):
  dt-bindings: usb: ti,hd3ss3220: Update interrupt property as optional
  usb: typec: hd3ss3220: Fix NULL pointer crash
  usb: typec: hd3ss3220: Sort header files
  usb: typec: hd3ss3220: Add polling support

 .../devicetree/bindings/usb/ti,hd3ss3220.yaml |  1 -
 drivers/usb/typec/hd3ss3220.c                 | 42 ++++++++++++++++---
 2 files changed, 37 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/4] dt-bindings: usb: ti,hd3ss3220: Update interrupt property as optional

[Biju Das]

On some platforms(rg: RZ/V2M EVK), interrupt is not populated. Update
the binding to make interrupt property as optional.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml b/Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml
index b86bf6bc9cd6..a1cffb70c621 100644
--- a/Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml
@@ -46,7 +46,6 @@ properties:
 required:
   - compatible
   - reg
-  - interrupts
 
 additionalProperties: false
 
-- 
2.25.1

[PATCH 2/4] usb: typec: hd3ss3220: Fix NULL pointer crash

[Biju Das]

The value returned by usb_role_switch_get() can be NULL and it leads
to NULL pointer crash. This patch fixes this issue by adding NULL
check for the role switch handle.

[   25.336613] Hardware name: Silicon Linux RZ/G2E evaluation kit EK874 (CAT874 + CAT875) (DT)
[   25.344991] Workqueue: events_unbound deferred_probe_work_func
[   25.350869] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   25.357854] pc : renesas_usb3_role_switch_get+0x40/0x80 [renesas_usb3]
[   25.364428] lr : renesas_usb3_role_switch_get+0x24/0x80 [renesas_usb3]
[   25.370986] sp : ffff80000a4b3a40
[   25.374311] x29: ffff80000a4b3a40 x28: 0000000000000000 x27: 0000000000000000
[   25.381476] x26: ffff80000a3ade78 x25: ffff00000a809005 x24: ffff80000117f178
[   25.388641] x23: ffff00000a8d7810 x22: ffff00000a8d8410 x21: 0000000000000000
[   25.395805] x20: ffff000011cd7080 x19: ffff000011cd7080 x18: 0000000000000020
[   25.402969] x17: ffff800076196000 x16: ffff800008004000 x15: 0000000000004000
[   25.410133] x14: 000000000000022b x13: 0000000000000001 x12: 0000000000000001
[   25.417291] x11: 0000000000000000 x10: 0000000000000a40 x9 : ffff80000a4b3770
[   25.424452] x8 : ffff00007fbc9000 x7 : 0040000000000008 x6 : ffff00000a8d8590
[   25.431615] x5 : ffff80000a4b3960 x4 : 0000000000000000 x3 : ffff00000a8d84f4
[   25.438776] x2 : 0000000000000218 x1 : ffff80000a715218 x0 : 0000000000000218
[   25.445942] Call trace:
[   25.448398]  renesas_usb3_role_switch_get+0x40/0x80 [renesas_usb3]
[   25.454613]  renesas_usb3_role_switch_set+0x4c/0x440 [renesas_usb3]
[   25.460908]  usb_role_switch_set_role+0x44/0xa4
[   25.465468]  hd3ss3220_set_role+0xa0/0x100 [hd3ss3220]
[   25.470635]  hd3ss3220_probe+0x118/0x2fc [hd3ss3220]
[   25.475621]  i2c_device_probe+0x338/0x384

Fixes: 5a9a8a4c5058 ("usb: typec: hd3ss3220: hd3ss3220_probe() warn: passing zero to 'PTR_ERR'")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
This issue triggered on RZ/G2E board, where there is no USB3 firmware and it
returned a null role switch handle.
---
 drivers/usb/typec/hd3ss3220.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 2a58185fb14c..c24bbccd14f9 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -186,7 +186,10 @@ static int hd3ss3220_probe(struct i2c_client *client,
 		hd3ss3220->role_sw = usb_role_switch_get(hd3ss3220->dev);
 	}
 
-	if (IS_ERR(hd3ss3220->role_sw)) {
+	if (!hd3ss3220->role_sw) {
+		ret = -ENODEV;
+		goto err_put_fwnode;
+	} else if (IS_ERR(hd3ss3220->role_sw)) {
 		ret = PTR_ERR(hd3ss3220->role_sw);
 		goto err_put_fwnode;
 	}
-- 
2.25.1

[PATCH 3/4] usb: typec: hd3ss3220: Sort header files

[Biju Das]

This patch sorts the header files alphabetically.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/usb/typec/hd3ss3220.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index c24bbccd14f9..666a93f7ec65 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -5,15 +5,15 @@
  * Copyright (C) 2019 Renesas Electronics Corp.
  */
 
-#include <linux/module.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
-#include <linux/usb/role.h>
-#include <linux/irqreturn.h>
 #include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/module.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/usb/role.h>
 #include <linux/usb/typec.h>
-#include <linux/delay.h>
 
 #define HD3SS3220_REG_CN_STAT_CTRL	0x09
 #define HD3SS3220_REG_GEN_CTRL		0x0A
-- 
2.25.1

[PATCH 4/4] usb: typec: hd3ss3220: Add polling support

[Biju Das]

Some platforms(for eg: RZ/V2M EVK) does not have interrupt pin
connected to HD3SS3220. Add polling support for role detection.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/usb/typec/hd3ss3220.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 666a93f7ec65..9894fa96cc5f 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -14,6 +14,7 @@
 #include <linux/slab.h>
 #include <linux/usb/role.h>
 #include <linux/usb/typec.h>
+#include <linux/workqueue.h>
 
 #define HD3SS3220_REG_CN_STAT_CTRL	0x09
 #define HD3SS3220_REG_GEN_CTRL		0x0A
@@ -37,6 +38,9 @@ struct hd3ss3220 {
 	struct regmap *regmap;
 	struct usb_role_switch	*role_sw;
 	struct typec_port *port;
+	struct delayed_work output_poll_work;
+	enum usb_role role_state;
+	bool poll;
 };
 
 static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
@@ -118,6 +122,22 @@ static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
 	default:
 		break;
 	}
+
+	hd3ss3220->role_state = role_state;
+}
+
+static void output_poll_execute(struct work_struct *work)
+{
+	struct delayed_work *delayed_work = to_delayed_work(work);
+	struct hd3ss3220 *hd3ss3220 = container_of(delayed_work,
+						   struct hd3ss3220,
+						   output_poll_work);
+	enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
+
+	if (hd3ss3220->role_state != role_state)
+		hd3ss3220_set_role(hd3ss3220);
+
+	schedule_delayed_work(&hd3ss3220->output_poll_work, HZ);
 }
 
 static irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220)
@@ -227,6 +247,9 @@ static int hd3ss3220_probe(struct i2c_client *client,
 					"hd3ss3220", &client->dev);
 		if (ret)
 			goto err_unreg_port;
+	} else {
+		INIT_DELAYED_WORK(&hd3ss3220->output_poll_work, output_poll_execute);
+		hd3ss3220->poll = true;
 	}
 
 	ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
@@ -235,6 +258,9 @@ static int hd3ss3220_probe(struct i2c_client *client,
 
 	fwnode_handle_put(connector);
 
+	if (hd3ss3220->poll)
+		schedule_delayed_work(&hd3ss3220->output_poll_work, HZ);
+
 	dev_info(&client->dev, "probed revision=0x%x\n", ret);
 
 	return 0;
@@ -252,6 +278,9 @@ static void hd3ss3220_remove(struct i2c_client *client)
 {
 	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
 
+	if (hd3ss3220->poll)
+		cancel_delayed_work_sync(&hd3ss3220->output_poll_work);
+
 	typec_unregister_port(hd3ss3220->port);
 	usb_role_switch_put(hd3ss3220->role_sw);
 }
-- 
2.25.1

Re: [PATCH 3/4] usb: typec: hd3ss3220: Sort header files

[Greg Kroah-Hartman]

On Fri, Dec 09, 2022 at 03:56:22PM +0000, Biju Das wrote:
> This patch sorts the header files alphabetically.

This says what you are doing, but not why.

Why is this needed?  What does this fix?  This is no requirement of USB
code that I know of.

thanks,

greg k-h

Re: [PATCH 2/4] usb: typec: hd3ss3220: Fix NULL pointer crash

[Greg Kroah-Hartman]

On Fri, Dec 09, 2022 at 03:56:21PM +0000, Biju Das wrote:
> The value returned by usb_role_switch_get() can be NULL and it leads
> to NULL pointer crash. This patch fixes this issue by adding NULL
> check for the role switch handle.

Why isn't this the first patch here, and cc: stable, or just as an
individual patch that has nothing to do with the other new feature
patches?

thanks,

greg k-h

RE: [PATCH 3/4] usb: typec: hd3ss3220: Sort header files

[Biju Das]

Hi Greg,

Thanks for the feedback.

> Subject: Re: [PATCH 3/4] usb: typec: hd3ss3220: Sort header files
> 
> On Fri, Dec 09, 2022 at 03:56:22PM +0000, Biju Das wrote:
> > This patch sorts the header files alphabetically.
> 
> This says what you are doing, but not why.
> 
> Why is this needed?  What does this fix?  This is no requirement of USB
> code that I know of.

Previously I got some feedback from other subsystem maintainers, to sort the
headers alphabetically. Based on that, I thought of doing same here.

I would like to drop this patch, as there is no requirement of USB code to
sort the headers.

Cheers,
Biju

RE: [PATCH 2/4] usb: typec: hd3ss3220: Fix NULL pointer crash

[Biju Das]

Hi Greg,

Thanks for the feedback.

> Subject: Re: [PATCH 2/4] usb: typec: hd3ss3220: Fix NULL pointer crash
> 
> On Fri, Dec 09, 2022 at 03:56:21PM +0000, Biju Das wrote:
> > The value returned by usb_role_switch_get() can be NULL and it leads
> > to NULL pointer crash. This patch fixes this issue by adding NULL
> > check for the role switch handle.
> 
> Why isn't this the first patch here, and cc: stable, or just as an
> individual patch that has nothing to do with the other new feature
> patches?

OK, I will send this as standalone patch, cc to stable on next version.

Cheers,
Biju

Re: [PATCH 2/4] usb: typec: hd3ss3220: Fix NULL pointer crash

[Sergei Shtylyov]

Hello!

On 12/9/22 6:56 PM, Biju Das wrote:

> The value returned by usb_role_switch_get() can be NULL and it leads
> to NULL pointer crash. This patch fixes this issue by adding NULL
> check for the role switch handle.
> 
> [   25.336613] Hardware name: Silicon Linux RZ/G2E evaluation kit EK874 (CAT874 + CAT875) (DT)
> [   25.344991] Workqueue: events_unbound deferred_probe_work_func
> [   25.350869] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [   25.357854] pc : renesas_usb3_role_switch_get+0x40/0x80 [renesas_usb3]
> [   25.364428] lr : renesas_usb3_role_switch_get+0x24/0x80 [renesas_usb3]
> [   25.370986] sp : ffff80000a4b3a40
> [   25.374311] x29: ffff80000a4b3a40 x28: 0000000000000000 x27: 0000000000000000
> [   25.381476] x26: ffff80000a3ade78 x25: ffff00000a809005 x24: ffff80000117f178
> [   25.388641] x23: ffff00000a8d7810 x22: ffff00000a8d8410 x21: 0000000000000000
> [   25.395805] x20: ffff000011cd7080 x19: ffff000011cd7080 x18: 0000000000000020
> [   25.402969] x17: ffff800076196000 x16: ffff800008004000 x15: 0000000000004000
> [   25.410133] x14: 000000000000022b x13: 0000000000000001 x12: 0000000000000001
> [   25.417291] x11: 0000000000000000 x10: 0000000000000a40 x9 : ffff80000a4b3770
> [   25.424452] x8 : ffff00007fbc9000 x7 : 0040000000000008 x6 : ffff00000a8d8590
> [   25.431615] x5 : ffff80000a4b3960 x4 : 0000000000000000 x3 : ffff00000a8d84f4
> [   25.438776] x2 : 0000000000000218 x1 : ffff80000a715218 x0 : 0000000000000218
> [   25.445942] Call trace:
> [   25.448398]  renesas_usb3_role_switch_get+0x40/0x80 [renesas_usb3]
> [   25.454613]  renesas_usb3_role_switch_set+0x4c/0x440 [renesas_usb3]
> [   25.460908]  usb_role_switch_set_role+0x44/0xa4
> [   25.465468]  hd3ss3220_set_role+0xa0/0x100 [hd3ss3220]
> [   25.470635]  hd3ss3220_probe+0x118/0x2fc [hd3ss3220]
> [   25.475621]  i2c_device_probe+0x338/0x384
> 
> Fixes: 5a9a8a4c5058 ("usb: typec: hd3ss3220: hd3ss3220_probe() warn: passing zero to 'PTR_ERR'")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> This issue triggered on RZ/G2E board, where there is no USB3 firmware and it
> returned a null role switch handle.
> ---
>  drivers/usb/typec/hd3ss3220.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index 2a58185fb14c..c24bbccd14f9 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -186,7 +186,10 @@ static int hd3ss3220_probe(struct i2c_client *client,
>  		hd3ss3220->role_sw = usb_role_switch_get(hd3ss3220->dev);
>  	}
>  
> -	if (IS_ERR(hd3ss3220->role_sw)) {
> +	if (!hd3ss3220->role_sw) {
> +		ret = -ENODEV;
> +		goto err_put_fwnode;
> +	} else if (IS_ERR(hd3ss3220->role_sw)) {

   No need for *else* after *goto*.

[...]

MBR, Sergey

Re: [PATCH 3/4] usb: typec: hd3ss3220: Sort header files

[Geert Uytterhoeven]

Hi Greg,

On Fri, Dec 9, 2022 at 5:16 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Dec 09, 2022 at 03:56:22PM +0000, Biju Das wrote:
> > This patch sorts the header files alphabetically.
>
> This says what you are doing, but not why.
>
> Why is this needed?  What does this fix?  This is no requirement of USB
> code that I know of.

... to avoid merge conflicts when adding/removing includes in future
patches.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] usb: typec: hd3ss3220: Sort header files

[Greg Kroah-Hartman]

On Thu, Dec 15, 2022 at 11:05:13AM +0100, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Fri, Dec 9, 2022 at 5:16 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Dec 09, 2022 at 03:56:22PM +0000, Biju Das wrote:
> > > This patch sorts the header files alphabetically.
> >
> > This says what you are doing, but not why.
> >
> > Why is this needed?  What does this fix?  This is no requirement of USB
> > code that I know of.
> 
> ... to avoid merge conflicts when adding/removing includes in future
> patches.

Then let's deal with that then, again, this isn't a real issue, sorry.

greg k-h