* [PATCH v1 0/4] usb: typec: ucsi: Update UCSI alternate mode
@ 2024-04-19 21:16 Jameson Thies
2024-04-19 21:16 ` [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Jameson Thies @ 2024-04-19 21:16 UTC (permalink / raw)
To: heikki.krogerus, linux-usb
Cc: jthies, pmalani, bleung, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-19 21:16 [PATCH v1 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
@ 2024-04-19 21:16 ` Jameson Thies
2024-04-20 13:15 ` [PATCH " Markus Elfring
2024-04-21 9:30 ` [PATCH v1 " Christian A. Ehrhardt
2024-04-19 21:16 ` [PATCH v1 2/4] usb: typec: Update sysfs when setting ops Jameson Thies
` (2 subsequent siblings)
3 siblings, 2 replies; 15+ messages in thread
From: Jameson Thies @ 2024-04-19 21:16 UTC (permalink / raw)
To: heikki.krogerus, linux-usb
Cc: jthies, pmalani, bleung, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v1 2/4] usb: typec: Update sysfs when setting ops
2024-04-19 21:16 [PATCH v1 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
2024-04-19 21:16 ` [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
@ 2024-04-19 21:16 ` Jameson Thies
2024-04-22 11:38 ` Heikki Krogerus
2024-04-23 15:35 ` Benson Leung
2024-04-19 21:16 ` [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery Jameson Thies
2024-04-19 21:16 ` [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes Jameson Thies
3 siblings, 2 replies; 15+ messages in thread
From: Jameson Thies @ 2024-04-19 21:16 UTC (permalink / raw)
To: heikki.krogerus, linux-usb
Cc: jthies, pmalani, bleung, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery
2024-04-19 21:16 [PATCH v1 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
2024-04-19 21:16 ` [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
2024-04-19 21:16 ` [PATCH v1 2/4] usb: typec: Update sysfs when setting ops Jameson Thies
@ 2024-04-19 21:16 ` Jameson Thies
2024-04-22 12:39 ` Heikki Krogerus
2024-04-23 15:36 ` Benson Leung
2024-04-19 21:16 ` [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes Jameson Thies
3 siblings, 2 replies; 15+ messages in thread
From: Jameson Thies @ 2024-04-19 21:16 UTC (permalink / raw)
To: heikki.krogerus, linux-usb
Cc: jthies, pmalani, bleung, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes
2024-04-19 21:16 [PATCH v1 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
` (2 preceding siblings ...)
2024-04-19 21:16 ` [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery Jameson Thies
@ 2024-04-19 21:16 ` Jameson Thies
2024-04-22 12:41 ` Heikki Krogerus
2024-04-23 15:37 ` Benson Leung
3 siblings, 2 replies; 15+ messages in thread
From: Jameson Thies @ 2024-04-19 21:16 UTC (permalink / raw)
To: heikki.krogerus, linux-usb
Cc: jthies, pmalani, bleung, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-19 21:16 ` [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
@ 2024-04-20 13:15 ` Markus Elfring
2024-04-20 13:20 ` Greg Kroah-Hartman
2024-04-21 9:30 ` [PATCH v1 " Christian A. Ehrhardt
1 sibling, 1 reply; 15+ messages in thread
From: Markus Elfring @ 2024-04-20 13:15 UTC (permalink / raw)
To: Abhishek Pandit-Subedi, linux-usb, kernel-janitors,
Heikki Krogerus, Jameson Thies
Cc: LKML, Benson Leung, Bjorn Andersson, Dmitry Baryshkov,
Fabrice Gasnier, Greg Kroah-Hartman, Hans de Goede,
Neil Armstrong, Rajaram Regupathy, Saranya Gopal
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-20 13:15 ` [PATCH " Markus Elfring
@ 2024-04-20 13:20 ` Greg Kroah-Hartman
0 siblings, 0 replies; 15+ messages in thread
From: Greg Kroah-Hartman @ 2024-04-20 13:20 UTC (permalink / raw)
To: Markus Elfring
Cc: Abhishek Pandit-Subedi, linux-usb, kernel-janitors,
Heikki Krogerus, Jameson Thies, LKML, Benson Leung,
Bjorn Andersson, Dmitry Baryshkov, Fabrice Gasnier, Hans de Goede,
Neil Armstrong, Rajaram Regupathy, Saranya Gopal
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-19 21:16 ` [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
2024-04-20 13:15 ` [PATCH " Markus Elfring
@ 2024-04-21 9:30 ` Christian A. Ehrhardt
2024-04-24 0:29 ` Jameson Thies
1 sibling, 1 reply; 15+ messages in thread
From: Christian A. Ehrhardt @ 2024-04-21 9:30 UTC (permalink / raw)
To: Jameson Thies
Cc: heikki.krogerus, linux-usb, pmalani, bleung, abhishekpandit,
andersson, dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/4] usb: typec: Update sysfs when setting ops
2024-04-19 21:16 ` [PATCH v1 2/4] usb: typec: Update sysfs when setting ops Jameson Thies
@ 2024-04-22 11:38 ` Heikki Krogerus
2024-04-23 15:35 ` Benson Leung
1 sibling, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2024-04-22 11:38 UTC (permalink / raw)
To: Jameson Thies
Cc: linux-usb, pmalani, bleung, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery
2024-04-19 21:16 ` [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery Jameson Thies
@ 2024-04-22 12:39 ` Heikki Krogerus
2024-04-23 15:36 ` Benson Leung
1 sibling, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2024-04-22 12:39 UTC (permalink / raw)
To: Jameson Thies
Cc: linux-usb, pmalani, bleung, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes
2024-04-19 21:16 ` [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes Jameson Thies
@ 2024-04-22 12:41 ` Heikki Krogerus
2024-04-23 15:37 ` Benson Leung
1 sibling, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2024-04-22 12:41 UTC (permalink / raw)
To: Jameson Thies
Cc: linux-usb, pmalani, bleung, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/4] usb: typec: Update sysfs when setting ops
2024-04-19 21:16 ` [PATCH v1 2/4] usb: typec: Update sysfs when setting ops Jameson Thies
2024-04-22 11:38 ` Heikki Krogerus
@ 2024-04-23 15:35 ` Benson Leung
1 sibling, 0 replies; 15+ messages in thread
From: Benson Leung @ 2024-04-23 15:35 UTC (permalink / raw)
To: Jameson Thies
Cc: heikki.krogerus, linux-usb, pmalani, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery
2024-04-19 21:16 ` [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery Jameson Thies
2024-04-22 12:39 ` Heikki Krogerus
@ 2024-04-23 15:36 ` Benson Leung
1 sibling, 0 replies; 15+ messages in thread
From: Benson Leung @ 2024-04-23 15:36 UTC (permalink / raw)
To: Jameson Thies
Cc: heikki.krogerus, linux-usb, pmalani, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes
2024-04-19 21:16 ` [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes Jameson Thies
2024-04-22 12:41 ` Heikki Krogerus
@ 2024-04-23 15:37 ` Benson Leung
1 sibling, 0 replies; 15+ messages in thread
From: Benson Leung @ 2024-04-23 15:37 UTC (permalink / raw)
To: Jameson Thies
Cc: heikki.krogerus, linux-usb, pmalani, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-21 9:30 ` [PATCH v1 " Christian A. Ehrhardt
@ 2024-04-24 0:29 ` Jameson Thies
0 siblings, 0 replies; 15+ messages in thread
From: Jameson Thies @ 2024-04-24 0:29 UTC (permalink / raw)
To: Christian A. Ehrhardt
Cc: heikki.krogerus, linux-usb, pmalani, bleung, abhishekpandit,
andersson, dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-04-24 0:29 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-19 21:16 [PATCH v1 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
2024-04-19 21:16 ` [PATCH v1 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
2024-04-20 13:15 ` [PATCH " Markus Elfring
2024-04-20 13:20 ` Greg Kroah-Hartman
2024-04-21 9:30 ` [PATCH v1 " Christian A. Ehrhardt
2024-04-24 0:29 ` Jameson Thies
2024-04-19 21:16 ` [PATCH v1 2/4] usb: typec: Update sysfs when setting ops Jameson Thies
2024-04-22 11:38 ` Heikki Krogerus
2024-04-23 15:35 ` Benson Leung
2024-04-19 21:16 ` [PATCH v1 3/4] usb: typec: ucsi: Delay alternate mode discovery Jameson Thies
2024-04-22 12:39 ` Heikki Krogerus
2024-04-23 15:36 ` Benson Leung
2024-04-19 21:16 ` [PATCH v1 4/4] usb: typec: ucsi: Always set number of alternate modes Jameson Thies
2024-04-22 12:41 ` Heikki Krogerus
2024-04-23 15:37 ` Benson Leung
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.