* [PATCH v2 0/4] usb: typec: ucsi: Update UCSI alternate mode
@ 2024-04-24 1:48 Jameson Thies
2024-04-24 1:48 ` [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Jameson Thies @ 2024-04-24 1:48 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
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 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 | 21 ++++++++++++++++-----
drivers/usb/typec/ucsi/ucsi.h | 2 +-
include/linux/usb/typec.h | 3 +++
6 files changed, 39 insertions(+), 9 deletions(-)
base-commit: 0d31ea587709216d88183fe4ca0c8aba5e0205b8
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-24 1:48 [PATCH v2 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
@ 2024-04-24 1:48 ` Jameson Thies
2024-04-24 2:06 ` Dmitry Baryshkov
` (2 more replies)
2024-04-24 1:48 ` [PATCH v2 2/4] usb: typec: Update sysfs when setting ops Jameson Thies
` (2 subsequent siblings)
3 siblings, 3 replies; 16+ messages in thread
From: Jameson Thies @ 2024-04-24 1:48 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. Return an error from
ucsi_register_displayport when it is not supported and register the
altmode with typec_port_register_altmode.
Reviewed-by: Jameson Thies <jthies@google.com>
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Changes in V2:
- Checks for error response from ucsi_register_displayport when
registering DisplayPort alternate mode.
drivers/usb/typec/ucsi/ucsi.c | 3 +++
drivers/usb/typec/ucsi/ucsi.h | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index cb52e7b0a2c5c..f3b413f94fd28 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
switch (desc->svid) {
case USB_TYPEC_DP_SID:
alt = ucsi_register_displayport(con, override, i, desc);
+ if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP)
+ alt = typec_port_register_altmode(con->port, desc);
+
break;
case USB_TYPEC_NVIDIA_VLINK_SID:
if (desc->vdo == USB_TYPEC_NVIDIA_VLINK_DBG_VDO)
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] 16+ messages in thread
* [PATCH v2 2/4] usb: typec: Update sysfs when setting ops
2024-04-24 1:48 [PATCH v2 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
2024-04-24 1:48 ` [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
@ 2024-04-24 1:48 ` Jameson Thies
2024-04-24 1:48 ` [PATCH v2 3/4] usb: typec: ucsi: Delay alternate mode discovery Jameson Thies
2024-04-24 1:48 ` [PATCH v2 4/4] usb: typec: ucsi: Always set number of alternate modes Jameson Thies
3 siblings, 0 replies; 16+ messages in thread
From: Jameson Thies @ 2024-04-24 1:48 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,
Benson Leung
From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
When adding altmode ops, update the sysfs group so that visibility is
also recalculated.
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Benson Leung <bleung@chromium.org>
Reviewed-by: Jameson Thies <jthies@google.com>
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
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.44.0.769.g3c40516874-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/4] usb: typec: ucsi: Delay alternate mode discovery
2024-04-24 1:48 [PATCH v2 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
2024-04-24 1:48 ` [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
2024-04-24 1:48 ` [PATCH v2 2/4] usb: typec: Update sysfs when setting ops Jameson Thies
@ 2024-04-24 1:48 ` Jameson Thies
2024-04-24 1:48 ` [PATCH v2 4/4] usb: typec: ucsi: Always set number of alternate modes Jameson Thies
3 siblings, 0 replies; 16+ messages in thread
From: Jameson Thies @ 2024-04-24 1:48 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,
Benson Leung
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 <heikki.krogerus@linux.intel.com>
Reviewed-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Jameson Thies <jthies@google.com>
---
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 f3b413f94fd28..f167891e3f79d 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -966,7 +966,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:
@@ -1250,7 +1250,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] 16+ messages in thread
* [PATCH v2 4/4] usb: typec: ucsi: Always set number of alternate modes
2024-04-24 1:48 [PATCH v2 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
` (2 preceding siblings ...)
2024-04-24 1:48 ` [PATCH v2 3/4] usb: typec: ucsi: Delay alternate mode discovery Jameson Thies
@ 2024-04-24 1:48 ` Jameson Thies
3 siblings, 0 replies; 16+ messages in thread
From: Jameson Thies @ 2024-04-24 1:48 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,
Benson Leung
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 <heikki.krogerus@linux.intel.com>
Reviewed-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Jameson Thies <jthies@google.com>
---
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 f167891e3f79d..994b8a6b6be96 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -815,10 +815,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;
@@ -1141,7 +1142,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;
@@ -1175,6 +1176,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] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-24 1:48 ` [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
@ 2024-04-24 2:06 ` Dmitry Baryshkov
2024-04-25 0:00 ` Abhishek Pandit-Subedi
2024-04-25 8:51 ` Markus Elfring
2024-04-30 7:17 ` Dan Carpenter
2 siblings, 1 reply; 16+ messages in thread
From: Dmitry Baryshkov @ 2024-04-24 2:06 UTC (permalink / raw)
To: Jameson Thies
Cc: heikki.krogerus, linux-usb, pmalani, bleung, abhishekpandit,
andersson, fabrice.gasnier, gregkh, hdegoede, neil.armstrong,
rajaram.regupathy, saranya.gopal, linux-kernel
On Wed, 24 Apr 2024 at 04:48, Jameson Thies <jthies@google.com> 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. Return an error from
> ucsi_register_displayport when it is not supported and register the
> altmode with typec_port_register_altmode.
>
> Reviewed-by: Jameson Thies <jthies@google.com>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> Changes in V2:
> - Checks for error response from ucsi_register_displayport when
> registering DisplayPort alternate mode.
>
> drivers/usb/typec/ucsi/ucsi.c | 3 +++
> drivers/usb/typec/ucsi/ucsi.h | 2 +-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index cb52e7b0a2c5c..f3b413f94fd28 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
> switch (desc->svid) {
> case USB_TYPEC_DP_SID:
> alt = ucsi_register_displayport(con, override, i, desc);
> + if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP)
This makes it ignore EOPNOTSUPP if it is returned by the non-stub
implementation. I think the current state is actually better than the
implementation found in this patch. I'd suggest adding a comment to
ucsi_register_displayport() stub instead.
> + alt = typec_port_register_altmode(con->port, desc);
> +
> break;
> case USB_TYPEC_NVIDIA_VLINK_SID:
> if (desc->vdo == USB_TYPEC_NVIDIA_VLINK_DBG_VDO)
> 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
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-24 2:06 ` Dmitry Baryshkov
@ 2024-04-25 0:00 ` Abhishek Pandit-Subedi
2024-04-25 5:30 ` Christian A. Ehrhardt
0 siblings, 1 reply; 16+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-04-25 0:00 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Jameson Thies, heikki.krogerus, linux-usb, pmalani, bleung,
andersson, fabrice.gasnier, gregkh, hdegoede, neil.armstrong,
rajaram.regupathy, saranya.gopal, linux-kernel
On Tue, Apr 23, 2024 at 7:06 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, 24 Apr 2024 at 04:48, Jameson Thies <jthies@google.com> 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. Return an error from
> > ucsi_register_displayport when it is not supported and register the
> > altmode with typec_port_register_altmode.
> >
> > Reviewed-by: Jameson Thies <jthies@google.com>
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> > Changes in V2:
> > - Checks for error response from ucsi_register_displayport when
> > registering DisplayPort alternate mode.
> >
> > drivers/usb/typec/ucsi/ucsi.c | 3 +++
> > drivers/usb/typec/ucsi/ucsi.h | 2 +-
> > 2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index cb52e7b0a2c5c..f3b413f94fd28 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
> > switch (desc->svid) {
> > case USB_TYPEC_DP_SID:
> > alt = ucsi_register_displayport(con, override, i, desc);
> > + if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP)
>
> This makes it ignore EOPNOTSUPP if it is returned by the non-stub
> implementation. I think the current state is actually better than the
> implementation found in this patch. I'd suggest adding a comment to
> ucsi_register_displayport() stub instead.
So originally on my system, I didn't have the displayport driver
config enabled. My expectation was that the alt-mode would show up but
would not be controllable (like all other alt-modes without drivers).
What ends up happening is that no alt-mode shows up and trying to
enable the trace crashes.
When the displayport support isn't there, I think it should just be
enumerated as a normal, unsupported alt-mode.
>
> > + alt = typec_port_register_altmode(con->port, desc);
> > +
> > break;
> > case USB_TYPEC_NVIDIA_VLINK_SID:
> > if (desc->vdo == USB_TYPEC_NVIDIA_VLINK_DBG_VDO)
> > 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
> >
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-25 0:00 ` Abhishek Pandit-Subedi
@ 2024-04-25 5:30 ` Christian A. Ehrhardt
0 siblings, 0 replies; 16+ messages in thread
From: Christian A. Ehrhardt @ 2024-04-25 5:30 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: Dmitry Baryshkov, Jameson Thies, heikki.krogerus, linux-usb,
pmalani, bleung, andersson, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
Hi,
On Wed, Apr 24, 2024 at 05:00:24PM -0700, Abhishek Pandit-Subedi wrote:
> On Tue, Apr 23, 2024 at 7:06 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Wed, 24 Apr 2024 at 04:48, Jameson Thies <jthies@google.com> 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. Return an error from
> > > ucsi_register_displayport when it is not supported and register the
> > > altmode with typec_port_register_altmode.
> > >
> > > Reviewed-by: Jameson Thies <jthies@google.com>
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > ---
> > > Changes in V2:
> > > - Checks for error response from ucsi_register_displayport when
> > > registering DisplayPort alternate mode.
> > >
> > > drivers/usb/typec/ucsi/ucsi.c | 3 +++
> > > drivers/usb/typec/ucsi/ucsi.h | 2 +-
> > > 2 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > index cb52e7b0a2c5c..f3b413f94fd28 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > @@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
> > > switch (desc->svid) {
> > > case USB_TYPEC_DP_SID:
> > > alt = ucsi_register_displayport(con, override, i, desc);
> > > + if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP)
> >
> > This makes it ignore EOPNOTSUPP if it is returned by the non-stub
> > implementation. I think the current state is actually better than the
> > implementation found in this patch. I'd suggest adding a comment to
> > ucsi_register_displayport() stub instead.
>
> So originally on my system, I didn't have the displayport driver
> config enabled. My expectation was that the alt-mode would show up but
> would not be controllable (like all other alt-modes without drivers).
> What ends up happening is that no alt-mode shows up and trying to
> enable the trace crashes.
>
> When the displayport support isn't there, I think it should just be
> enumerated as a normal, unsupported alt-mode.
This sounds reasonable. Thus if displayport support is not compiled in
ucsi_register_displayport() should just call typec_port_register_altmode(),
I guess.
Best regards,
Christian
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-24 1:48 ` [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
2024-04-24 2:06 ` Dmitry Baryshkov
@ 2024-04-25 8:51 ` Markus Elfring
2024-04-25 19:35 ` Greg Kroah-Hartman
2024-04-30 7:17 ` Dan Carpenter
2 siblings, 1 reply; 16+ messages in thread
From: Markus Elfring @ 2024-04-25 8:51 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, Prashant Malani, Rajaram Regupathy, Saranya Gopal,
Uwe Kleine-König
…
> 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.
…
Can it be nicer to use the term “null pointer dereference” for
the commit message here?
Regards,
Markus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-25 8:51 ` Markus Elfring
@ 2024-04-25 19:35 ` Greg Kroah-Hartman
2024-04-30 0:43 ` Jameson Thies
0 siblings, 1 reply; 16+ messages in thread
From: Greg Kroah-Hartman @ 2024-04-25 19:35 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, Prashant Malani, Rajaram Regupathy, Saranya Gopal,
Uwe Kleine-König
On Thu, Apr 25, 2024 at 10:51:53AM +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.
> …
>
> Can it be nicer to use the term “null pointer dereference” for
> the commit message here?
>
> Regards,
> Markus
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] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-25 19:35 ` Greg Kroah-Hartman
@ 2024-04-30 0:43 ` Jameson Thies
2024-04-30 1:00 ` Dmitry Baryshkov
0 siblings, 1 reply; 16+ messages in thread
From: Jameson Thies @ 2024-04-30 0:43 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Markus Elfring, Abhishek Pandit-Subedi, linux-usb,
kernel-janitors, Heikki Krogerus, LKML, Benson Leung,
Bjorn Andersson, Dmitry Baryshkov, Fabrice Gasnier, Hans de Goede,
Neil Armstrong, Prashant Malani, Rajaram Regupathy, Saranya Gopal,
Uwe Kleine-König
Hi Dmitry,
what are your thoughts on Abhishek's comment? I think we should
attempt to register the alternate mode when CONFIG_TYPEC_DP_ALTMODE is
not enabled. It would give us a more accurate representation of the
partner in user space. I understand your point about ignoring a
potential EOPNOTSUPP response from the non-stub function. What if we
leave ucsi.c alone, and replace the stub function's null return with a
call to typec_port_register_altmode(). That would register DP altmode
as an unsupported mode when CONFIG_TYPEC_DP_ALTMODE is not enabled,
and fix the null pointer dereference. But, it won't change behavior
when CONFIG_TYPEC_DP_ALTMODE is enabled.
Hi Markus,
thanks for your feedback. I'll update the commit message to say "null
pointer dereference" when I upload a v3 series.
Thanks,
Jameson
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-30 0:43 ` Jameson Thies
@ 2024-04-30 1:00 ` Dmitry Baryshkov
2024-04-30 1:14 ` Jameson Thies
0 siblings, 1 reply; 16+ messages in thread
From: Dmitry Baryshkov @ 2024-04-30 1:00 UTC (permalink / raw)
To: Jameson Thies
Cc: Greg Kroah-Hartman, Markus Elfring, Abhishek Pandit-Subedi,
linux-usb, kernel-janitors, Heikki Krogerus, LKML, Benson Leung,
Bjorn Andersson, Fabrice Gasnier, Hans de Goede, Neil Armstrong,
Prashant Malani, Rajaram Regupathy, Saranya Gopal,
Uwe Kleine-König
On Mon, Apr 29, 2024 at 05:43:16PM -0700, Jameson Thies wrote:
> Hi Dmitry,
> what are your thoughts on Abhishek's comment? I think we should
> attempt to register the alternate mode when CONFIG_TYPEC_DP_ALTMODE is
> not enabled. It would give us a more accurate representation of the
> partner in user space. I understand your point about ignoring a
> potential EOPNOTSUPP response from the non-stub function. What if we
> leave ucsi.c alone, and replace the stub function's null return with a
> call to typec_port_register_altmode(). That would register DP altmode
> as an unsupported mode when CONFIG_TYPEC_DP_ALTMODE is not enabled,
> and fix the null pointer dereference. But, it won't change behavior
> when CONFIG_TYPEC_DP_ALTMODE is enabled.
Yes, this sounds like a perfect idea!
>
> Hi Markus,
> thanks for your feedback. I'll update the commit message to say "null
> pointer dereference" when I upload a v3 series.
>
> Thanks,
> Jameson
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-30 1:00 ` Dmitry Baryshkov
@ 2024-04-30 1:14 ` Jameson Thies
0 siblings, 0 replies; 16+ messages in thread
From: Jameson Thies @ 2024-04-30 1:14 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Greg Kroah-Hartman, Markus Elfring, Abhishek Pandit-Subedi,
linux-usb, kernel-janitors, Heikki Krogerus, LKML, Benson Leung,
Bjorn Andersson, Fabrice Gasnier, Hans de Goede, Neil Armstrong,
Prashant Malani, Rajaram Regupathy, Saranya Gopal,
Uwe Kleine-König
Great! I'll post a v3 series applying this shortly.
Thanks,
Jameson
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-24 1:48 ` [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
2024-04-24 2:06 ` Dmitry Baryshkov
2024-04-25 8:51 ` Markus Elfring
@ 2024-04-30 7:17 ` Dan Carpenter
2024-05-02 22:47 ` Jameson Thies
2 siblings, 1 reply; 16+ messages in thread
From: Dan Carpenter @ 2024-04-30 7:17 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
On Wed, Apr 24, 2024 at 01:48:18AM +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. Return an error from
> ucsi_register_displayport when it is not supported and register the
> altmode with typec_port_register_altmode.
>
> Reviewed-by: Jameson Thies <jthies@google.com>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
This is not the correct way to fix this. The normal thing to do would
be to add NULL checks.
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/'
regards,
dan carpenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-04-30 7:17 ` Dan Carpenter
@ 2024-05-02 22:47 ` Jameson Thies
2024-05-03 12:03 ` Dan Carpenter
0 siblings, 1 reply; 16+ messages in thread
From: Jameson Thies @ 2024-05-02 22:47 UTC (permalink / raw)
To: Dan Carpenter
Cc: heikki.krogerus, linux-usb, pmalani, bleung, abhishekpandit,
andersson, dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
Hi Dan,
thank you for the reference. In this case I think returning NULL from
ucsi_register_displayport stub and using a NULL check to prevent a
NULL pointer dereference won't give us the desired behavior. When
CONFIG_TYPEC_DP_ALTMODE is not enabled, the function would return NULL
and the UCSI driver will never end up registering DisplayPort
alternate mode. I'll update the commit message to note that this patch
adds a fallback registration for DisplayPort alternate mode in
addition to simply fixing the NULL pointer dereference caused by
returning it in the ucsi_register_displayport stub. Separately we
could add a NULL check, but I don't think it's necessary. Neither the
non-stub ucsi_register_displayport or typec_port_register_altmode look
like they will return NULL.
Thanks,
Jameson
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace
2024-05-02 22:47 ` Jameson Thies
@ 2024-05-03 12:03 ` Dan Carpenter
0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2024-05-03 12:03 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
It looks like in v3 you changed this to:
- return NULL;
+ return typec_port_register_altmode(con->port, desc);
Which is fine. Returning a special error pointer which means success is
the part that I objected to.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2024-05-03 12:03 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-24 1:48 [PATCH v2 0/4] usb: typec: ucsi: Update UCSI alternate mode Jameson Thies
2024-04-24 1:48 ` [PATCH v2 1/4] usb: typec: ucsi: Fix null deref in trace Jameson Thies
2024-04-24 2:06 ` Dmitry Baryshkov
2024-04-25 0:00 ` Abhishek Pandit-Subedi
2024-04-25 5:30 ` Christian A. Ehrhardt
2024-04-25 8:51 ` Markus Elfring
2024-04-25 19:35 ` Greg Kroah-Hartman
2024-04-30 0:43 ` Jameson Thies
2024-04-30 1:00 ` Dmitry Baryshkov
2024-04-30 1:14 ` Jameson Thies
2024-04-30 7:17 ` Dan Carpenter
2024-05-02 22:47 ` Jameson Thies
2024-05-03 12:03 ` Dan Carpenter
2024-04-24 1:48 ` [PATCH v2 2/4] usb: typec: Update sysfs when setting ops Jameson Thies
2024-04-24 1:48 ` [PATCH v2 3/4] usb: typec: ucsi: Delay alternate mode discovery Jameson Thies
2024-04-24 1:48 ` [PATCH v2 4/4] usb: typec: ucsi: Always set number of alternate modes Jameson Thies
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).