On Thu, Apr 11, 2024 at 11:25:50PM +0300, Dmitry Baryshkov wrote:
> 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.
>
Just add a comment. This static checker rule is going to have false
positives, and I did think it might be a false positive, but it just
wasn't totally obvious.
regards,
dan carpenter
