* [bug report] usb: typec: ucsi: extract code to read PD caps
@ 2024-04-11 13:20 Dan Carpenter
2024-04-11 20:25 ` Dmitry Baryshkov
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2024-04-11 13:20 UTC (permalink / raw
To: dmitry.baryshkov; +Cc: linux-usb
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.
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [bug report] usb: typec: ucsi: extract code to read PD caps
2024-04-11 13:20 [bug report] usb: typec: ucsi: extract code to read PD caps Dan Carpenter
@ 2024-04-11 20:25 ` Dmitry Baryshkov
2024-04-14 11:52 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Baryshkov @ 2024-04-11 20:25 UTC (permalink / raw
To: Dan Carpenter; +Cc: linux-usb
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] usb: typec: ucsi: extract code to read PD caps
2024-04-11 20:25 ` Dmitry Baryshkov
@ 2024-04-14 11:52 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2024-04-14 11:52 UTC (permalink / raw
To: Dmitry Baryshkov; +Cc: linux-usb
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-14 11:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-11 13:20 [bug report] usb: typec: ucsi: extract code to read PD caps Dan Carpenter
2024-04-11 20:25 ` Dmitry Baryshkov
2024-04-14 11:52 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.