Hello Dan,
On Thu, Apr 11, 2024 at 04:20:22PM +0300, Dan Carpenter wrote:
> Hello Dmitry Baryshkov,
>
> Commit ad4bc68bef3e ("usb: typec: ucsi: extract code to read PD
> caps") from Mar 29, 2024 (linux-next), leads to the following Smatch
> static checker warning:
>
> drivers/usb/typec/ucsi/ucsi.c:689 ucsi_get_pd_caps()
> warn: passing zero to 'ERR_PTR'
>
> drivers/usb/typec/ucsi/ucsi.c
> 680 static struct usb_power_delivery_capabilities *ucsi_get_pd_caps(struct ucsi_connector *con,
> 681 enum typec_role role,
> 682 bool is_partner)
> 683 {
> 684 struct usb_power_delivery_capabilities_desc pd_caps;
> 685 int ret;
> 686
> 687 ret = ucsi_get_pdos(con, role, is_partner, pd_caps.pdo);
> 688 if (ret <= 0)
> --> 689 return ERR_PTR(ret);
>
> This is returns NULL if ucsi_get_pdos() returns zero. It's really hard
> to say if this is intentional or not... Originally, we treated error
> codes and zero the same but we didn't report errors back to the user,
> the code just continued silently. Now it's reporting errors but just
> continuing along if ucsi_get_pdos() returns zero.
The code is correct. The calling function checks that the result of
ucsi_get_pd_caps() is an ERR code and if that's not the case, assigns
PD capabilites to the storage at the connector. If ucsi_get_pdos()
returns 0, there are no PD objects, nothing to create capabilites for.
Thus the function correctly returns NULL.
If you think it is better to be explicit, I can send either an explicit
fixup `if (!ret) return NULL;` or just a comment that we should return
NULL if there are no PDOs.
>
> My instinct says that we should modify ucsi_get_pdos() to not return
> zero but I don't know the code...
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 8abde8db6bcf..427b7610a074 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -650,6 +650,8 @@ static int ucsi_get_pdos(struct ucsi_connector *con, enum typec_role role,
> return ret;
>
> num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> + if (num_pdos == 0)
> + return -EINVAL;
> if (num_pdos < UCSI_MAX_PDOS)
> return num_pdos;
>
> Or, if returning NULL is intentional then it would be nice to add a
> comment.
>
> 690
> 691 if (ret < PDO_MAX_OBJECTS)
> 692 pd_caps.pdo[ret] = 0;
> 693
> 694 pd_caps.role = role;
> 695
> 696 return usb_power_delivery_register_capabilities(is_partner ? con->partner_pd : con->pd,
> 697 &pd_caps);
> 698 }
>
> regards,
> dan carpenter
--
With best wishes
Dmitry
