Re: [PATCH][drivers/scsi/u14-34f.c] duplicate test 'SCpnt->sc_data_direction == DMA_FROM_DEVICE'
James Bottomley wrote: > On Mon, 2008-02-04 at 23:36 +0100, Roel Kluin wrote: >> Note the duplicate test 'SCpnt->sc_data_direction == DMA_FROM_DEVICE' >> - if (SCpnt->sc_data_direction == DMA_FROM_DEVICE) { >> + if (SCpnt->sc_data_direction == DMA_TO_DEVICE) { >>cpp->xdir = DTD_IN; >>return; >>} >> else if (SCpnt->sc_data_direction == DMA_FROM_DEVICE) { > > Well spotted, this is definitely a huge bug in the driver. > > However, I think on closer examination that DTD_IN actually means > transfer from device to host, so DMA_FROM_DEVICE is correct for that > case. It should be DMA_TO_DEVICE in the else if() piece. > > Could you correct and resend the patch and I'll apply it. Thanks for your review. --- Direction of data transfer 'DMA_FROM_DEVICE' was tested twice. DTD_OUT means transfer from host to device. This should occur when the direction of data transfer (sc_data_direction) is 'DMA_TO_DEVICE'. Signed-off-by: Roel Kluin <[EMAIL PROTECTED]> --- diff --git a/drivers/scsi/u14-34f.c b/drivers/scsi/u14-34f.c index 662c004..58d7eee 100644 --- a/drivers/scsi/u14-34f.c +++ b/drivers/scsi/u14-34f.c @@ -1215,9 +1215,9 @@ static void scsi_to_dev_dir(unsigned int i, unsigned int j) { if (SCpnt->sc_data_direction == DMA_FROM_DEVICE) { cpp->xdir = DTD_IN; return; } - else if (SCpnt->sc_data_direction == DMA_FROM_DEVICE) { + else if (SCpnt->sc_data_direction == DMA_TO_DEVICE) { cpp->xdir = DTD_OUT; return; } else if (SCpnt->sc_data_direction == DMA_NONE) { - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][drivers/scsi/u14-34f.c] duplicate test 'SCpnt->sc_data_direction == DMA_FROM_DEVICE'
On Mon, 2008-02-04 at 23:36 +0100, Roel Kluin wrote: > It should be like this I guess? this patch was not yet tested, please > confirm. > -- > Note the duplicate test 'SCpnt->sc_data_direction == DMA_FROM_DEVICE' > > from Documentation/DMA-API.txt: > DMA_TO_DEVICE = PCI_DMA_TODEVICE data is going from the > memory to the device > DMA_FROM_DEVICE = PCI_DMA_FROMDEVICEdata is coming from > the device to the > > Signed-off-by: Roel Kluin <[EMAIL PROTECTED]> > --- > diff --git a/drivers/scsi/u14-34f.c b/drivers/scsi/u14-34f.c > index 662c004..1e704f9 100644 > --- a/drivers/scsi/u14-34f.c > +++ b/drivers/scsi/u14-34f.c > @@ -1208,15 +1208,15 @@ static void scsi_to_dev_dir(unsigned int i, unsigned > int j) { >}; > > struct mscp *cpp; > struct scsi_cmnd *SCpnt; > > cpp = &HD(j)->cp[i]; SCpnt = cpp->SCpnt; > > - if (SCpnt->sc_data_direction == DMA_FROM_DEVICE) { > + if (SCpnt->sc_data_direction == DMA_TO_DEVICE) { >cpp->xdir = DTD_IN; >return; >} > else if (SCpnt->sc_data_direction == DMA_FROM_DEVICE) { >cpp->xdir = DTD_OUT; >return; >} Well spotted, this is definitely a huge bug in the driver. However, I think on closer examination that DTD_IN actually means transfer from device to host, so DMA_FROM_DEVICE is correct for that case. It should be DMA_TO_DEVICE in the else if() piece. Could you correct and resend the patch and I'll apply it. Thanks, James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][drivers/scsi/u14-34f.c] duplicate test 'SCpnt->sc_data_direction == DMA_FROM_DEVICE'
[EMAIL PROTECTED] wrote: > Good to know that somebody still uses the Ultrastor 14f board :). > Yes, this typo was introduced by somebody doing massive editing to all > scsi drivers long ago. > Cheers, > --db Actually, I do not own a Ultrastor 14f board. I found this by searching for if (test) ... else if (exactly same test) ... Thanks, Roel - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH][drivers/scsi/u14-34f.c] duplicate test 'SCpnt->sc_data_direction == DMA_FROM_DEVICE'
Good to know that somebody still uses the Ultrastor 14f board :). Yes, this typo was introduced by somebody doing massive editing to all scsi drivers long ago. Cheers, --db -Original Message- From: Roel Kluin [mailto:[EMAIL PROTECTED] Sent: Monday, February 04, 2008 11:37 PM To: Ballabio, Dario Cc: linux-scsi@vger.kernel.org; lkml Subject: [PATCH][drivers/scsi/u14-34f.c] duplicate test 'SCpnt->sc_data_direction == DMA_FROM_DEVICE' It should be like this I guess? this patch was not yet tested, please confirm. -- Note the duplicate test 'SCpnt->sc_data_direction == DMA_FROM_DEVICE' from Documentation/DMA-API.txt: DMA_TO_DEVICE = PCI_DMA_TODEVICE data is going from the memory to the device DMA_FROM_DEVICE = PCI_DMA_FROMDEVICEdata is coming from the device to the Signed-off-by: Roel Kluin <[EMAIL PROTECTED]> --- diff --git a/drivers/scsi/u14-34f.c b/drivers/scsi/u14-34f.c index 662c004..1e704f9 100644 --- a/drivers/scsi/u14-34f.c +++ b/drivers/scsi/u14-34f.c @@ -1208,15 +1208,15 @@ static void scsi_to_dev_dir(unsigned int i, unsigned int j) { }; struct mscp *cpp; struct scsi_cmnd *SCpnt; cpp = &HD(j)->cp[i]; SCpnt = cpp->SCpnt; - if (SCpnt->sc_data_direction == DMA_FROM_DEVICE) { + if (SCpnt->sc_data_direction == DMA_TO_DEVICE) { cpp->xdir = DTD_IN; return; } else if (SCpnt->sc_data_direction == DMA_FROM_DEVICE) { cpp->xdir = DTD_OUT; return; } - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html