FUJITA Tomonori wrote: > On Wed, 22 Oct 2008 12:08:27 +0200 > Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: > >> FUJITA Tomonori wrote: >>> On Wed, 22 Oct 2008 11:04:44 +0200 >>> Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: >>> >>>> FUJITA Tomonori wrote: >>>>> On Tue, 21 Oct 2008 22:22:37 +0200 >>>>> "Pascal Terjan" <pterjan@xxxxxxxxx> wrote: >>>>> >>>>>> On Tue, Oct 21, 2008 at 9:54 PM, Matthew Wilcox <matthew@xxxxxx> wrote: >>>>>>> On Tue, Oct 21, 2008 at 12:47:01PM -0700, Andrew Morton wrote: >>>>>>>>> Latest working kernel version: 2.6.24 >>>>>>>>> Earliest failing kernel version: 2.6.27-rc8 >>>>>>>> It's a regression. >>>>>>>> >>>>>>>>> Pid: 2319, comm: diff Not tainted (2.6.27-server-0.rc8.2mnb #1) >>>>>>> It's also a distro kernel by the looks of things. Can it be reproduced >>>>>>> with an upstream kernel? >>>>>> I will try booting the server on vanilla kernel but I'm not sure when >>>>>> (we already rebooted it 2 times recently and users won't enjoy it). >>>>>> >>>>>> This is a distro kernel but I don't see patches that could impact this : >>>>>> http://svn.mandriva.com/cgi-bin/viewvc.cgi/packages/cooker/kernel/current/PATCHES/patches/ >>>>>> >>>>>> Machine is a old HP NetServer LT 6000 >>>>>> >>>>>> 04:03.1 I2O: Intel Corporation 80960RP (i960RP) Microprocessor (rev >>>>>> 09) (prog-if 01) >>>>>> Subsystem: Hewlett-Packard Company MegaRAID, Integrated NetRAID >>>>>> Flags: bus master, fast Back2Back, medium devsel, latency 64, IRQ 11 >>>>>> Memory at f4000000 (32-bit, prefetchable) [size=64M] >>>>>> [virtual] Expansion ROM at a8130000 [disabled] [size=32K] >>>>>> Capabilities: [80] Power Management version 2 >>>>>> Kernel driver in use: megaraid_legacy >>>>>> Kernel modules: i2o_core, megaraid >>>>> This patch helps? >>>>> >>>>> >>>>> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c >>>>> index 28c9da7..9294ed8 100644 >>>>> --- a/drivers/scsi/megaraid.c >>>>> +++ b/drivers/scsi/megaraid.c >>>>> @@ -4414,12 +4414,14 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) >>>>> >>>>> scmd = &adapter->int_scmd; >>>>> memset(scmd, 0, sizeof(Scsi_Cmnd)); >>>>> + memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb)); >>>>> >>>>> sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL); >>>>> scmd->device = sdev; >>>>> >>>>> scmd->device->host = adapter->host; >>>>> scmd->host_scribble = (void *)scb; >>>>> + scmd->cmnd = adapter->int_cdb; >>>>> scmd->cmnd[0] = MEGA_INTERNAL_CMD; >>>>> >>>>> scb->state |= SCB_ACTIVE; >>>>> diff --git a/drivers/scsi/megaraid.h b/drivers/scsi/megaraid.h >>>>> index ee70bd4..5ffec15 100644 >>>>> --- a/drivers/scsi/megaraid.h >>>>> +++ b/drivers/scsi/megaraid.h >>>>> @@ -889,6 +889,7 @@ typedef struct { >>>>> u8 sglen; /* f/w supported scatter-gather list length */ >>>>> >>>>> scb_t int_scb; >>>>> + unsigned char int_cdb[MAX_COMMAND_SIZE]; >>>>> Scsi_Cmnd int_scmd; >>>>> struct mutex int_mtx; /* To synchronize the internal >>>>> commands */ >>>>> >>>>> -- >>>> Hi TOMO. >>>> >>>> This might not be enough for example I don't see the allocation of sense_buffer. >>>> It might be much easer to allocate using the new command allocation API James >>>> did, just for such cases. These are: scsi_allocate_command/scsi_free_command >>> Yeah, it might be. It's fine by me too. But this code path is used >>> only for issuing internal special commands. It doesn't use the great >>> portion of scsi_cmnd. For example, these commands don't use sense >>> buffer, I think. The code path uses scsi_cmnd just for hooking scb_t, >>> a structure that megaraid allocates per command. >> OK Thanks. >> I was not sure because it looks like in mega_cmd_done(), if the status is >> 0x2 (CHECK_CONDITION) then it would set the sense_buffer. But from what >> you say, the HW will never return 0x2 in case of an Internal-Command. I >> Just wanted to make sure. > > I thought that all internal commands are non SCSI command but seems > that there is one exception (issuing INQUIRY as an internal command). > I'm not sure I understand correctly the driver but anyway here is a > version using scsi_allocate_command and scsi_free_command. > Thanks TOMO. This is actual my bug from the days of making a scsi_cmnd->cmnd into a pointer, and skipping this driver. Reviewed-by: Boaz Harrosh <bharrosh@xxxxxxxxxxx> > I guess that we need to check the kzalloc failure too but it is > supposed to be fixed by a different patch. > > > diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c > index 28c9da7..7dc62de 100644 > --- a/drivers/scsi/megaraid.c > +++ b/drivers/scsi/megaraid.c > @@ -4402,6 +4402,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) > scb_t *scb; > int rval; > > + scmd = scsi_allocate_command(GFP_KERNEL); > + if (!scmd) > + return -ENOMEM; > + > /* > * The internal commands share one command id and hence are > * serialized. This is so because we want to reserve maximum number of > @@ -4412,12 +4416,11 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) > scb = &adapter->int_scb; > memset(scb, 0, sizeof(scb_t)); > > - scmd = &adapter->int_scmd; > - memset(scmd, 0, sizeof(Scsi_Cmnd)); > - > sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL); > scmd->device = sdev; > > + memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb)); > + scmd->cmnd = adapter->int_cdb; > scmd->device->host = adapter->host; > scmd->host_scribble = (void *)scb; > scmd->cmnd[0] = MEGA_INTERNAL_CMD; > @@ -4456,6 +4459,8 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) > > mutex_unlock(&adapter->int_mtx); > > + scsi_free_command(GFP_KERNEL, scmd); > + > return rval; > } > > diff --git a/drivers/scsi/megaraid.h b/drivers/scsi/megaraid.h > index ee70bd4..795201f 100644 > --- a/drivers/scsi/megaraid.h > +++ b/drivers/scsi/megaraid.h > @@ -888,8 +888,8 @@ typedef struct { > > u8 sglen; /* f/w supported scatter-gather list length */ > > + unsigned char int_cdb[MAX_COMMAND_SIZE]; > scb_t int_scb; > - Scsi_Cmnd int_scmd; > struct mutex int_mtx; /* To synchronize the internal > commands */ > struct completion int_waitq; /* wait queue for internal TODO: One more thing that needs to be done in this driver is one time allocation of a scsi host-device and use it as a proper "scmd->device = sdev". And freeing at destruction. Failing to do so enables a posibility of an internal command been completed after the deletion of the host. Thanks again Boaz -- 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
