On Tue, Aug 29, 2023 at 11:42:07PM +0200, Oliver Neukum wrote:
>
>
> On 29.08.23 20:49, Alan Stern wrote:
> > On Tue, Aug 29, 2023 at 08:14:05PM +0200, Oliver Neukum wrote:
> > > Hi Alan,
> > >
> > > as you did something on this driver, doesn't
> > > this condition:
> > >
> > > (status[0] & 0x80) ||
> > > ((status[0] & 0x1F) == 0x10) || ((status[1] & 0x01) == 0)
> > >
> > > look odd to you? Especially the parentheses?
> >
> > (The actual text in my copy of the file is:
>
> Yes, I rearranged it by the parantheses.
>
> > if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)
> > || ((status[1] & 0x01) == 0)) {
> >
> > This probably doesn't affect your point...)
> >
> > Certainly the layout is a little peculiar, and the extra parentheses
> > don't help any. But they don't really hurt, either, and the meaning is
> > clear. It doesn't look obviously wrong.
>
> Ok, then this is just me. THe parantheses would make perfect sense
> if the actual intent were:
>
> (status[0] & 0x80) ||
> ((status[0] & 0x1F) == 0x10) && ((status[1] & 0x01) == 0)
In fact, if I were writing that expression, I would do:
(status[0] & 0x80) ||
((status[0] & 0x1F) == 0x10 && (status[1] & 0x01) == 0)
simply because I don't like relying on the relative precedence of || and
&&. That's just my own neurosis. (On the other hand I have no qualms
about the relative precedence of && and ==, because that combination of
operators gets used all the time. Maybe Daniel Drake preferred not to
rely on it?)
For all we know, this is what the code _should_ be. Without any
documentation on the meaning of the status bits, there's no way to tell.
> > Those two lines go back to the original version of the driver, added in
> > 2005 by commit e80b0fade09e ("[PATCH] USB Storage: add alauda support"),
> > written by Daniel Drake and edited by Matt Dharm. So it's been around
> > for quite a while and there may not be many devices left that need this
> > driver.
>
> Yes, I know. Hence my question.
>
> > Did you want to change it?
>
> Nope. I just looked through the log and saw your patch for the
> failure of the transfer and the subsequent test looked
> messed up.
At this point, I don't think it matters much.
Alan Stern
