Add a comment explaining why find_tt() will not return error even though
find_tt() is checking for NULL and other errors.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/usb/host/ehci-sched.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 6dfb242f9a4b..0f85aa9b2fb1 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -244,6 +244,12 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
/* FS/LS bus bandwidth */
if (tt_usecs) {
+ /*
+ * find_tt() will not return any error here as we have
+ * already called find_tt() before calling this function
+ * and checked for any error return. The previous call
+ * would have created the data structure.
+ */
tt = find_tt(qh->ps.udev);
if (sign > 0)
list_add_tail(&qh->ps.ps_list, &tt->ps_list);
@@ -1337,6 +1343,12 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
}
}
+ /*
+ * find_tt() will not return any error here as we have
+ * already called find_tt() before calling this function
+ * and checked for any error return. The previous call
+ * would have created the data structure.
+ */
tt = find_tt(stream->ps.udev);
if (sign > 0)
list_add_tail(&stream->ps.ps_list, &tt->ps_list);
--
2.11.0
On Sun, Oct 11, 2020 at 09:50:08PM +0100, Sudip Mukherjee wrote:
> Add a comment explaining why find_tt() will not return error even though
> find_tt() is checking for NULL and other errors.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/usb/host/ehci-sched.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index 6dfb242f9a4b..0f85aa9b2fb1 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -244,6 +244,12 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
>
> /* FS/LS bus bandwidth */
> if (tt_usecs) {
> + /*
> + * find_tt() will not return any error here as we have
> + * already called find_tt() before calling this function
> + * and checked for any error return. The previous call
> + * would have created the data structure.
> + */
> tt = find_tt(qh->ps.udev);
> if (sign > 0)
> list_add_tail(&qh->ps.ps_list, &tt->ps_list);
> @@ -1337,6 +1343,12 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
> }
> }
>
> + /*
> + * find_tt() will not return any error here as we have
> + * already called find_tt() before calling this function
> + * and checked for any error return. The previous call
> + * would have created the data structure.
> + */
> tt = find_tt(stream->ps.udev);
> if (sign > 0)
> list_add_tail(&stream->ps.ps_list, &tt->ps_list);
Acked-by: Alan Stern <[email protected]>
On Sun, 11 Oct 2020, Sudip Mukherjee wrote:
> Add a comment explaining why find_tt() will not return error even though
> find_tt() is checking for NULL and other errors.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
I get the intent of the comment but there is a large risk that somebody
could remove the find_tt() call before the calling the function and there
is no chance to then see later that the comment is actually wrong.
I guess you want to link this comment here to a code line above and
request anyone touching the line above to reconsider the comment then.
But there is no 'concept' for such kind of requests to changes and
comments.
So, the comment is true now, but might be true or wrong later.
I am wondering if such comment deserves to be included if we cannot check
its validity later...
I would prefer a simple tool that could check that your basic assumption
on the code is valid and if it the basic assumption is still valid,
just shut up any stupid tool that simply does not get that these calls
here cannot return any error.
We encountered this issue because of clang analyzer complaining, but it is
clear that it is a false positive of that (smart but) incomplete tool.
Do you intend to add comment for all false positives from all tools or are
we going to find a better solution than that?
I am happy to contribute to the smarter solution...
e.g., a cocci script that checks that the call is still there and then
always marking the related tool finding as false positive.
Lukas
> ---
> drivers/usb/host/ehci-sched.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index 6dfb242f9a4b..0f85aa9b2fb1 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -244,6 +244,12 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
>
> /* FS/LS bus bandwidth */
> if (tt_usecs) {
> + /*
> + * find_tt() will not return any error here as we have
> + * already called find_tt() before calling this function
> + * and checked for any error return. The previous call
> + * would have created the data structure.
> + */
> tt = find_tt(qh->ps.udev);
> if (sign > 0)
> list_add_tail(&qh->ps.ps_list, &tt->ps_list);
> @@ -1337,6 +1343,12 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
> }
> }
>
> + /*
> + * find_tt() will not return any error here as we have
> + * already called find_tt() before calling this function
> + * and checked for any error return. The previous call
> + * would have created the data structure.
> + */
> tt = find_tt(stream->ps.udev);
> if (sign > 0)
> list_add_tail(&stream->ps.ps_list, &tt->ps_list);
> --
> 2.11.0
>
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#76): https://lists.elisa.tech/g/linux-safety/message/76
> Mute This Topic: https://lists.elisa.tech/mt/77448152/1714638
> Group Owner: [email protected]
> Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [[email protected]]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>
>
On Mon, Oct 12, 2020 at 04:11:38PM +0200, Lukas Bulwahn wrote:
>
>
> On Sun, 11 Oct 2020, Sudip Mukherjee wrote:
>
> > Add a comment explaining why find_tt() will not return error even though
> > find_tt() is checking for NULL and other errors.
> >
> > Signed-off-by: Sudip Mukherjee <[email protected]>
>
> I get the intent of the comment but there is a large risk that somebody
> could remove the find_tt() call before the calling the function and there
> is no chance to then see later that the comment is actually wrong.
Why would anybody do that? Who would deliberately go change a driver in
a way that is obviously wrong and would break it? Especially when such
changes are likely to cause compile errors?
There are tons of changes one might make to any program that will leave
it syntactically valid but will cause it to fail. What's special about
removing the early calls to find_tt()?
> I guess you want to link this comment here to a code line above and
> request anyone touching the line above to reconsider the comment then.
That really seems unnecessary.
> But there is no 'concept' for such kind of requests to changes and
> comments.
>
> So, the comment is true now, but might be true or wrong later.
>
> I am wondering if such comment deserves to be included if we cannot check
> its validity later...
>
> I would prefer a simple tool that could check that your basic assumption
> on the code is valid and if it the basic assumption is still valid,
> just shut up any stupid tool that simply does not get that these calls
> here cannot return any error.
Real code contains so many assumptions, especially if you include ones
which are obvious to everybody, that such a tool seems impractical.
Alan Stern
On Mon, 12 Oct 2020, Alan Stern wrote:
> On Mon, Oct 12, 2020 at 04:11:38PM +0200, Lukas Bulwahn wrote:
> >
> >
> > On Sun, 11 Oct 2020, Sudip Mukherjee wrote:
> >
> > > Add a comment explaining why find_tt() will not return error even though
> > > find_tt() is checking for NULL and other errors.
> > >
> > > Signed-off-by: Sudip Mukherjee <[email protected]>
> >
> > I get the intent of the comment but there is a large risk that somebody
> > could remove the find_tt() call before the calling the function and there
> > is no chance to then see later that the comment is actually wrong.
>
> Why would anybody do that? Who would deliberately go change a driver in
> a way that is obviously wrong and would break it? Especially when such
> changes are likely to cause compile errors?
>
> There are tons of changes one might make to any program that will leave
> it syntactically valid but will cause it to fail. What's special about
> removing the early calls to find_tt()?
>
> > I guess you want to link this comment here to a code line above and
> > request anyone touching the line above to reconsider the comment then.
>
> That really seems unnecessary.
>
> > But there is no 'concept' for such kind of requests to changes and
> > comments.
> >
> > So, the comment is true now, but might be true or wrong later.
> >
> > I am wondering if such comment deserves to be included if we cannot check
> > its validity later...
> >
> > I would prefer a simple tool that could check that your basic assumption
> > on the code is valid and if it the basic assumption is still valid,
> > just shut up any stupid tool that simply does not get that these calls
> > here cannot return any error.
>
> Real code contains so many assumptions, especially if you include ones
> which are obvious to everybody, that such a tool seems impractical.
>
I fear that problem applies to all static code analysis tools I have seen;
at some point, the remaining findings are simply obviously wrong to
everybody but the tool does not get those assumptions and continues
complaining, making the tool seem impractical.
Alan, so would you be willing to take patches where _anyone_ simply adds
comments on what functions returns, depending on what this person might
consider just not obvious enough?
Or are you going to simply reject this 'added a comment' patch here?
I am not arguing either way, it is just that it is unclear to me what the
added value of the comment really is here.
And for the static analysis finding, we need to find a way to ignore this
finding without simply ignoring all findings or new findings that just
look very similar to the original finding, but which are valid.
Lukas
Hi Lukas,
On Mon, Oct 12, 2020 at 3:11 PM Lukas Bulwahn <[email protected]> wrote:
>
>
>
> On Sun, 11 Oct 2020, Sudip Mukherjee wrote:
>
> > Add a comment explaining why find_tt() will not return error even though
> > find_tt() is checking for NULL and other errors.
> >
> > Signed-off-by: Sudip Mukherjee <[email protected]>
>
> I get the intent of the comment but there is a large risk that somebody
> could remove the find_tt() call before the calling the function and there
> is no chance to then see later that the comment is actually wrong.
Removing the find_tt() will mean a major rework of the code.
>
> I guess you want to link this comment here to a code line above and
> request anyone touching the line above to reconsider the comment then.
>
> But there is no 'concept' for such kind of requests to changes and
> comments.
>
> So, the comment is true now, but might be true or wrong later.
If it is wrong later due to some code change then I guess someone will
need to send a patch to correct the comment.
>
> I am wondering if such comment deserves to be included if we cannot check
> its validity later...
I am failing to understand why will you not be able to check its
validity later. You just need to read the code to check it.
>
> I would prefer a simple tool that could check that your basic assumption
> on the code is valid and if it the basic assumption is still valid,
> just shut up any stupid tool that simply does not get that these calls
> here cannot return any error.
>
> We encountered this issue because of clang analyzer complaining, but it is
> clear that it is a false positive of that (smart but) incomplete tool.
I dont think it is a false positive. The error return value is not
checked and that is true. Only that it is not applicable because of
the way the coding is done.
>
> Do you intend to add comment for all false positives from all tools or are
> we going to find a better solution than that?
I think tools will always give you some false positives and you will
need to read the code to know if its false positive or not. I dont
think there is any tool yet which will only give true positives.
--
Regards
Sudip
On Mon, Oct 12, 2020 at 05:10:21PM +0200, Lukas Bulwahn wrote:
> And for the static analysis finding, we need to find a way to ignore this
> finding without simply ignoring all findings or new findings that just
> look very similar to the original finding, but which are valid.
Then I suggest you fix the tool that "flagged" this, surely this is not
the only thing it detected with a test like this, right?
What tool reported this?
thanks,
greg k-h
On Mon, Oct 12, 2020 at 05:10:21PM +0200, Lukas Bulwahn wrote:
>
>
> On Mon, 12 Oct 2020, Alan Stern wrote:
> > Real code contains so many assumptions, especially if you include ones
> > which are obvious to everybody, that such a tool seems impractical.
> >
>
> I fear that problem applies to all static code analysis tools I have seen;
> at some point, the remaining findings are simply obviously wrong to
> everybody but the tool does not get those assumptions and continues
> complaining, making the tool seem impractical.
Indeed, it is well known that the problem of finding all errors or bugs
by static code analysis is Turing complete.
> Alan, so would you be willing to take patches where _anyone_ simply adds
> comments on what functions returns, depending on what this person might
> consider just not obvious enough?
No. I would take such patches from anyone, but depending on what _I_
consider not obvious enough.
> Or are you going to simply reject this 'added a comment' patch here?
I have already accepted it. In fact, the patch was my suggestion in the
first place.
When I originally wrote this code, I was aware that it was somewhat
subtle, but at the time it didn't seem to warrant a comment or
explanation. Sudip's patch has changed my mind.
> I am not arguing either way, it is just that it is unclear to me what the
> added value of the comment really is here.
As with many other comments, its purpose is to explain a somewhat
obscure aspect of the code -- something which is there by design but
isn't immediately obvious to the reader. That is the added value.
> And for the static analysis finding, we need to find a way to ignore this
> finding without simply ignoring all findings or new findings that just
> look very similar to the original finding, but which are valid.
Agreed. In this case, the new comment does a pretty good job of telling
people using the tool that the finding is unjustified.
If you are suggesting some sort of special code annotation that the tool
would understand, I am open to that. But I'm not aware of any even
vaguely standard way of marking up a particular function call to
indicate it will not return an error.
Alan Stern