[PATCH v1] usb: typec: Fix arg check for usb_power_delivery_unregister_capabilities

[PATCH v1] usb: typec: Fix arg check for usb_power_delivery_unregister_capabilities

[Amit Sunil Dhamne]

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.

Cc: stable@vger.kernel.org
Fixes: 662a60102c12 ("usb: typec: Separate USB Power Delivery from USB Type-C")
Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/pd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/pd.c b/drivers/usb/typec/pd.c
index d78c04a421bc..761fe4dddf1b 100644
--- a/drivers/usb/typec/pd.c
+++ b/drivers/usb/typec/pd.c
@@ -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
-- 
2.46.0.792.g87dc391469-goog

Re: [PATCH v1] usb: typec: Fix arg check for usb_power_delivery_unregister_capabilities

[Greg KH]

On Thu, Sep 19, 2024 at 12:58:12AM -0700, Amit Sunil Dhamne wrote:
> 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.
> 
> Cc: stable@vger.kernel.org
> Fixes: 662a60102c12 ("usb: typec: Separate USB Power Delivery from USB Type-C")
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/pd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/pd.c b/drivers/usb/typec/pd.c
> index d78c04a421bc..761fe4dddf1b 100644
> --- a/drivers/usb/typec/pd.c
> +++ b/drivers/usb/typec/pd.c
> @@ -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

Re: [PATCH v1] usb: typec: Fix arg check for usb_power_delivery_unregister_capabilities

[Dmitry Baryshkov]

On Thu, Sep 19, 2024 at 10:11:37AM GMT, Greg KH wrote:
> On Thu, Sep 19, 2024 at 12:58:12AM -0700, Amit Sunil Dhamne wrote:
> > 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.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 662a60102c12 ("usb: typec: Separate USB Power Delivery from USB Type-C")
> > Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> > Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  drivers/usb/typec/pd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/typec/pd.c b/drivers/usb/typec/pd.c
> > index d78c04a421bc..761fe4dddf1b 100644
> > --- a/drivers/usb/typec/pd.c
> > +++ b/drivers/usb/typec/pd.c
> > @@ -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.

-- 
With best wishes
Dmitry

Re: [PATCH v1] usb: typec: Fix arg check for usb_power_delivery_unregister_capabilities

[Amit Sunil Dhamne]

Hi Greg, Dmitry,

Thanks for the review!

On 9/19/24 3:03 AM, Dmitry Baryshkov wrote:
> On Thu, Sep 19, 2024 at 10:11:37AM GMT, Greg KH wrote:
>> On Thu, Sep 19, 2024 at 12:58:12AM -0700, Amit Sunil Dhamne wrote:
>>> 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.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 662a60102c12 ("usb: typec: Separate USB Power Delivery from USB Type-C")
>>> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
>>> Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>
>>> ---
>>>   drivers/usb/typec/pd.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/typec/pd.c b/drivers/usb/typec/pd.c
>>> index d78c04a421bc..761fe4dddf1b 100644
>>> --- a/drivers/usb/typec/pd.c
>>> +++ b/drivers/usb/typec/pd.c
>>> @@ -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.
>
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

Re: [PATCH v1] usb: typec: Fix arg check for usb_power_delivery_unregister_capabilities

[Dmitry Baryshkov]

On Thu, Sep 19, 2024 at 12:58:12AM GMT, Amit Sunil Dhamne wrote:
> 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.

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.

> Cc: stable@vger.kernel.org
> Fixes: 662a60102c12 ("usb: typec: Separate USB Power Delivery from USB Type-C")
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/pd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/pd.c b/drivers/usb/typec/pd.c
> index d78c04a421bc..761fe4dddf1b 100644
> --- a/drivers/usb/typec/pd.c
> +++ b/drivers/usb/typec/pd.c
> @@ -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
> -- 
> 2.46.0.792.g87dc391469-goog
> 

-- 
With best wishes
Dmitry