Subject: Re: [PATCH] [v2] aha152x cmnd->device oops - msg#00362

Previous Message by Date: click to view message preview

Re: [usb-storage] Re: [PATCH] SCSI: limit mode sense usage

On Tue, Oct 28, 2003 at 07:16:21PM -0700, Ronald Kuetemeier wrote: > > The scsi_dev_flags overrides the host adapter setting, and we did not tell > > it to skip page 8. Since it worked it does not matter, but we could have > > used (given BLIST_MS_SKIP_PAGE_08 is 0x2000 and BLIST_MS_SKIP_PAGE_3F is > > 0x4000): > Patrick, > isn't BLIST_MS_SKIP_PAGE_08 always turned "on"? No, see scsi_devinfo.c scsi_get_device_flags, it can override the shost->flags value. So if wanted we could set it to zero via /proc/scsi/device_info or scsi_dev_flags. -- Patrick Mansfield - 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

Next Message by Date: click to view message preview

RE: Request for review of Linux iSCSI driver version 4.0.0.1

Hi, I have interspersed my answers below for 2 of your comments. > - kill the is_root_disk special casing. drivers can't know > what the root disks is, there might not even be a single one. This flag is needed for our iSCSI network boot implementation to check if we are booted on iSCSI disk. This is used while trying to shutdown the iSCSI service. > - what the heck is iscsi_set_if_addr? You have absolutely no business > messing with network device configuration. This is again needed for network boot. The n/w driver and iSCSI driver are loaded during early boot. Since the n/w interface is not setup at that time, we need to bring up the interface through this call in iSCSI driver. Thanks, surekha - 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

Previous Message by Thread: click to view message preview

Re: [PATCH] [v2] aha152x cmnd->device oops

