[PATCH v1 0/4] usb: typec: ucsi: Update UCSI alternate mode

[PATCH v1 0/4] usb: typec: ucsi: Update UCSI alternate mode

[Jameson Thies]

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

[PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace

[Jameson Thies]

From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

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 <abhishekpandit@chromium.org>
---
 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

Re: [PATCH 1/4] usb: typec: ucsi: Fix null deref in trace

[Markus Elfring]

> 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

Re: [PATCH 1/4] usb: typec: ucsi: Fix null deref in trace

[Greg Kroah-Hartman]

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

Re: [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace

[Christian A. Ehrhardt]

Hi Jameson,

On Fri, Apr 19, 2024 at 09:16:47PM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> 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 <abhishekpandit@chromium.org>
> ---
>  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

Re: [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace

[Jameson Thies]

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

[PATCH v1 2/4] usb: typec: Update sysfs when setting ops

[Jameson Thies]

From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

When adding altmode ops, update the sysfs group so that visibility is
also recalculated.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
 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

Re: [PATCH v1 2/4] usb: typec: Update sysfs when setting ops

[Heikki Krogerus]

On Fri, Apr 19, 2024 at 09:16:48PM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> When adding altmode ops, update the sysfs group so that visibility is
> also recalculated.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  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

Re: [PATCH v1 2/4] usb: typec: Update sysfs when setting ops

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 3446 bytes --]

On Fri, Apr 19, 2024 at 09:16:48PM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> When adding altmode ops, update the sysfs group so that visibility is
> also recalculated.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Reviewed-by: Benson Leung <bleung@chromium.org>

> ---
>  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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery

[Jameson Thies]

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 <jthies@google.com>
---
 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

Re: [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery

[Heikki Krogerus]

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 <jthies@google.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  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

Re: [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]

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 <jthies@google.com>

Reviewed-by: Benson Leung <bleung@chromium.org>

> ---
>  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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes

[Jameson Thies]

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 <jthies@google.com>
---
 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

Re: [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes

[Heikki Krogerus]

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 <jthies@google.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  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

Re: [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

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 <jthies@google.com>

Reviewed-by: Benson Leung <bleung@chromium.org>

> ---
>  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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]