[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20200811194319.GB344152@rowland.harvard.edu>
Date: Tue, 11 Aug 2020 15:43:19 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Tom Rix <trix@...hat.com>
Cc: gregkh@...uxfoundation.org, acozzette@...hmc.edu,
linux-usb@...r.kernel.org, usb-storage@...ts.one-eyed-alien.net,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: realtek_cr: fix return check for dma functions
On Tue, Aug 11, 2020 at 11:54:28AM -0700, Tom Rix wrote:
>
> On 8/11/20 10:53 AM, Alan Stern wrote:
> >>> Instead of changing all these call sites, wouldn't it be a lot easier
> >>> just to change rts51x_read_mem() to make it always return a negative
> >>> value (such as -EIO) when there's an error?
> >>>
> >>> Alan Stern
> >> I thought about that but there was already existing (retval !=
> >> STATUS_SUCCESS) checks for these calls.
> > The only values that routine currently returns are
> > USB_STOR_TRANSPORT_ERROR, -EIO, and 0. None of the callers distinguish
> > between the first two values, so you can just change the first to the
> > second.
> >
> > Note that STATUS_SUCCESS is simply 0.
>
> Yes, i noted all of these already. My change is consistent with the
> existing correct checks. consistency is important. returning a neg
> value to reuse the exiting check should mean the STATUS_SUCCESS != 0
> checks are changed to neg check.
Do you mean the "retval == STATUS_SUCCESS" checks? Those checks would
end up doing exactly the same thing as they do now, since
USB_STOR_TRANSPORT_ERROR and -EIO are both different from 0.
Yes, it is true that consistency is important. But correctness is more
important than consistency.
> i can do this larger change if
> required.
Let me put it this way: Suppose you changed the USB_STOR_TRANSPORT_ERROR
in rts51x_read_mem() to -EIO, without changing anything else. Wouldn't
that fix the problem reported by the clang static analysis? If not, why
not?
Alan Stern
Powered by blists - more mailing lists