On Tue, 28 Oct 2003 14:50:28 -0800 Mike Christie <mikenc@xxxxxxxxxx> wrote: | Hi Randy, | | > if(!(DONE_SC->SCp.Status & not_issued)) { | > - Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC); | > + Scsi_Cmnd *cmnd = scsi_get_command(DONE_SC->device, GFP_ATOMIC); | | Do you need to add a scsi_put_command to balance this get when the | command completes? | | > + cmnd->device = ptr->device; | | Don't even need this now. scsi_get_command will set the device for you. | | Mike Hers's v3 of the aha152x patch. However, I'm not satisfied with it, so I'm not asking James to apply it. I don't know the state machine or the hardware well enough to understand and patch it. Things that I don't like about it: a. calling aha152x_internal_queue() with cmnd pointer in 2 places. Probably the second one should be NULL. b. Easy to confuse the state machine by changing just one variable, like DONE_SC (i.e., it's risky and I can't test it, although we do have a bug report and can ask that reporter to test it). c. int result = SCpnt->SCp.Status; /*FIXME*/ SCp.Status is not valid here AFAIK, and I don't know how to get the current command status at this point. -- ~Randy description: aha152x oopses when it references cmnd->device->... before cmnd->device has been init, so instead of allocating a new scsi_cmnd, just reuse the current one for the mode sense; then in internal_done(), if the command was a special REQUEST_SENSE command, restore the "old" command. maintainer: Juergen Fischer <fischer@xxxxxxxxx> product_versions: Linux linux-260-test9-pv patch_name: aha152x-cmd-ca.patch author: Randy.Dunlap <rddunlap@xxxxxxxx> patch_version: 2003-10-28.16:17:37 diffstat:= drivers/scsi/aha152x.c | 75 +++++++++++++++++++++++++++++-------------------- 1 files changed, 45 insertions(+), 30 deletions(-) diff -Naurp ./drivers/scsi/aha152x.c~aha152xfix ./drivers/scsi/aha152x.c --- ./drivers/scsi/aha152x.c~aha152xfix 2003-10-25 11:42:50.000000000 -0700 +++ ./drivers/scsi/aha152x.c 2003-10-28 16:15:24.000000000 -0800 @@ -306,6 +306,8 @@ #define DELAY_DEFAULT 1000 +#define AHA15_INTERNAL_SENSE_MAGIC 0x42 + #if defined(PCMCIA) #define IRQ_MIN 0 #define IRQ_MAX 16 @@ -1513,6 +1515,21 @@ static void internal_done(Scsi_Cmnd *SCp DPRINTK(debug_eh, INFO_LEAD "internal_done called\n", CMDINFO(SCpnt)); #endif + if (SCpnt && SCpnt->cmnd[0] == REQUEST_SENSE && SCpnt->cmnd[6] == AHA15_INTERNAL_SENSE_MAGIC) { + int result = SCpnt->SCp.Status; /*FIXME*/ + /* restore old result if the request sense was successful */ + if (result == 0) + result = SCpnt->cmnd[7]; + /* now restore the original command */ + memcpy((void *) SCpnt->cmnd, (void *) SCpnt->data_cmnd, + sizeof(SCpnt->data_cmnd)); + SCpnt->request_buffer = SCpnt->buffer; + SCpnt->request_bufflen = SCpnt->bufflen; + SCpnt->use_sg = SCpnt->old_use_sg; + SCpnt->cmd_len = SCpnt->old_cmd_len; + SCpnt->sc_data_direction = SCpnt->sc_old_data_direction; + SCpnt->underflow = SCpnt->old_underflow; + } if(SCSEM(SCpnt)) up(SCSEM(SCpnt)); } @@ -2001,38 +2018,36 @@ static void busfree_run(struct Scsi_Host #endif if(!(DONE_SC->SCp.Status & not_issued)) { - Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC); - - if(cmnd) { - Scsi_Cmnd *ptr=DONE_SC; - DONE_SC=0; - + Scsi_Cmnd *cmnd = DONE_SC; + int status = DONE_SC->SCp.Status; + DONE_SC=0; #if 0 - DPRINTK(debug_eh, ERR_LEAD "requesting sense\n", CMDINFO(ptr)); + DPRINTK(debug_eh, ERR_LEAD "requesting sense\n", + CMDINFO(ptr)); #endif - - cmnd->cmnd[0] = REQUEST_SENSE; - cmnd->cmnd[1] = 0; - cmnd->cmnd[2] = 0; - cmnd->cmnd[3] = 0; - cmnd->cmnd[4] = sizeof(ptr->sense_buffer); - cmnd->cmnd[5] = 0; - cmnd->cmd_len = 6; - cmnd->device->host = ptr->device->host; - cmnd->device->id = ptr->device->id; - cmnd->device->lun = ptr->device->lun; - cmnd->use_sg = 0; - cmnd->request_buffer = ptr->sense_buffer; - cmnd->request_bufflen = sizeof(ptr->sense_buffer); - - DO_UNLOCK(flags); - aha152x_internal_queue(cmnd, 0, 0, ptr, internal_done); - DO_LOCK(flags); - } else { - printk(ERR_LEAD "allocation failed\n", CMDINFO(CURRENT_SC)); - if(cmnd) - kfree(cmnd); - } + cmnd->cmnd[0] = REQUEST_SENSE; + cmnd->cmnd[1] = 0; + cmnd->cmnd[2] = 0; + cmnd->cmnd[3] = 0; + cmnd->cmnd[4] = sizeof(cmnd->sense_buffer); + cmnd->cmnd[5] = 0; + cmnd->cmd_len = 6; + /* Here's a quiet hack [from 53c700.c]: the + * REQUEST_SENSE command is six bytes, + * so store a flag indicating that + * this was an internal sense request + * and the original status at the end + * of the command */ + cmnd->cmnd[6] = AHA15_INTERNAL_SENSE_MAGIC; + cmnd->cmnd[7] = status; + cmnd->use_sg = 0; + cmnd->sc_data_direction = SCSI_DATA_READ; + cmnd->request_buffer = cmnd->sense_buffer; + cmnd->request_bufflen = sizeof(cmnd->sense_buffer); + + DO_UNLOCK(flags); + aha152x_internal_queue(cmnd, 0, 0, cmnd, internal_done); + DO_LOCK(flags); } else { #if 0 DPRINTK(debug_eh, ERR_LEAD "command not issued - CHECK CONDITION ignored\n", CMDINFO(DONE_SC)); - 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

Next Message by Thread: click to view message preview

Re: [PATCH] [v2] aha152x cmnd->device oops

On Wed, 2003-10-29 at 06:20, Juergen E. Fischer wrote: > The first is when a command returns with status CHECK CONDITION and the > driver needs to do a REQUEST SENSE to fetch sense data and add that to > the Ssci_Cmnd which resulted in the CHECK CONDITION. The internally > queued command fills the sense_buffer of the offending command and then > calls its ->scsi_done(). You don't actually need a new command for this. The struct scsi_cmnd layout is designed to allow you to reuse the current command for clearing contingent allegiance conditions (the fields you need to alter all have copies stored somewhere which you can restore when you have the sense data. See for example how the 53c700 does this, or aic7xxx_old) > The second it when the controller needs to be resetted. Both are purely > internal to the driver. As a rule of thumb: as long as the command is never seen outside the driver, then using kmalloc is fine. If it is ever seen outside the driver (like in requeueing conditions) then you should use scsi_get_command(). James James - 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