On Thu, Oct 04, 2012 at 11:49:50PM +0100, Ben Hutchings wrote: > On Tue, Oct 02, 2012 at 12:54:10AM +0200, Willy Tarreau wrote: > > 2.6.32-longterm review patch. If anyone has any objections, please let me > > know. > > > > ------------------ > > > > From: Stephen M. Cameron <scame...@beardog.cce.hp.com> > > > > commit b0cf0b118c90477d1a6811f2cd2307f6a5578362 upstream. > > > > Delete code which sets SCSI status incorrectly as it's already been set > > correctly above this incorrect code. The bug was introduced in 2009 by > > commit b0e15f6db111 ("cciss: fix typo that causes scsi status to be > > lost.") > > That commit was in 2.6.33 and it changed the '<' to '<<'. It hasn't > been backported to 2.6.32.y.
But apparently the status was already incorrect before the first patch which tried to fix it first. I based myself on the comment from this patch which says "it's already been set correctly above this incorrect code". Above we find this : cmd->result = (DID_OK << 16); /* host byte */ cmd->result |= (COMMAND_COMPLETE << 8); /* msg byte */ /* cmd->result |= (GOOD < 1); */ /* status byte */ cmd->result |= (ei->ScsiStatus); If such a status is valid, then I conclude that both the following forms from the two previous versions are incorrect : - cmd->result |= (ei->ScsiStatus < 1); + cmd->result |= (ei->ScsiStatus << 1); Hence I preferred to backport the fix and have the same code as in mainline and newer versions which nobody has yet complained about. > > - cmd->result |= (ei->ScsiStatus < 1); > [...] > > Unless ei->ScsiStatus can be negative (it is declared as int, but > I don't think it's actually meant to be negative), this statement > is a no-op. Hmmm I disagree here, the code above does exactly the same thing as : cmd->result |= !ei->ScsiStatus; Which looks kind of strange to me after doing the exact opposite above, since the result is that the lowest bit of cmd->result will always be forced to 1 whatever ScsiStatus between 0 and 1. This might be what the original patch author meant with "fix typo that causes scsi status to be lost". So I'd rather keep this fix. Regards, Willy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/