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
Abhishek Pandit-Subedi (2):
usb: typec: ucsi: Fix null deref 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: 684e9f5f97eb4b7831298ffad140d5c1d426ff27
--
2.44.0.769.g3c40516874-goog
From: Abhishek Pandit-Subedi <[email protected]>
ucsi_register_altmode checks IS_ERR on returned pointer and treats
NULL as valid. This results in a null deref when
trace_ucsi_register_altmode is called.
Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
---
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..c663dce0659ee 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 ERR_PTR(-EOPNOTSUPP);
}
static inline void
--
2.44.0.769.g3c40516874-goog
From: Abhishek Pandit-Subedi <[email protected]>
When adding altmode ops, update the sysfs group so that visibility is
also recalculated.
Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
---
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.44.0.769.g3c40516874-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.
Signed-off-by: Jameson Thies <[email protected]>
---
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 b8d56a443531f..cd4c3b7a5d989 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.44.0.769.g3c40516874-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.
Signed-off-by: Jameson Thies <[email protected]>
---
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 cd4c3b7a5d989..ea76d42fd1504 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.44.0.769.g3c40516874-goog
> ucsi_register_altmode checks IS_ERR on returned pointer and treats
> NULL as valid. This results in a null deref when
> trace_ucsi_register_altmode is called.
I find that the change description can be improved further.
Is another imperative wording desirable?
Can it be nicer to use the term “dereference” for the final commit message?
Regards,
Markus
On Sat, Apr 20, 2024 at 03:15:16PM +0200, Markus Elfring wrote:
> > ucsi_register_altmode checks IS_ERR on returned pointer and treats
> > NULL as valid. This results in a null deref when
> > trace_ucsi_register_altmode is called.
>
> I find that the change description can be improved further.
> Is another imperative wording desirable?
>
> Can it be nicer to use the term “dereference” for the final commit message?
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list. I strongly suggest that you not do this anymore. Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all. The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback. Please feel free to also ignore emails
from them.
thanks,
greg k-h's patch email bot
Hi Jameson,
On Fri, Apr 19, 2024 at 09:16:47PM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <[email protected]>
>
> ucsi_register_altmode checks IS_ERR on returned pointer and treats
> NULL as valid. This results in a null deref when
> trace_ucsi_register_altmode is called.
>
> Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
> ---
> 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..c663dce0659ee 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 ERR_PTR(-EOPNOTSUPP);
> }
Hm. This does not look correct to me. Ignoring trace the old code
would have returned success if displayport is not compiled in and
all altmodes (except for display port) would be registered.
With your code ucsi_register_altmodes will always fail and abort
altmode registration if it finds a displayport altmode and
CONFIG_TYPEC_DP_ALTMODE is not set. I don't think this is what we want.
Maybe it is better to guard the trace call with an if?
Am I missing something?
Best regards,
Christian
On Fri, Apr 19, 2024 at 09:16:48PM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <[email protected]>
>
> When adding altmode ops, update the sysfs group so that visibility is
> also recalculated.
>
> Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
> ---
> 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.44.0.769.g3c40516874-goog
--
heikki
On Fri, Apr 19, 2024 at 09:16:49PM +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.
>
> Signed-off-by: Jameson Thies <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
> ---
> 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 b8d56a443531f..cd4c3b7a5d989 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.44.0.769.g3c40516874-goog
--
heikki
On Fri, Apr 19, 2024 at 09:16:50PM +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.
>
> Signed-off-by: Jameson Thies <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
> ---
> 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 cd4c3b7a5d989..ea76d42fd1504 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.44.0.769.g3c40516874-goog
--
heikki
On Fri, Apr 19, 2024 at 09:16:48PM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <[email protected]>
>
> When adding altmode ops, update the sysfs group so that visibility is
> also recalculated.
>
> Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
Reviewed-by: Benson Leung <[email protected]>
> ---
> 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.44.0.769.g3c40516874-goog
>
On Fri, Apr 19, 2024 at 09:16:50PM +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.
>
> Signed-off-by: Jameson Thies <[email protected]>
Reviewed-by: Benson Leung <[email protected]>
> ---
> 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 cd4c3b7a5d989..ea76d42fd1504 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.44.0.769.g3c40516874-goog
>
On Fri, Apr 19, 2024 at 09:16:49PM +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.
>
> Signed-off-by: Jameson Thies <[email protected]>
Reviewed-by: Benson Leung <[email protected]>
> ---
> 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 b8d56a443531f..cd4c3b7a5d989 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.44.0.769.g3c40516874-goog
>
Hi Christian,
thank you for catching this. You are correct, this would prevent
correct altmode registration if CONFIG_TYPEC_DP_ALTMODE is not set.
There was a miscommunication on our end when setting up this series.
The intention was to check for the EOPNOTSUPP response and fall back
to default altmode registration when CONFIG_TYPEC_DP_ALTMODE is not
set. Sorry about the confusion, I'll fix this in a v2 series shortly.
Thanks,
Jameson