Re: [PATCH] [SCSI] arcmsr: &/| confusion in arcmsr_build_ccb()

[Matthew Wilcox <matthew@wil.cx>]

On Sat, Jan 01, 2011 at 05:45:11PM +0100, roel kluin wrote:
> The WRITE_{6,10,12} are defined numbers, so the branch was always taken.

You're right, but even this will basically always set the write flag,
since some bits will be set the same, even for reads.  Eg:

#define READ_10               0x28
#define WRITE_10              0x2a

It should instead be:

> +	if (pcmd->cmnd[0] == WRITE_6 || pcmd->cmnd[0] == WRITE_10 ||
> +			pcmd->cmnd[0] == WRITE_12 ){

That will of course miss other commands which do writes, such as UNMAP
and WRITE_32.  So we should do it properly:

	if (pcmd->sc_data_direction == DMA_TO_DEVICE)
		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;

I don't have an Areca card to test with.

> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
>  drivers/scsi/arcmsr/arcmsr_hba.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> See include/scsi/scsi.h:58-123 for
> #define WRITE_6               0x0a
> #define WRITE_10              0x2a
> #define WRITE_12              0xaa
> 
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 17e3df4..a6e5222 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -1171,7 +1171,8 @@ static int arcmsr_build_ccb(struct AdapterControlBlock *acb,
>  	arcmsr_cdb->msgPages = arccdbsize/0x100 + (arccdbsize % 0x100 ? 1 : 0);
>  	if ( arccdbsize > 256)
>  		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_SGL_BSIZE;
> -	if (pcmd->cmnd[0]|WRITE_6 || pcmd->cmnd[0]|WRITE_10 || pcmd->cmnd[0]|WRITE_12 ){
> +	if (pcmd->cmnd[0] & WRITE_6 || pcmd->cmnd[0] & WRITE_10 ||
> +			pcmd->cmnd[0] & WRITE_12 ){
>  		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;
>  	}
>  	ccb->arc_cdb_size = arccdbsize;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."