Hi Heikki,
This series appliess some changes to the UCSI driver to help support AP
driven alternate mode entry. This includes...
1. An update to the altmode sysfs group after registration to make
"active" writable.
2. A change to the ucsi_partner_task delay when queuing
ucsi_check_altmodes to prevent it from running before other discovery
functions.
3. An update to always define a number of alternate modes for partners
and plugs.
Not related to AP driven altmode entry, there is an additional fix for a
null derefrence in this series.
I tested the series on a ChromeOS v6.8 kernel merged with usb-testing.
That build had some additinal patches to enable a PPM in ChromeOS. Let
me know if you have any questions.
Thanks,
Jameson
Changes in V5:
- Fixed signed off by line order.
Changes in V4:
- Updates to the commit messages.
Changes in V3:
- Returns typec_port_register_altmode call from
ucsi_register_displayport when CONFIG_TYPEC_DP_ALTMODE is not enabled.
Changes in V2:
- Checks for error response from ucsi_register_displayport when
registering DisplayPort alternate mode.
Abhishek Pandit-Subedi (2):
usb: typec: ucsi: Fix null pointer dereference in trace
usb: typec: Update sysfs when setting ops
Jameson Thies (2):
usb: typec: ucsi: Delay alternate mode discovery
usb: typec: ucsi: Always set number of alternate modes
drivers/usb/typec/altmodes/displayport.c | 2 +-
drivers/usb/typec/class.c | 18 +++++++++++++++++-
drivers/usb/typec/ucsi/displayport.c | 2 +-
drivers/usb/typec/ucsi/ucsi.c | 18 +++++++++++++-----
drivers/usb/typec/ucsi/ucsi.h | 2 +-
include/linux/usb/typec.h | 3 +++
6 files changed, 36 insertions(+), 9 deletions(-)
base-commit: e4306116b5e93748b3eaa7666aa55c390b48a8f4
--
2.45.0.118.g7fe29c98d7-goog
From: Abhishek Pandit-Subedi <[email protected]>
ucsi_register_altmode checks IS_ERR for the alt pointer and treats
NULL as valid. When CONFIG_TYPEC_DP_ALTMODE is not enabled,
ucsi_register_displayport returns NULL which causes a NULL pointer
dereference in trace. Rather than return NULL, call
typec_port_register_altmode to register DisplayPort alternate mode
as a non-controllable mode when CONFIG_TYPEC_DP_ALTMODE is not enabled.
Reviewed-by: Benson Leung <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
Signed-off-by: Jameson Thies <[email protected]>
---
Changes in V5:
- Fixed signed off by line order.
Changes in V4:
- Updated commit message.
Changes in V3:
- Returns typec_port_register_altmode call from
ucsi_register_displayport when CONFIG_TYPEC_DP_ALTMODE is not enabled.
Updated commit message.
Changes in V2:
- Checks for error response from ucsi_register_displayport when
registering DisplayPort alternate mode.
drivers/usb/typec/ucsi/ucsi.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index c4d103db9d0f8..f66224a270bc6 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -496,7 +496,7 @@ ucsi_register_displayport(struct ucsi_connector *con,
bool override, int offset,
struct typec_altmode_desc *desc)
{
- return NULL;
+ return typec_port_register_altmode(con->port, desc);
}
static inline void
--
2.45.0.118.g7fe29c98d7-goog
From: Abhishek Pandit-Subedi <[email protected]>
When adding altmode ops, update the sysfs group so that visibility is
also recalculated.
Reviewed-by: Heikki Krogerus <[email protected]>
Reviewed-by: Benson Leung <[email protected]>
Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
Signed-off-by: Jameson Thies <[email protected]>
---
Changes in V5:
- Fixed signed off by line order.
Changes in V4:
- None.
Changes in V3:
- None.
Changes in V2:
- None.
drivers/usb/typec/altmodes/displayport.c | 2 +-
drivers/usb/typec/class.c | 18 +++++++++++++++++-
drivers/usb/typec/ucsi/displayport.c | 2 +-
include/linux/usb/typec.h | 3 +++
4 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 596cd4806018b..92cc1b1361208 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -746,7 +746,7 @@ int dp_altmode_probe(struct typec_altmode *alt)
dp->alt = alt;
alt->desc = "DisplayPort";
- alt->ops = &dp_altmode_ops;
+ typec_altmode_set_ops(alt, &dp_altmode_ops);
if (plug) {
plug->desc = "Displayport";
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 9610e647a8d48..9262fcd4144f8 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -467,6 +467,22 @@ static const struct attribute_group *typec_altmode_groups[] = {
NULL
};
+/**
+ * typec_altmode_set_ops - Set ops for altmode
+ * @adev: Handle to the alternate mode
+ * @ops: Ops for the alternate mode
+ *
+ * After setting ops, attribute visiblity needs to be refreshed if the alternate
+ * mode can be activated.
+ */
+void typec_altmode_set_ops(struct typec_altmode *adev,
+ const struct typec_altmode_ops *ops)
+{
+ adev->ops = ops;
+ sysfs_update_group(&adev->dev.kobj, &typec_altmode_group);
+}
+EXPORT_SYMBOL_GPL(typec_altmode_set_ops);
+
static int altmode_id_get(struct device *dev)
{
struct ida *ids;
@@ -2317,7 +2333,7 @@ void typec_port_register_altmodes(struct typec_port *port,
continue;
}
- alt->ops = ops;
+ typec_altmode_set_ops(alt, ops);
typec_altmode_set_drvdata(alt, drvdata);
altmodes[index] = alt;
index++;
diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index d9d3c91125ca8..eb7b8d6e47d00 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -337,7 +337,7 @@ struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
dp->con = con;
dp->alt = alt;
- alt->ops = &ucsi_displayport_ops;
+ typec_altmode_set_ops(alt, &ucsi_displayport_ops);
typec_altmode_set_drvdata(alt, dp);
return alt;
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index b35b427561ab5..549275f8ac1b3 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -167,6 +167,9 @@ struct typec_port *typec_altmode2port(struct typec_altmode *alt);
void typec_altmode_update_active(struct typec_altmode *alt, bool active);
+void typec_altmode_set_ops(struct typec_altmode *alt,
+ const struct typec_altmode_ops *ops);
+
enum typec_plug_index {
TYPEC_PLUG_SOP_P,
TYPEC_PLUG_SOP_PP,
--
2.45.0.118.g7fe29c98d7-goog
Delay the ucsi_check_altmodes task to be inline with surrounding partner
tasks. This allows partner, cable and identity discovery to complete
before alternate mode registration. With that order, alternate mode
discovery can be used to indicate the ucsi driver has completed
discovery.
Reviewed-by: Heikki Krogerus <[email protected]>
Reviewed-by: Benson Leung <[email protected]>
Signed-off-by: Jameson Thies <[email protected]>
---
Changes in V5:
- None.
Changes in V4:
- None.
Changes in V3:
- None.
Changes in V2:
- None.
drivers/usb/typec/ucsi/ucsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index cb52e7b0a2c5c..bb6e57064513d 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -963,7 +963,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
con->rdo = con->status.request_data_obj;
typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
ucsi_partner_task(con, ucsi_get_src_pdos, 30, 0);
- ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
+ ucsi_partner_task(con, ucsi_check_altmodes, 30, HZ);
ucsi_partner_task(con, ucsi_register_partner_pdos, 1, HZ);
break;
case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
@@ -1247,7 +1247,7 @@ static void ucsi_handle_connector_change(struct work_struct *work)
}
if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
- ucsi_partner_task(con, ucsi_check_altmodes, 1, 0);
+ ucsi_partner_task(con, ucsi_check_altmodes, 1, HZ);
out_unlock:
mutex_unlock(&con->lock);
--
2.45.0.118.g7fe29c98d7-goog
Providing the number of known alternate modes allows user space to
determine when device registration has completed. Always register a
number of known alternate modes for the partner and cable plug, even
when the number of supported alternate modes is 0.
Reviewed-by: Heikki Krogerus <[email protected]>
Reviewed-by: Benson Leung <[email protected]>
Signed-off-by: Jameson Thies <[email protected]>
---
Changes in V5:
- None.
Changes in V4:
- None.
Changes in V3:
- None.
Changes in V2:
- None.
drivers/usb/typec/ucsi/ucsi.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index bb6e57064513d..52a14bfe4107e 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -812,10 +812,11 @@ static int ucsi_check_altmodes(struct ucsi_connector *con)
/* Ignoring the errors in this case. */
if (con->partner_altmode[0]) {
num_partner_am = ucsi_get_num_altmode(con->partner_altmode);
- if (num_partner_am > 0)
- typec_partner_set_num_altmodes(con->partner, num_partner_am);
+ typec_partner_set_num_altmodes(con->partner, num_partner_am);
ucsi_altmode_update_active(con);
return 0;
+ } else {
+ typec_partner_set_num_altmodes(con->partner, 0);
}
return ret;
@@ -1138,7 +1139,7 @@ static int ucsi_check_connection(struct ucsi_connector *con)
static int ucsi_check_cable(struct ucsi_connector *con)
{
u64 command;
- int ret;
+ int ret, num_plug_am;
if (con->cable)
return 0;
@@ -1172,6 +1173,13 @@ static int ucsi_check_cable(struct ucsi_connector *con)
return ret;
}
+ if (con->plug_altmode[0]) {
+ num_plug_am = ucsi_get_num_altmode(con->plug_altmode);
+ typec_plug_set_num_altmodes(con->plug, num_plug_am);
+ } else {
+ typec_plug_set_num_altmodes(con->plug, 0);
+ }
+
return 0;
}
--
2.45.0.118.g7fe29c98d7-goog
Hi everyone. Friendly ping on this series. It still applies cleanly to
the top of usb-next. If there is anything you would like me to change,
please let me know.
Thanks,
Jameson
On Fri, May 10, 2024 at 08:12:41PM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <[email protected]>
>
> ucsi_register_altmode checks IS_ERR for the alt pointer and treats
> NULL as valid. When CONFIG_TYPEC_DP_ALTMODE is not enabled,
> ucsi_register_displayport returns NULL which causes a NULL pointer
> dereference in trace. Rather than return NULL, call
> typec_port_register_altmode to register DisplayPort alternate mode
> as a non-controllable mode when CONFIG_TYPEC_DP_ALTMODE is not enabled.
>
> Reviewed-by: Benson Leung <[email protected]>
> Reviewed-by: Heikki Krogerus <[email protected]>
> Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
> Signed-off-by: Jameson Thies <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Fri, May 10, 2024 at 08:12:42PM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <[email protected]>
>
> When adding altmode ops, update the sysfs group so that visibility is
> also recalculated.
>
> Reviewed-by: Heikki Krogerus <[email protected]>
> Reviewed-by: Benson Leung <[email protected]>
> Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
> Signed-off-by: Jameson Thies <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Fri, May 10, 2024 at 08:12:43PM +0000, Jameson Thies wrote:
> Delay the ucsi_check_altmodes task to be inline with surrounding partner
> tasks. This allows partner, cable and identity discovery to complete
> before alternate mode registration. With that order, alternate mode
> discovery can be used to indicate the ucsi driver has completed
> discovery.
>
> Reviewed-by: Heikki Krogerus <[email protected]>
> Reviewed-by: Benson Leung <[email protected]>
> Signed-off-by: Jameson Thies <[email protected]>
> ---
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Fri, May 10, 2024 at 08:12:44PM +0000, Jameson Thies wrote:
> Providing the number of known alternate modes allows user space to
> determine when device registration has completed. Always register a
> number of known alternate modes for the partner and cable plug, even
> when the number of supported alternate modes is 0.
>
> Reviewed-by: Heikki Krogerus <[email protected]>
> Reviewed-by: Benson Leung <[email protected]>
> Signed-off-by: Jameson Thies <[email protected]>
> ---
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry