Some usb hubs will negotiate DisplayPort Alt mode with the device but will then negotiate a data role swap after entering the alt mode. The data role swap causes the device to unregister all alt modes, however the usb hub will still send Attention messages even after failing to reregister the Alt Mode. type_altmode_attention currently does not verify whether or not a device's altmode partner exists, which results in a NULL pointer error when dereferencing the typec_altmode and typec_altmode_ops belonging to the altmode partner.
This patch verifies the presence of a device's altmode partner before sending the Attention message to the Alt Mode driver. It also changes the return type from void to int so errors can be logged at the tcpm level.
Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes") Cc: stable@vger.kernel.org Signed-off-by: RD Babiera rdbabiera@google.com --- drivers/usb/typec/bus.c | 12 ++++++++++-- drivers/usb/typec/tcpm/tcpm.c | 5 ++++- include/linux/usb/typec_altmode.h | 2 +- 3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c index fe5b9a2e61f5..2f1823e16b3b 100644 --- a/drivers/usb/typec/bus.c +++ b/drivers/usb/typec/bus.c @@ -183,12 +183,20 @@ EXPORT_SYMBOL_GPL(typec_altmode_exit); * * Notifies the partner of @adev about Attention command. */ -void typec_altmode_attention(struct typec_altmode *adev, u32 vdo) +int typec_altmode_attention(struct typec_altmode *adev, u32 vdo) { - struct typec_altmode *pdev = &to_altmode(adev)->partner->adev; + struct altmode *partner = to_altmode(adev)->partner; + struct typec_altmode *pdev = &partner->adev; + + if (!partner || !pdev) + return -ENODEV;
if (pdev->ops && pdev->ops->attention) pdev->ops->attention(pdev, vdo); + else + return -EOPNOTSUPP; + + return 0; } EXPORT_SYMBOL_GPL(typec_altmode_attention);
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 829d75ebab42..be37a662e54d 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -1791,6 +1791,7 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port, u32 p[PD_MAX_PAYLOAD]; u32 response[8] = { }; int i, rlen = 0; + int ret;
for (i = 0; i < cnt; i++) p[i] = le32_to_cpu(payload[i]); @@ -1877,7 +1878,9 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port, } break; case ADEV_ATTENTION: - typec_altmode_attention(adev, p[1]); + ret = typec_altmode_attention(adev, p[1]); + if (ret) + tcpm_log(port, "altmode_attention failed ret:%d", ret); break; } } diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h index 350d49012659..28aeef8f9e7b 100644 --- a/include/linux/usb/typec_altmode.h +++ b/include/linux/usb/typec_altmode.h @@ -67,7 +67,7 @@ struct typec_altmode_ops {
int typec_altmode_enter(struct typec_altmode *altmode, u32 *vdo); int typec_altmode_exit(struct typec_altmode *altmode); -void typec_altmode_attention(struct typec_altmode *altmode, u32 vdo); +int typec_altmode_attention(struct typec_altmode *altmode, u32 vdo); int typec_altmode_vdm(struct typec_altmode *altmode, const u32 header, const u32 *vdo, int count); int typec_altmode_notify(struct typec_altmode *altmode, unsigned long conf,
base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
On 7/25/23 19:09, RD Babiera wrote:
Some usb hubs will negotiate DisplayPort Alt mode with the device but will then negotiate a data role swap after entering the alt mode. The data role swap causes the device to unregister all alt modes, however the usb hub will still send Attention messages even after failing to reregister the Alt Mode. type_altmode_attention currently does not verify whether or not a device's altmode partner exists, which results in a NULL pointer error when dereferencing the typec_altmode and typec_altmode_ops belonging to the altmode partner.
Is this theory or actually observed ?
This patch verifies the presence of a device's altmode partner before sending the Attention message to the Alt Mode driver. It also changes the return type from void to int so errors can be logged at the tcpm level.
Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes") Cc: stable@vger.kernel.org Signed-off-by: RD Babiera rdbabiera@google.com
drivers/usb/typec/bus.c | 12 ++++++++++-- drivers/usb/typec/tcpm/tcpm.c | 5 ++++- include/linux/usb/typec_altmode.h | 2 +- 3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c index fe5b9a2e61f5..2f1823e16b3b 100644 --- a/drivers/usb/typec/bus.c +++ b/drivers/usb/typec/bus.c @@ -183,12 +183,20 @@ EXPORT_SYMBOL_GPL(typec_altmode_exit);
- Notifies the partner of @adev about Attention command.
*/ -void typec_altmode_attention(struct typec_altmode *adev, u32 vdo) +int typec_altmode_attention(struct typec_altmode *adev, u32 vdo) {
- struct typec_altmode *pdev = &to_altmode(adev)->partner->adev;
- struct altmode *partner = to_altmode(adev)->partner;
- struct typec_altmode *pdev = &partner->adev;
This dereferences partner
- if (!partner || !pdev)
... and then checks if partner is NULL.
On top of that, pdev is not NULL even if partner is NULL because adev is not the first element of struct altmode.
In summary, this code and the check as implemented does not make sense. Maybe partner can be NULL, but pdev will never be NULL.
return -ENODEV;if (pdev->ops && pdev->ops->attention) pdev->ops->attention(pdev, vdo);
- else
return -EOPNOTSUPP;
So far this was explicitly permitted. Now it will log an error each time it is observed. I do not see the point of this log message; obviously it was not intended to be considered an error, and I do not understand why it should suddenly be one that is worth clogging the log.
Guenter
- return 0; } EXPORT_SYMBOL_GPL(typec_altmode_attention);
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 829d75ebab42..be37a662e54d 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -1791,6 +1791,7 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port, u32 p[PD_MAX_PAYLOAD]; u32 response[8] = { }; int i, rlen = 0;
- int ret;
for (i = 0; i < cnt; i++) p[i] = le32_to_cpu(payload[i]); @@ -1877,7 +1878,9 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port, } break; case ADEV_ATTENTION:
typec_altmode_attention(adev, p[1]);
ret = typec_altmode_attention(adev, p[1]);if (ret)tcpm_log(port, "altmode_attention failed ret:%d", ret); break;diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h index 350d49012659..28aeef8f9e7b 100644 --- a/include/linux/usb/typec_altmode.h +++ b/include/linux/usb/typec_altmode.h @@ -67,7 +67,7 @@ struct typec_altmode_ops { int typec_altmode_enter(struct typec_altmode *altmode, u32 *vdo); int typec_altmode_exit(struct typec_altmode *altmode); -void typec_altmode_attention(struct typec_altmode *altmode, u32 vdo); +int typec_altmode_attention(struct typec_altmode *altmode, u32 vdo); int typec_altmode_vdm(struct typec_altmode *altmode, const u32 header, const u32 *vdo, int count); int typec_altmode_notify(struct typec_altmode *altmode, unsigned long conf,
base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
On Tue, Jul 25, 2023 at 7:27 PM Guenter Roeck linux@roeck-us.net wrote:
Is this theory or actually observed ?
This is actually observed.
This dereferences partner
if (!partner || !pdev)... and then checks if partner is NULL.
On top of that, pdev is not NULL even if partner is NULL because adev is not the first element of struct altmode.
In summary, this code and the check as implemented does not make sense. Maybe partner can be NULL, but pdev will never be NULL.
After looking at it more carefully, I agree on the pdev assignment and check being odd. I'll follow how typec_altmode_vdm handles the NULL partner case and remove the NULL check on pdev.
return -ENODEV; if (pdev->ops && pdev->ops->attention) pdev->ops->attention(pdev, vdo);elsereturn -EOPNOTSUPP;So far this was explicitly permitted. Now it will log an error each time it is observed. I do not see the point of this log message; obviously it was not intended to be considered an error, and I do not understand why it should suddenly be one that is worth clogging the log.
My rationale for logging anything is that it will make it easier to identify port partners that send Attention messages regardless of whether or not the port registers the partner Alt Mode. You're right that this is not an error, so I will treat this case as a successful return moving forward. Then the only log generated will be for a port partner that sends Attention messages to a port that hasn't registered an altmode for it.
Thanks for the feedback, RD
linux-stable-mirror@lists.linaro.org
-
Guenter Roeck -
RD Babiera