From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Date: Thu, 19 Feb 2009 13:01:42 -0800
> On Wed, 18 Feb 2009 14:39:16 -0800 (PST)
> David Miller <davem@xxxxxxxxxxxxx> wrote:
>
> > From: Roel Kluin <roel.kluin@xxxxxxxxx>
> > Date: Wed, 18 Feb 2009 13:48:11 +0100
> >
> > > Add missing parentheses
> > >
> > > Signed-off-by: Roel Kluin <roel.kluin@xxxxxxxxx>
> >
> > So what bug does this cause?
> >
> > > #define QLOGICPTI_REQ_QUEUE_LEN 255 /* must be power of two - 1 */
> > > -#define QLOGICPTI_MAX_SG(ql) (4 + ((ql) > 0) ? 7*((ql) - 1) : 0)
> > > +#define QLOGICPTI_MAX_SG(ql) (4 + (((ql) > 0) ? 7*((ql) - 1) : 0))
> >
> > The "ql > 0" bids to the "?" operator, so this expression
> > does the right thing as-is as far as I can tell.
> >
> > Do you think that "4 + ((ql > 0)" binds to "?", I don't think
> > it does.
>
> The code is crap, and buggy if passed an expression with side-effects.
Thank got it never is called that way :-)
> Now, 4 plus a boolean can only ever evaluate to 4 or 5, so this
> function can never return zero. So yeah, I assume that this was meant:
>
> static inline int qlogicpti_max_sg(int ql)
> {
> if (ql > 0)
> return 7 * (ql - 1) + 4;
> else
> return 4;
> }
>
I think this doesn't even match the intention, and I wrote
the code so...
We want to return 0 if the queue has no free entries.
But if it does have free slots, we want "4 + xxx"
These problems probably explain the mysterious workaround
in update_can_queue().
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
