usb: typec: Fix arg check for usb_power_delivery_unregister_capabilities
Subsystems (3)
PATCH 1/1 usb: typec: Fix arg check for usb_power_delivery_unregister_capabilities
usb_power_delivery_register_capabilities() returns ERR_PTR in case of failure. usb_power_delivery_unregister_capabilities() we only check argument ("cap") for NULL. A more robust check would be checking for ERR_PTR as well.
--- drivers/usb/typec/pd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/typec/pd.c +1 -1
@@ -519,7 +519,7 @@ EXPORT_SYMBOL_GPL(usb_power_delivery_register_capabilities);*/ void usb_power_delivery_unregister_capabilities (struct usb_power_delivery_capabilities *cap) { -if (!cap) +if (IS_ERR_OR_NULL (cap))return ;device_for_each_child (&cap->dev,NULL , remove_pdo);
base-commit: 68d4209158f43a558c5553ea95ab0c8975eab18c
Signature · 1 line
2.46.0.792.g87dc391469-goog
Quoted 4 lines · Amit Sunil Dhamne
usb_power_delivery_register_capabilities()returnsERR_PTRin case of failure.usb_power_delivery_unregister_capabilities()we only check argument ("cap") for NULL. A more robust check would be checking forERR_PTRas well.
No. The calling drivers are not supposed to pass ERR_PTR to usb_power_delivery_unregister_capabilities(). If you check the TCPM and UCSI driver code, they check return value of the register function before saving it internally.
Quoted 26 lines
Fixes 662a60102c12 ("usb: typec: Separate USB Power Delivery from USB Type-C")Signed-off-by Amit Sunil Dhamne <[email protected]>Reviewed-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/pd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/usb/typec/pd.c +1
-1@@ -519,7 +519,7 @@ EXPORT_SYMBOL_GPL(usb_power_delivery_register_capabilities);*/ void usb_power_delivery_unregister_capabilities (struct usb_power_delivery_capabilities *cap) { -if (!cap) +if (IS_ERR_OR_NULL (cap))return ;device_for_each_child (&cap->dev,NULL , remove_pdo);base-commit:
68d4209158f43a558c5553ea95ab0c8975eab18cSignature · 1 line
2.46.0.792.g87dc391469-goog
Signature · 2 lines
With best wishes Dmitry
Greg KH
maintainerQuoted 18 lines · Amit Sunil Dhamne
usb_power_delivery_register_capabilities()returnsERR_PTRin case of failure.usb_power_delivery_unregister_capabilities()we only check argument ("cap") for NULL. A more robust check would be checking forERR_PTRas well.Fixes 662a60102c12 ("usb: typec: Separate USB Power Delivery from USB Type-C")Signed-off-by Amit Sunil Dhamne <[email protected]>Reviewed-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/pd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/usb/typec/pd.c +0
-0
drivers/usb/typec/pd.c EXPORT_SYMBOL_GPL(usb_power_delivery_register_capabilities); · line 522
Quoted 5 lines
drivers/usb/typec/pd.c +1
-1@@ -519,7 +519,7 @@ EXPORT_SYMBOL_GPL(usb_power_delivery_register_capabilities);*/ void usb_power_delivery_unregister_capabilities (struct usb_power_delivery_capabilities *cap) { -if (!cap) +if (IS_ERR_OR_NULL (cap))
This feels like there's a wrong caller, why would this be called with an error value in the first place? Why not fix that? And why would this be called with NULL as well in the first place?
thanks,
greg k-h
Quoted 14 lines · Greg KH
Quoted 13 lines · Amit Sunil Dhamne
usb_power_delivery_register_capabilities()returnsERR_PTRin case of failure.usb_power_delivery_unregister_capabilities()we only check argument ("cap") for NULL. A more robust check would be checking forERR_PTRas well.Fixes 662a60102c12 ("usb: typec: Separate USB Power Delivery from USB Type-C")Signed-off-by Amit Sunil Dhamne <[email protected]>Reviewed-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/pd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 14 lines
drivers/usb/typec/pd.c EXPORT_SYMBOL_GPL(usb_power_delivery_register_capabilities); · line 522
Quoted 10 lines
drivers/usb/typec/pd.c +1
-1@@ -519,7 +519,7 @@ EXPORT_SYMBOL_GPL(usb_power_delivery_register_capabilities);*/ void usb_power_delivery_unregister_capabilities (struct usb_power_delivery_capabilities *cap) { -if (!cap) +if (IS_ERR_OR_NULL (cap))This feels like there's a wrong caller, why would this be called with an error value in the first place? Why not fix that? And why would this be called with NULL as well in the first place?
I think passing NULL matches the rest of the kernel, it removes unnecessary if(!NULL) statements from the caller side.
Signature · 2 lines
With best wishes Dmitry
Hi Greg, Dmitry,
Thanks for the review!
Quoted 28 lines · Dmitry Baryshkov
Quoted 14 lines · Greg KH
Quoted 13 lines · Amit Sunil Dhamne
usb_power_delivery_register_capabilities()returnsERR_PTRin case of failure.usb_power_delivery_unregister_capabilities()we only check argument ("cap") for NULL. A more robust check would be checking forERR_PTRas well.Fixes 662a60102c12 ("usb: typec: Separate USB Power Delivery from USB Type-C")Signed-off-by Amit Sunil Dhamne <[email protected]>Reviewed-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/pd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Quoted 13 lines
drivers/usb/typec/pd.c EXPORT_SYMBOL_GPL(usb_power_delivery_register_capabilities); · line 522
Quoted 10 lines
drivers/usb/typec/pd.c +1
-1@@ -519,7 +519,7 @@ EXPORT_SYMBOL_GPL(usb_power_delivery_register_capabilities);*/ void usb_power_delivery_unregister_capabilities (struct usb_power_delivery_capabilities *cap) { -if (!cap) +if (IS_ERR_OR_NULL (cap))This feels like there's a wrong caller, why would this be called with an error value in the first place? Why not fix that? And why would this be called with NULL as well in the first place?
Quoted 3 lines
I think passing NULL matches the rest of the kernel, it removes unnecessary if(!NULL) statements from the caller side.
The reason for this patch was just to be a little more defensive in case things slip through cracks and be consistent with the rest of the PD class. For example usb_power_delivery_unregister() & usb_power_delivery_unlink_device() has similar arg checks.
Regards,
Amit