Re: [linux-safety] [PATCH] usb: host: ehci-sched: add comment about find_tt() not returning error

["Lukas Bulwahn" <lukas.bulwahn@gmail.com>]

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 <sudipm.mukherjee@gmail.com>

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
> 
> 
> 
> 
> 
> 
>