* [PATCH v5 0/3] usb: typec: ucsi: allow retry to find role switch
@ 2022-04-22 3:10 Linyu Yuan
2022-04-22 3:10 ` [PATCH v5 1/3] usb: typec: ucsi: add a common function ucsi_unregister_connectors() Linyu Yuan
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Linyu Yuan @ 2022-04-22 3:10 UTC (permalink / raw)
To: Heikki Krogerus, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham, Linyu Yuan
when one role switch module built as module, it may load late than
ucsi, this change series allow retry from ucsi.
v2: improve ucsi_connector_clean() to cover all condition,
and add one new change to avoid allocate one unused connector.
v3: fix comment from ucsi maintainer.
v4: fix review comment from Heikki Krogerus,
merge patch#1 and patch#2 in V3 into one patch,
add counter for retry limit,
and some other minor fix.
v5: only update commit description of two patches
Linyu Yuan (3):
usb: typec: ucsi: add a common function ucsi_unregister_connectors()
usb: typec: ucsi: do not allocate one extra unused connector
usb: typec: ucsi: Wait for the USB role switches
drivers/usb/typec/ucsi/ucsi.c | 86 +++++++++++++++++++++++++------------------
drivers/usb/typec/ucsi/ucsi.h | 6 ++-
2 files changed, 55 insertions(+), 37 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 1/3] usb: typec: ucsi: add a common function ucsi_unregister_connectors()
2022-04-22 3:10 [PATCH v5 0/3] usb: typec: ucsi: allow retry to find role switch Linyu Yuan
@ 2022-04-22 3:10 ` Linyu Yuan
2022-04-25 8:09 ` Heikki Krogerus
2022-04-22 3:10 ` [PATCH v5 2/3] usb: typec: ucsi: do not allocate one extra unused connector Linyu Yuan
2022-04-22 3:10 ` [PATCH v5 3/3] usb: typec: ucsi: Wait for the USB role switches Linyu Yuan
2 siblings, 1 reply; 9+ messages in thread
From: Linyu Yuan @ 2022-04-22 3:10 UTC (permalink / raw)
To: Heikki Krogerus, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham, Linyu Yuan
As con->port will be used in error path of ucsi_init(),
it should be NULL or valid.
In error path of ucsi_init(), it will unregister all valid ucsi connectors,
and similar operation also happen in ucsi_unregister(),
add a common function for two places.
Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: improve ucsi_connector_clean(), check total number of connector.
v3: rename to ucsi_unregister_connectors(), suggest by maintainer
v4: merge patch#1 in V3, fix a typo samiliar -> similar in commit description
v5: no change
drivers/usb/typec/ucsi/ucsi.c | 52 ++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index f0c2fa1..af9a2a1 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1100,6 +1100,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
con->port = typec_register_port(ucsi->dev, cap);
if (IS_ERR(con->port)) {
ret = PTR_ERR(con->port);
+ con->port = NULL;
goto out;
}
@@ -1186,6 +1187,32 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
return ret;
}
+static void ucsi_unregister_connectors(struct ucsi *ucsi)
+{
+ struct ucsi_connector *con;
+ int i;
+
+ if (!ucsi->connector)
+ return;
+
+ for (i = 0; i < ucsi->cap.num_connectors; i++) {
+ con = &ucsi->connector[i];
+ if (!con->port)
+ break;
+
+ cancel_work_sync(&con->work);
+ ucsi_unregister_partner(con);
+ ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
+ ucsi_unregister_port_psy(con);
+ if (con->wq)
+ destroy_workqueue(con->wq);
+ typec_unregister_port(con->port);
+ }
+
+ kfree(ucsi->connector);
+ ucsi->connector = NULL;
+}
+
/**
* ucsi_init - Initialize UCSI interface
* @ucsi: UCSI to be initialized
@@ -1194,7 +1221,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
*/
static int ucsi_init(struct ucsi *ucsi)
{
- struct ucsi_connector *con;
u64 command;
int ret;
int i;
@@ -1249,15 +1275,7 @@ static int ucsi_init(struct ucsi *ucsi)
return 0;
err_unregister:
- for (con = ucsi->connector; con->port; con++) {
- ucsi_unregister_partner(con);
- ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
- ucsi_unregister_port_psy(con);
- if (con->wq)
- destroy_workqueue(con->wq);
- typec_unregister_port(con->port);
- con->port = NULL;
- }
+ ucsi_unregister_connectors(ucsi);
err_reset:
memset(&ucsi->cap, 0, sizeof(ucsi->cap));
@@ -1363,7 +1381,6 @@ EXPORT_SYMBOL_GPL(ucsi_register);
void ucsi_unregister(struct ucsi *ucsi)
{
u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
- int i;
/* Make sure that we are not in the middle of driver initialization */
cancel_work_sync(&ucsi->work);
@@ -1371,18 +1388,7 @@ void ucsi_unregister(struct ucsi *ucsi)
/* Disable notifications */
ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
- for (i = 0; i < ucsi->cap.num_connectors; i++) {
- cancel_work_sync(&ucsi->connector[i].work);
- ucsi_unregister_partner(&ucsi->connector[i]);
- ucsi_unregister_altmodes(&ucsi->connector[i],
- UCSI_RECIPIENT_CON);
- ucsi_unregister_port_psy(&ucsi->connector[i]);
- if (ucsi->connector[i].wq)
- destroy_workqueue(ucsi->connector[i].wq);
- typec_unregister_port(ucsi->connector[i].port);
- }
-
- kfree(ucsi->connector);
+ ucsi_unregister_connectors(ucsi);
}
EXPORT_SYMBOL_GPL(ucsi_unregister);
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v5 2/3] usb: typec: ucsi: do not allocate one extra unused connector
2022-04-22 3:10 [PATCH v5 0/3] usb: typec: ucsi: allow retry to find role switch Linyu Yuan
2022-04-22 3:10 ` [PATCH v5 1/3] usb: typec: ucsi: add a common function ucsi_unregister_connectors() Linyu Yuan
@ 2022-04-22 3:10 ` Linyu Yuan
2022-04-25 8:13 ` Heikki Krogerus
2022-04-22 3:10 ` [PATCH v5 3/3] usb: typec: ucsi: Wait for the USB role switches Linyu Yuan
2 siblings, 1 reply; 9+ messages in thread
From: Linyu Yuan @ 2022-04-22 3:10 UTC (permalink / raw)
To: Heikki Krogerus, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham, Linyu Yuan
In ucsi_init(), it allocate number of (ucsi->cap.num_connectors + 1)
connectors, there is one extra as the ending.
ucsi_unregister_connectors() is safe to unregister all ucsi connectors
according ucsi->cap.num_connectors.
Let's remove the extra one connector to save memory.
Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: new change
v3: no change
v4: fix a typo extral -> extra in commit description
v5: update commit description
drivers/usb/typec/ucsi/ucsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index af9a2a1..ce9192e 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1251,7 +1251,7 @@ static int ucsi_init(struct ucsi *ucsi)
}
/* Allocate the connectors. Released in ucsi_unregister() */
- ucsi->connector = kcalloc(ucsi->cap.num_connectors + 1,
+ ucsi->connector = kcalloc(ucsi->cap.num_connectors,
sizeof(*ucsi->connector), GFP_KERNEL);
if (!ucsi->connector) {
ret = -ENOMEM;
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v5 3/3] usb: typec: ucsi: Wait for the USB role switches
2022-04-22 3:10 [PATCH v5 0/3] usb: typec: ucsi: allow retry to find role switch Linyu Yuan
2022-04-22 3:10 ` [PATCH v5 1/3] usb: typec: ucsi: add a common function ucsi_unregister_connectors() Linyu Yuan
2022-04-22 3:10 ` [PATCH v5 2/3] usb: typec: ucsi: do not allocate one extra unused connector Linyu Yuan
@ 2022-04-22 3:10 ` Linyu Yuan
2022-04-25 8:14 ` Heikki Krogerus
2 siblings, 1 reply; 9+ messages in thread
From: Linyu Yuan @ 2022-04-22 3:10 UTC (permalink / raw)
To: Heikki Krogerus, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham, Linyu Yuan
When role switch module probe late than ucsi module,
fwnode_usb_role_switch_get() will return -EPROBE_DEFER,
it is better to restart ucsi init work to find
it again every 100ms, total wait time is 10 second.
It also means change ucsi init work to delayed_work.
Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: keep original con->num in debug log
v3: change return value from -EAGAIN to PTR_ERR()
v4: change subject line,
add counter for retry limit,
correct commit descripton to match change in V3
v5: small update of commit description
drivers/usb/typec/ucsi/ucsi.c | 32 ++++++++++++++++++++------------
drivers/usb/typec/ucsi/ucsi.h | 6 +++++-
2 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ce9192e..11f8808 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1053,6 +1053,14 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
con->num = index + 1;
con->ucsi = ucsi;
+ cap->fwnode = ucsi_find_fwnode(con);
+ con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
+ if (IS_ERR(con->usb_role_sw)) {
+ dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
+ con->num);
+ return PTR_ERR(con->usb_role_sw);
+ }
+
/* Delay other interactions with the con until registration is complete */
mutex_lock(&con->lock);
@@ -1088,7 +1096,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
*accessory = TYPEC_ACCESSORY_DEBUG;
- cap->fwnode = ucsi_find_fwnode(con);
cap->driver_data = con;
cap->ops = &ucsi_ops;
@@ -1147,13 +1154,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
ucsi_port_psy_changed(con);
}
- con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
- if (IS_ERR(con->usb_role_sw)) {
- dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
- con->num);
- con->usb_role_sw = NULL;
- }
-
/* Only notify USB controller if partner supports USB data */
if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB))
u_role = USB_ROLE_NONE;
@@ -1286,12 +1286,20 @@ static int ucsi_init(struct ucsi *ucsi)
static void ucsi_init_work(struct work_struct *work)
{
- struct ucsi *ucsi = container_of(work, struct ucsi, work);
+ struct ucsi *ucsi = container_of(work, struct ucsi, work.work);
int ret;
ret = ucsi_init(ucsi);
if (ret)
dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
+
+ if (ret == -EPROBE_DEFER) {
+ if (ucsi->work_count++ > UCSI_ROLE_SWITCH_WAIT_COUNT)
+ return;
+
+ queue_delayed_work(system_long_wq, &ucsi->work,
+ UCSI_ROLE_SWITCH_INTERVAL);
+ }
}
/**
@@ -1331,7 +1339,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
if (!ucsi)
return ERR_PTR(-ENOMEM);
- INIT_WORK(&ucsi->work, ucsi_init_work);
+ INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
mutex_init(&ucsi->ppm_lock);
ucsi->dev = dev;
ucsi->ops = ops;
@@ -1366,7 +1374,7 @@ int ucsi_register(struct ucsi *ucsi)
if (!ucsi->version)
return -ENODEV;
- queue_work(system_long_wq, &ucsi->work);
+ queue_delayed_work(system_long_wq, &ucsi->work, 0);
return 0;
}
@@ -1383,7 +1391,7 @@ void ucsi_unregister(struct ucsi *ucsi)
u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
/* Make sure that we are not in the middle of driver initialization */
- cancel_work_sync(&ucsi->work);
+ cancel_delayed_work_sync(&ucsi->work);
/* Disable notifications */
ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 280f1e1..8eb391e 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -287,7 +287,11 @@ struct ucsi {
struct ucsi_capability cap;
struct ucsi_connector *connector;
- struct work_struct work;
+ struct delayed_work work;
+ int work_count;
+#define UCSI_ROLE_SWITCH_RETRY_PER_HZ 10
+#define UCSI_ROLE_SWITCH_INTERVAL (HZ / UCSI_ROLE_SWITCH_RETRY_PER_HZ)
+#define UCSI_ROLE_SWITCH_WAIT_COUNT (10 * UCSI_ROLE_SWITCH_RETRY_PER_HZ)
/* PPM Communication lock */
struct mutex ppm_lock;
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/3] usb: typec: ucsi: add a common function ucsi_unregister_connectors()
2022-04-22 3:10 ` [PATCH v5 1/3] usb: typec: ucsi: add a common function ucsi_unregister_connectors() Linyu Yuan
@ 2022-04-25 8:09 ` Heikki Krogerus
2022-04-25 8:23 ` Linyu Yuan (QUIC)
0 siblings, 1 reply; 9+ messages in thread
From: Heikki Krogerus @ 2022-04-25 8:09 UTC (permalink / raw)
To: Linyu Yuan; +Cc: Greg Kroah-Hartman, linux-usb, Jack Pham
On Fri, Apr 22, 2022 at 11:10:20AM +0800, Linyu Yuan wrote:
> As con->port will be used in error path of ucsi_init(),
> it should be NULL or valid.
>
> In error path of ucsi_init(), it will unregister all valid ucsi connectors,
> and similar operation also happen in ucsi_unregister(),
> add a common function for two places.
>
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: improve ucsi_connector_clean(), check total number of connector.
> v3: rename to ucsi_unregister_connectors(), suggest by maintainer
> v4: merge patch#1 in V3, fix a typo samiliar -> similar in commit description
> v5: no change
>
> drivers/usb/typec/ucsi/ucsi.c | 52 ++++++++++++++++++++++++-------------------
> 1 file changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index f0c2fa1..af9a2a1 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1100,6 +1100,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> con->port = typec_register_port(ucsi->dev, cap);
> if (IS_ERR(con->port)) {
> ret = PTR_ERR(con->port);
> + con->port = NULL;
I'm not sure you need to add that line. See below.
> goto out;
> }
>
> @@ -1186,6 +1187,32 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> return ret;
> }
>
> +static void ucsi_unregister_connectors(struct ucsi *ucsi)
> +{
> + struct ucsi_connector *con;
> + int i;
> +
> + if (!ucsi->connector)
> + return;
> +
> + for (i = 0; i < ucsi->cap.num_connectors; i++) {
> + con = &ucsi->connector[i];
> + if (!con->port)
> + break;
if (IS_ERR_OR_NULL(con->port))
break;
> + cancel_work_sync(&con->work);
> + ucsi_unregister_partner(con);
> + ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
> + ucsi_unregister_port_psy(con);
> + if (con->wq)
> + destroy_workqueue(con->wq);
> + typec_unregister_port(con->port);
> + }
> +
> + kfree(ucsi->connector);
> + ucsi->connector = NULL;
> +}
> +
> /**
> * ucsi_init - Initialize UCSI interface
> * @ucsi: UCSI to be initialized
> @@ -1194,7 +1221,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> */
> static int ucsi_init(struct ucsi *ucsi)
> {
> - struct ucsi_connector *con;
> u64 command;
> int ret;
> int i;
> @@ -1249,15 +1275,7 @@ static int ucsi_init(struct ucsi *ucsi)
> return 0;
>
> err_unregister:
> - for (con = ucsi->connector; con->port; con++) {
> - ucsi_unregister_partner(con);
> - ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
> - ucsi_unregister_port_psy(con);
> - if (con->wq)
> - destroy_workqueue(con->wq);
> - typec_unregister_port(con->port);
> - con->port = NULL;
> - }
> + ucsi_unregister_connectors(ucsi);
>
> err_reset:
> memset(&ucsi->cap, 0, sizeof(ucsi->cap));
> @@ -1363,7 +1381,6 @@ EXPORT_SYMBOL_GPL(ucsi_register);
> void ucsi_unregister(struct ucsi *ucsi)
> {
> u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
> - int i;
>
> /* Make sure that we are not in the middle of driver initialization */
> cancel_work_sync(&ucsi->work);
> @@ -1371,18 +1388,7 @@ void ucsi_unregister(struct ucsi *ucsi)
> /* Disable notifications */
> ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
>
> - for (i = 0; i < ucsi->cap.num_connectors; i++) {
> - cancel_work_sync(&ucsi->connector[i].work);
> - ucsi_unregister_partner(&ucsi->connector[i]);
> - ucsi_unregister_altmodes(&ucsi->connector[i],
> - UCSI_RECIPIENT_CON);
> - ucsi_unregister_port_psy(&ucsi->connector[i]);
> - if (ucsi->connector[i].wq)
> - destroy_workqueue(ucsi->connector[i].wq);
> - typec_unregister_port(ucsi->connector[i].port);
> - }
> -
> - kfree(ucsi->connector);
> + ucsi_unregister_connectors(ucsi);
> }
> EXPORT_SYMBOL_GPL(ucsi_unregister);
thanks,
--
heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/3] usb: typec: ucsi: do not allocate one extra unused connector
2022-04-22 3:10 ` [PATCH v5 2/3] usb: typec: ucsi: do not allocate one extra unused connector Linyu Yuan
@ 2022-04-25 8:13 ` Heikki Krogerus
2022-04-25 8:23 ` Linyu Yuan (QUIC)
0 siblings, 1 reply; 9+ messages in thread
From: Heikki Krogerus @ 2022-04-25 8:13 UTC (permalink / raw)
To: Linyu Yuan; +Cc: Greg Kroah-Hartman, linux-usb, Jack Pham
On Fri, Apr 22, 2022 at 11:10:21AM +0800, Linyu Yuan wrote:
> In ucsi_init(), it allocate number of (ucsi->cap.num_connectors + 1)
> connectors, there is one extra as the ending.
> ucsi_unregister_connectors() is safe to unregister all ucsi connectors
> according ucsi->cap.num_connectors.
>
> Let's remove the extra one connector to save memory.
Maybe you could just merge this one into the first patch.
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: new change
> v3: no change
> v4: fix a typo extral -> extra in commit description
> v5: update commit description
>
> drivers/usb/typec/ucsi/ucsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index af9a2a1..ce9192e 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1251,7 +1251,7 @@ static int ucsi_init(struct ucsi *ucsi)
> }
>
> /* Allocate the connectors. Released in ucsi_unregister() */
> - ucsi->connector = kcalloc(ucsi->cap.num_connectors + 1,
> + ucsi->connector = kcalloc(ucsi->cap.num_connectors,
> sizeof(*ucsi->connector), GFP_KERNEL);
> if (!ucsi->connector) {
> ret = -ENOMEM;
> --
> 2.7.4
thanks,
--
heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 3/3] usb: typec: ucsi: Wait for the USB role switches
2022-04-22 3:10 ` [PATCH v5 3/3] usb: typec: ucsi: Wait for the USB role switches Linyu Yuan
@ 2022-04-25 8:14 ` Heikki Krogerus
0 siblings, 0 replies; 9+ messages in thread
From: Heikki Krogerus @ 2022-04-25 8:14 UTC (permalink / raw)
To: Linyu Yuan; +Cc: Greg Kroah-Hartman, linux-usb, Jack Pham
On Fri, Apr 22, 2022 at 11:10:22AM +0800, Linyu Yuan wrote:
> When role switch module probe late than ucsi module,
> fwnode_usb_role_switch_get() will return -EPROBE_DEFER,
> it is better to restart ucsi init work to find
> it again every 100ms, total wait time is 10 second.
>
> It also means change ucsi init work to delayed_work.
>
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> v2: keep original con->num in debug log
> v3: change return value from -EAGAIN to PTR_ERR()
> v4: change subject line,
> add counter for retry limit,
> correct commit descripton to match change in V3
> v5: small update of commit description
>
> drivers/usb/typec/ucsi/ucsi.c | 32 ++++++++++++++++++++------------
> drivers/usb/typec/ucsi/ucsi.h | 6 +++++-
> 2 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index ce9192e..11f8808 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1053,6 +1053,14 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> con->num = index + 1;
> con->ucsi = ucsi;
>
> + cap->fwnode = ucsi_find_fwnode(con);
> + con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> + if (IS_ERR(con->usb_role_sw)) {
> + dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> + con->num);
> + return PTR_ERR(con->usb_role_sw);
> + }
> +
> /* Delay other interactions with the con until registration is complete */
> mutex_lock(&con->lock);
>
> @@ -1088,7 +1096,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
> *accessory = TYPEC_ACCESSORY_DEBUG;
>
> - cap->fwnode = ucsi_find_fwnode(con);
> cap->driver_data = con;
> cap->ops = &ucsi_ops;
>
> @@ -1147,13 +1154,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> ucsi_port_psy_changed(con);
> }
>
> - con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> - if (IS_ERR(con->usb_role_sw)) {
> - dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> - con->num);
> - con->usb_role_sw = NULL;
> - }
> -
> /* Only notify USB controller if partner supports USB data */
> if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB))
> u_role = USB_ROLE_NONE;
> @@ -1286,12 +1286,20 @@ static int ucsi_init(struct ucsi *ucsi)
>
> static void ucsi_init_work(struct work_struct *work)
> {
> - struct ucsi *ucsi = container_of(work, struct ucsi, work);
> + struct ucsi *ucsi = container_of(work, struct ucsi, work.work);
> int ret;
>
> ret = ucsi_init(ucsi);
> if (ret)
> dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
> +
> + if (ret == -EPROBE_DEFER) {
> + if (ucsi->work_count++ > UCSI_ROLE_SWITCH_WAIT_COUNT)
> + return;
> +
> + queue_delayed_work(system_long_wq, &ucsi->work,
> + UCSI_ROLE_SWITCH_INTERVAL);
> + }
> }
>
> /**
> @@ -1331,7 +1339,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
> if (!ucsi)
> return ERR_PTR(-ENOMEM);
>
> - INIT_WORK(&ucsi->work, ucsi_init_work);
> + INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
> mutex_init(&ucsi->ppm_lock);
> ucsi->dev = dev;
> ucsi->ops = ops;
> @@ -1366,7 +1374,7 @@ int ucsi_register(struct ucsi *ucsi)
> if (!ucsi->version)
> return -ENODEV;
>
> - queue_work(system_long_wq, &ucsi->work);
> + queue_delayed_work(system_long_wq, &ucsi->work, 0);
>
> return 0;
> }
> @@ -1383,7 +1391,7 @@ void ucsi_unregister(struct ucsi *ucsi)
> u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
>
> /* Make sure that we are not in the middle of driver initialization */
> - cancel_work_sync(&ucsi->work);
> + cancel_delayed_work_sync(&ucsi->work);
>
> /* Disable notifications */
> ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 280f1e1..8eb391e 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -287,7 +287,11 @@ struct ucsi {
> struct ucsi_capability cap;
> struct ucsi_connector *connector;
>
> - struct work_struct work;
> + struct delayed_work work;
> + int work_count;
> +#define UCSI_ROLE_SWITCH_RETRY_PER_HZ 10
> +#define UCSI_ROLE_SWITCH_INTERVAL (HZ / UCSI_ROLE_SWITCH_RETRY_PER_HZ)
> +#define UCSI_ROLE_SWITCH_WAIT_COUNT (10 * UCSI_ROLE_SWITCH_RETRY_PER_HZ)
>
> /* PPM Communication lock */
> struct mutex ppm_lock;
> --
> 2.7.4
--
heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v5 1/3] usb: typec: ucsi: add a common function ucsi_unregister_connectors()
2022-04-25 8:09 ` Heikki Krogerus
@ 2022-04-25 8:23 ` Linyu Yuan (QUIC)
0 siblings, 0 replies; 9+ messages in thread
From: Linyu Yuan (QUIC) @ 2022-04-25 8:23 UTC (permalink / raw)
To: Heikki Krogerus, Linyu Yuan (QUIC)
Cc: Greg Kroah-Hartman, linux-usb, Jack Pham (QUIC)
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Monday, April 25, 2022 4:10 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> Subject: Re: [PATCH v5 1/3] usb: typec: ucsi: add a common function
> ucsi_unregister_connectors()
>
> On Fri, Apr 22, 2022 at 11:10:20AM +0800, Linyu Yuan wrote:
> > As con->port will be used in error path of ucsi_init(),
> > it should be NULL or valid.
> >
> > In error path of ucsi_init(), it will unregister all valid ucsi connectors,
> > and similar operation also happen in ucsi_unregister(),
> > add a common function for two places.
> >
> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > ---
> > v2: improve ucsi_connector_clean(), check total number of connector.
> > v3: rename to ucsi_unregister_connectors(), suggest by maintainer
> > v4: merge patch#1 in V3, fix a typo samiliar -> similar in commit description
> > v5: no change
> >
> > drivers/usb/typec/ucsi/ucsi.c | 52 ++++++++++++++++++++++++-----------
> --------
> > 1 file changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index f0c2fa1..af9a2a1 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -1100,6 +1100,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> index)
> > con->port = typec_register_port(ucsi->dev, cap);
> > if (IS_ERR(con->port)) {
> > ret = PTR_ERR(con->port);
> > + con->port = NULL;
>
> I'm not sure you need to add that line. See below.
>
> > goto out;
> > }
> >
> > @@ -1186,6 +1187,32 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> index)
> > return ret;
> > }
> >
> > +static void ucsi_unregister_connectors(struct ucsi *ucsi)
> > +{
> > + struct ucsi_connector *con;
> > + int i;
> > +
> > + if (!ucsi->connector)
> > + return;
> > +
> > + for (i = 0; i < ucsi->cap.num_connectors; i++) {
> > + con = &ucsi->connector[i];
> > + if (!con->port)
> > + break;
>
> if (IS_ERR_OR_NULL(con->port))
> break;
Good suggestion, will change it next version.
>
> thanks,
>
> --
> heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v5 2/3] usb: typec: ucsi: do not allocate one extra unused connector
2022-04-25 8:13 ` Heikki Krogerus
@ 2022-04-25 8:23 ` Linyu Yuan (QUIC)
0 siblings, 0 replies; 9+ messages in thread
From: Linyu Yuan (QUIC) @ 2022-04-25 8:23 UTC (permalink / raw)
To: Heikki Krogerus, Linyu Yuan (QUIC)
Cc: Greg Kroah-Hartman, linux-usb, Jack Pham (QUIC)
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Monday, April 25, 2022 4:14 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> Subject: Re: [PATCH v5 2/3] usb: typec: ucsi: do not allocate one extra
> unused connector
>
> On Fri, Apr 22, 2022 at 11:10:21AM +0800, Linyu Yuan wrote:
> > In ucsi_init(), it allocate number of (ucsi->cap.num_connectors + 1)
> > connectors, there is one extra as the ending.
> > ucsi_unregister_connectors() is safe to unregister all ucsi connectors
> > according ucsi->cap.num_connectors.
> >
> > Let's remove the extra one connector to save memory.
>
> Maybe you could just merge this one into the first patch.
Sure, thanks, will merge next version.
>
> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > ---
> > v2: new change
> > v3: no change
> > v4: fix a typo extral -> extra in commit description
> > v5: update commit description
> >
> > drivers/usb/typec/ucsi/ucsi.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index af9a2a1..ce9192e 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -1251,7 +1251,7 @@ static int ucsi_init(struct ucsi *ucsi)
> > }
> >
> > /* Allocate the connectors. Released in ucsi_unregister() */
> > - ucsi->connector = kcalloc(ucsi->cap.num_connectors + 1,
> > + ucsi->connector = kcalloc(ucsi->cap.num_connectors,
> > sizeof(*ucsi->connector), GFP_KERNEL);
> > if (!ucsi->connector) {
> > ret = -ENOMEM;
> > --
> > 2.7.4
>
> thanks,
>
> --
> heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-04-25 8:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-22 3:10 [PATCH v5 0/3] usb: typec: ucsi: allow retry to find role switch Linyu Yuan
2022-04-22 3:10 ` [PATCH v5 1/3] usb: typec: ucsi: add a common function ucsi_unregister_connectors() Linyu Yuan
2022-04-25 8:09 ` Heikki Krogerus
2022-04-25 8:23 ` Linyu Yuan (QUIC)
2022-04-22 3:10 ` [PATCH v5 2/3] usb: typec: ucsi: do not allocate one extra unused connector Linyu Yuan
2022-04-25 8:13 ` Heikki Krogerus
2022-04-25 8:23 ` Linyu Yuan (QUIC)
2022-04-22 3:10 ` [PATCH v5 3/3] usb: typec: ucsi: Wait for the USB role switches Linyu Yuan
2022-04-25 8:14 ` Heikki Krogerus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.