fix up request buffer reference in various scsi drivers
Subsystems (12)
PATCH 1/1 fix up request buffer reference in various scsi drivers
Various scsi drivers use scsi_cmnd.buffer and scsi_cmnd.bufflen in their queuecommand functions. Those fields are internal storage for the midlayer only and are used to restore the original payload after request_buffer and request_bufflen have been overwritten for EH. Using the buffer and bufflen fields means they do very broken things in error handling.
drivers/block/cciss_scsi.c +3 -3
@@ -578,7 +578,7 @@if (cmd->use_sg) {pci_unmap_sg (ctlr->pdev, - cmd->buffer, cmd->use_sg, + cmd->request_buffer, cmd->use_sg, cmd->sc_data_direction); }else if (cmd->request_bufflen) {
@@ -1210,7 +1210,7 @@ struct scsi_cmnd *cmd) {unsigned int use_sg, nsegs=0 , len; -struct scatterlist *scatter = (struct scatterlist *) cmd->buffer; +struct scatterlist *scatter = (struct scatterlist *) cmd->request_buffer;__u64 addr64;/* is it just one virtual address? */
@@ -1232,7 +1232,7 @@ }/* else, must be a list of virtual addresses.... */ else if (cmd->use_sg <=MAXSGENTRIES ) {/* not too many addrs? */ - use_sg =pci_map_sg (pdev, cmd->buffer, cmd->use_sg, + use_sg =pci_map_sg (pdev, cmd->request_buffer, cmd->use_sg, cmd->sc_data_direction);for (nsegs=0 ; nsegs < use_sg; nsegs++) {
drivers/scsi/3w-9xxx.c +1 -1
@@ -1388,7 +1388,7 @@if (cmd->use_sg ==0 ) goto out; - use_sg =pci_map_sg (pdev, cmd->buffer, cmd->use_sg,DMA_BIDIRECTIONAL ); + use_sg =pci_map_sg (pdev, cmd->request_buffer, cmd->use_sg,DMA_BIDIRECTIONAL );if (use_sg ==0 ) {TW_PRINTK (tw_dev->host,TW_DRIVER ,0x1c ,"Failed to map scatter gather list" );
drivers/scsi/3w-xxxx.c +1 -1
@@ -1286,7 +1286,7 @@if (cmd->use_sg ==0 )return 0 ; - use_sg =pci_map_sg (pdev, cmd->buffer, cmd->use_sg,DMA_BIDIRECTIONAL ); + use_sg =pci_map_sg (pdev, cmd->request_buffer, cmd->use_sg,DMA_BIDIRECTIONAL );if (use_sg ==0 ) {printk (KERN_WARNING "3w-xxxx: tw_map_scsi_sg_data(): pci_map_sg() failed.\n" );
drivers/scsi/NCR5380.c +1 -1
@@ -296,7 +296,7 @@*/ if (cmd->use_sg) { - cmd->SCp.buffer = (struct scatterlist *) cmd->buffer; + cmd->SCp.buffer = (struct scatterlist *) cmd->request_buffer; cmd->SCp.buffers_residual = cmd->use_sg -1 ; cmd->SCp.ptr =page_address (cmd->SCp.buffer->page)+ cmd->SCp.buffer->offset;
drivers/scsi/aacraid/aachba.c +2 -2
@@ -961,7 +961,7 @@if (scsicmd->use_sg)pci_unmap_sg (dev->pdev, - (struct scatterlist *)scsicmd->buffer, + (struct scatterlist *)scsicmd->request_buffer, scsicmd->use_sg, scsicmd->sc_data_direction);else if (scsicmd->request_bufflen)
@@ -1919,7 +1919,7 @@if (scsicmd->use_sg)pci_unmap_sg (dev->pdev, - (struct scatterlist *)scsicmd->buffer, + (struct scatterlist *)scsicmd->request_buffer, scsicmd->use_sg, scsicmd->sc_data_direction);else if (scsicmd->request_bufflen)
drivers/scsi/atp870u.c +1 -1
@@ -473,7 +473,7 @@*/ if (workreq->use_sg) {pci_unmap_sg (dev->pdev, - (struct scatterlist *)workreq->buffer, + (struct scatterlist *)workreq->request_buffer, workreq->use_sg, workreq->sc_data_direction); } else if (workreq->request_bufflen &&
drivers/scsi/gdth.c +1 -1
@@ -2542,7 +2542,7 @@gdth_ha_str *ha;char *address; - cpcount = count<=(ushort )scp->bufflen ? count:(ushort )scp->bufflen; + cpcount = count<=(ushort )scp->request_bufflen ? count:(ushort )scp->request_bufflen; ha =HADATA (gdth_ctr_tab[hanum]);if (scp->use_sg) {
drivers/scsi/in2000.c +1 -1
@@ -370,7 +370,7 @@*/ if (cmd->use_sg) { - cmd->SCp.buffer = (struct scatterlist *) cmd->buffer; + cmd->SCp.buffer = (struct scatterlist *) cmd->request_buffer; cmd->SCp.buffers_residual = cmd->use_sg -1 ; cmd->SCp.ptr = (char *)page_address (cmd->SCp.buffer->page) + cmd->SCp.buffer->offset; cmd->SCp.this_residual = cmd->SCp.buffer->length;
drivers/scsi/ips.c +1 -1
@@ -4364,7 +4364,7 @@METHOD_TRACE ("ips_rdcap" ,1 ); -if (scb->scsi_cmd->bufflen <8 ) +if (scb->scsi_cmd->request_bufflen <8 )return (0 ); cap.lba =
drivers/scsi/libata-scsi.c +2 -2
@@ -2310,7 +2310,7 @@#endif } - qc->nbytes = cmd->bufflen; + qc->nbytes = cmd->request_bufflen;return 0 ; }
@@ -2500,7 +2500,7 @@* TODO: find out if we need to do more here to * cover scatter/gather case. */ - qc->nsect = cmd->bufflen /ATA_SECT_SIZE ; + qc->nsect = cmd->request_bufflen /ATA_SECT_SIZE ;return 0 ;
drivers/scsi/megaraid.c +2 -2
@@ -524,7 +524,7 @@* filter the internal and ioctl commands */ if ((cmd->cmnd[0 ] ==MEGA_INTERNAL_CMD )) { -return cmd->buffer; +return cmd->request_buffer; }
@@ -4493,7 +4493,7 @@ scmd->device = sdev; scmd->device->host = adapter->host; - scmd->buffer = (void *)scb; + scmd->request_buffer = (void *)scb; scmd->cmnd[0 ] =MEGA_INTERNAL_CMD ; scb->state |=SCB_ACTIVE ;
drivers/scsi/ncr53c8xx.c +3 -3
@@ -529,7 +529,7 @@ {switch (cmd->__data_mapped) {case 2 : -dma_unmap_sg (dev, cmd->buffer, cmd->use_sg, +dma_unmap_sg (dev, cmd->request_buffer, cmd->use_sg, cmd->sc_data_direction);break ;case 1 :
@@ -564,7 +564,7 @@if (cmd->use_sg ==0 )return 0 ; - use_sg =dma_map_sg (dev, cmd->buffer, cmd->use_sg, + use_sg =dma_map_sg (dev, cmd->request_buffer, cmd->use_sg, cmd->sc_data_direction); cmd->__data_mapped =2 ; cmd->__data_mapping = use_sg;
@@ -7697,7 +7697,7 @@if (!use_sg) segment =ncr_scatter_no_sglist (np, cp, cmd);else if ((use_sg =map_scsi_sg_data (np, cmd)) >0 ) { -struct scatterlist *scatter = (struct scatterlist *)cmd->buffer; +struct scatterlist *scatter = (struct scatterlist *)cmd->request_buffer;struct scr_tblmove *data;if (use_sg >MAX_SCATTER ) {
drivers/scsi/nsp32.c +1 -1
@@ -1636,7 +1636,7 @@if (SCpnt->use_sg) {pci_unmap_sg (data->Pci, - (struct scatterlist *)SCpnt->buffer, + (struct scatterlist *)SCpnt->request_buffer, SCpnt->use_sg, SCpnt->sc_data_direction); }else { pci_unmap_single(data->Pci,
drivers/scsi/sd.c +1 -1
@@ -891,7 +891,7 @@static void sd_rw_intr (struct scsi_cmnd * SCpnt) {int result = SCpnt->result; -int this_count = SCpnt->bufflen; +int this_count = SCpnt->request_bufflen;int good_bytes = (result ==0 ? this_count :0 );sector_t block_sectors =1 ;u64 first_err_block;
drivers/scsi/sr.c +1 -1
@@ -217,7 +217,7 @@static void rw_intr (struct scsi_cmnd * SCpnt) {int result = SCpnt->result; -int this_count = SCpnt->bufflen; +int this_count = SCpnt->request_bufflen;int good_bytes = (result ==0 ? this_count :0 );int block_sectors =0 ;long error_sector;
drivers/scsi/sym53c8xx_2/sym_glue.c +3 -3
@@ -156,7 +156,7 @@switch (SYM_UCMD_PTR (cmd)->data_mapped) {case 2 : -pci_unmap_sg (pdev, cmd->buffer, cmd->use_sg, dma_dir); +pci_unmap_sg (pdev, cmd->request_buffer, cmd->use_sg, dma_dir);break ;case 1 : pci_unmap_single(pdev, SYM_UCMD_PTR(cmd)->data_mapping,
@@ -186,7 +186,7 @@int use_sg;int dma_dir = cmd->sc_data_direction; - use_sg =pci_map_sg (pdev, cmd->buffer, cmd->use_sg, dma_dir); + use_sg =pci_map_sg (pdev, cmd->request_buffer, cmd->use_sg, dma_dir);if (use_sg >0 ) {SYM_UCMD_PTR (cmd)->data_mapped =2 ;SYM_UCMD_PTR (cmd)->data_mapping = use_sg;
@@ -376,7 +376,7 @@if (!use_sg) segment =sym_scatter_no_sglist (np, cp, cmd);else if ((use_sg =map_scsi_sg_data (np, cmd)) >0 ) { -struct scatterlist *scatter = (struct scatterlist *)cmd->buffer; +struct scatterlist *scatter = (struct scatterlist *)cmd->request_buffer;struct sym_tcb *tp = &np->target[cp->target];struct sym_tblmove *data;
drivers/usb/image/microtek.c +5 -5
@@ -513,7 +513,7 @@mts_transfer_cleanup (transfer); } - sg = context->srb->buffer; + sg = context->srb->request_buffer; context->fragment++; mts_int_submit_urb(transfer, context->data_pipe,
@@ -549,19 +549,19 @@ desc->context.fragment =0 ;if (!srb->use_sg) { -if ( !srb->bufflen ){ +if ( !srb->request_bufflen ){ desc->context.data =NULL ; desc->context.data_length =0 ;return ; }else { - desc->context.data = srb->buffer; - desc->context.data_length = srb->bufflen; + desc->context.data = srb->request_buffer; + desc->context.data_length = srb->request_bufflen;MTS_DEBUG ("length = %d or %d\n" , srb->request_bufflen, srb->bufflen); } }else {MTS_DEBUG ("Using scatter/gather\n" ); - sg = srb->buffer; + sg = srb->request_buffer; desc->context.data =page_address (sg[0 ].page) + sg[0 ].offset; desc->context.data_length = sg[0 ].length; }
Quoted 6 lines · Christoph Hellwig
Various scsi drivers use scsi_cmnd.buffer and scsi_cmnd.bufflen in their queuecommand functions. Those fields are internal storage for the midlayer only and are used to restore the original payload after
request_bufferandrequest_bufflenhave been overwritten for EH. Using the buffer and bufflen fields means they do very broken things in error handling.
False statement, for libata.
Please CC the author (me) or linux-ide, at least, when you touch libata.
Jeff
Quoted 9 lines · Jeff Garzik
Quoted 6 lines · Christoph Hellwig
Various scsi drivers use scsi_cmnd.buffer and scsi_cmnd.bufflen in their queuecommand functions. Those fields are internal storage for the midlayer only and are used to restore the original payload after
request_bufferandrequest_bufflenhave been overwritten for EH. Using the buffer and bufflen fields means they do very broken things in error handling.False statement, for libata.
Why? I don't see anywhere in libata where it touches ->buffer or ->bufflen other than the two cases Christoph removed.
Quoted 1 line · Jeff Garzik
False statement, for libata.
Could you amplify this statement, please ... I looked through the head of libata-dev and it seems that these are still the only two bufflen uses, which still do look obviously wrong ... I don't see where the problem in libata with this is?
Quoted 2 lines
Please CC the author (me) or linux-ide, at least, when you touch libata.
The tradition is that for infrastructure changes like this, particularly ones that are trivial, as this appears to be, you simply copy the scsi list and don't have to sweep up the individual maintainers of each driver.
James
Quoted 7 lines · James Bottomley
Quoted 1 line · Jeff Garzik
False statement, for libata.
Could you amplify this statement, please ... I looked through the head of libata-dev and it seems that these are still the only two bufflen uses, which still do look obviously wrong ... I don't see where the problem in libata with this is?
Christoph's false statement is: "Using the buffer and bufflen fields means they do very broken things in error handling."
libata does not do "very broken things in error handling."
Quoted 7 lines
Quoted 2 lines
Please CC the author (me) or linux-ide, at least, when you touch libata.
The tradition is that for infrastructure changes like this, particularly ones that are trivial, as this appears to be, you simply copy the scsi list and don't have to sweep up the individual maintainers of each driver.
This is apparently an EH-related patch, and libata uses SCSI EH very differently from all other drivers. In this case, Christoph's "very broken" justification is clearly inaccurate, and thus a CC is obviously warranted.
Also, I remind people constantly of this, so I'm sure you and Christoph have heard the request before.
Jeff, the one who forwards to linux-scsi when others forget
Quoted 9 lines · Jeff Garzik
Quoted 7 lines · James Bottomley
Quoted 1 line · Jeff Garzik
False statement, for libata.
Could you amplify this statement, please ... I looked through the head of libata-dev and it seems that these are still the only two bufflen uses, which still do look obviously wrong ... I don't see where the problem in libata with this is?
Quoted 4 lines
Christoph's false statement is: "Using the buffer and bufflen fields means they do very broken things in error handling."
libata does not do "very broken things in error handling."
OK ... let me explain what we're doing a bit. One of the things that has been going on for quite a while is that Intel (Ken Chen) has analyses showing that allocation clearing and freeing of the scsi_cmnd structure is a drag on the fast path. This is part of the drive to slim it down (i.e. get rid of a lot of the fields that are only used in eh).
Quoted 13 lines
Quoted 7 lines
Quoted 2 lines
Please CC the author (me) or linux-ide, at least, when you touch libata.
The tradition is that for infrastructure changes like this, particularly ones that are trivial, as this appears to be, you simply copy the scsi list and don't have to sweep up the individual maintainers of each driver.
This is apparently an EH-related patch, and libata uses SCSI EH very differently from all other drivers. In this case, Christoph's "very broken" justification is clearly inaccurate, and thus a CC is obviously warranted.
Quoted 4 lines
Also, I remind people constantly of this, so I'm sure you and Christoph have heard the request before.
Jeff, the one who forwards to linux-scsi when others forget
I appreciate that for changes you want to make to SCSI. However, this particular patch touches seventeen separately maintained pieces of code within the SCSI subsystem, only one of which is libata. I'm really reluctant to change the policy in this regard ... it's a policy we copied from lkml ... that for sweeping infrastructure changes we only send it to the list and don't have to work out who all the maintainers are.
James
Quoted 10 lines · James Bottomley
Quoted 9 lines · Jeff Garzik
Quoted 6 lines · James Bottomley
Quoted 1 line · Jeff Garzik
False statement, for libata.
Could you amplify this statement, please ... I looked through the head of libata-dev and it seems that these are still the only two bufflen uses, which still do look obviously wrong ... I don't see where the problem in libata with this is?
Christoph's false statement is: "Using the buffer and bufflen fields means they do very broken things in error handling."
Quoted 7 lines
Quoted 1 line
libata does not do "very broken things in error handling."
OK ... let me explain what we're doing a bit. One of the things that has been going on for quite a while is that Intel (Ken Chen) has analyses showing that allocation clearing and freeing of the
scsi_cmndstructure is a drag on the fast path. This is part of the drive to slim it down (i.e. get rid of a lot of the fields that are only used in eh).
OK.
Since libata does EH differently than all other SCSI drivers, it certainly makes sense to CC the appropriate maintainers.
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines
Quoted 2 lines
Please CC the author (me) or linux-ide, at least, when you touch libata.
The tradition is that for infrastructure changes like this, particularly ones that are trivial, as this appears to be, you simply copy the scsi list and don't have to sweep up the individual maintainers of each driver.
This is apparently an EH-related patch, and libata uses SCSI EH very differently from all other drivers. In this case, Christoph's "very broken" justification is clearly inaccurate, and thus a CC is obviously warranted.
Also, I remind people constantly of this, so I'm sure you and Christoph have heard the request before.
Jeff, the one who forwards to linux-scsi when others forget
Quoted 7 lines
I appreciate that for changes you want to make to SCSI. However, this particular patch touches seventeen separately maintained pieces of code within the SCSI subsystem, only one of which is libata. I'm really reluctant to change the policy in this regard ... it's a policy we copied from lkml ... that for sweeping infrastructure changes we only send it to the list and don't have to work out who all the maintainers are.
If the submittor is under the impression that libata's error handling is "very broken", I would appreciate a clarification. Otherwise, one must assume that the submittor should have CC'd linux-ide and relevant maintainers, because they do not understand the code they are patching.
What _precisely_ is broken, given that libata does all its own error handling, and ignores scsi_unjam_host() ?
Jeff
Quoted 10 lines · Jeff Garzik
If the submittor is under the impression that libata's error handling is
"very broken", I would appreciate a clarification. Otherwise, one must
assume that the submittor should have CC'd linux-ide and relevant maintainers, because they do not understand the code they are patching.What _precisely_ is broken, given that libata does all its own error handling, and ignores
scsi_unjam_host()?
The problem being fixed is that the two fields in question are exclusively for the use of the error handler. No driver should ever touch them ... there are equivalent fields for the drivers to use which contain the correct values, which is what this patch is switching to. I don't believe any criticism of the libata error handler was implied or intended ... the problem is the driver piece of libata uses fields it shouldn't, which the patch fixes.
James
Quoted 8 lines · James Bottomley
Quoted 6 lines · Jeff Garzik
If the submittor is under the impression that libata's error handling
is "very broken", I would appreciate a clarification. Otherwise, one
must assume that the submittor should have CC'd linux-ide and relevant maintainers, because they do not understand the code they are patching.What _precisely_ is broken, given that libata does all its own error handling, and ignores
scsi_unjam_host()?
Quoted 7 lines
The problem being fixed is that the two fields in question are exclusively for the use of the error handler. No driver should ever touch them ... there are equivalent fields for the drivers to use which contain the correct values, which is what this patch is switching to. I don't believe any criticism of the libata error handler was implied or intended ... the problem is the driver piece of libata uses fields it shouldn't, which the patch fixes.
ACK, with the added explanation of why this is needed.
Thanks,
Jeff
Quoted 6 lines · Christoph Hellwig
Various scsi drivers use scsi_cmnd.buffer and scsi_cmnd.bufflen in their queuecommand functions. Those fields are internal storage for the midlayer only and are used to restore the original payload after
request_bufferandrequest_bufflenhave been overwritten for EH. Using the buffer and bufflen fields means they do very broken things in error handling.
We probably need this too, don't we?
James
drivers/scsi/scsi_lib.c +1 -1
@@ -1235,7 +1235,7 @@ static void scsi_blk_pc_done(struct scsi* successfully. Since this is a REQ_BLOCK_PC command the * caller should check the request's errors value */ -scsi_io_completion (cmd, cmd->bufflen,0 ); +scsi_io_completion (cmd, cmd->request_bufflen,0 ); }static void scsi_setup_blk_pc_cmnd (struct scsi_cmnd *cmd)
Quoted 9 lines · James Bottomley
Quoted 6 lines · Christoph Hellwig
Various scsi drivers use scsi_cmnd.buffer and scsi_cmnd.bufflen in their queuecommand functions. Those fields are internal storage for the midlayer only and are used to restore the original payload after
request_bufferandrequest_bufflenhave been overwritten for EH. Using the buffer and bufflen fields means they do very broken things in error handling.We probably need this too, don't we?
At this point both values are always the same. The patch I sent is just driver fixes, all the core changes are in the second patch I sent out as an rfc.