lkml.org 
[lkml]   [2009]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [GIT PATCH] firs round of SCSI bug fixes for 2.6.29-rc1
    From
    Date
    On Fri, 2009-01-16 at 09:09 -0800, Andrew Morton wrote:
    > On Fri, 16 Jan 2009 09:56:11 -0500 James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
    >
    > > This is acutally four bug fixes and part of a fusion update which was a
    > > bit late processing through the merge window, but which should be
    > > perfectly acceptable for -rc1.
    > >
    > > ...
    > >
    > > Full diff below.
    >
    > I like full diffs.
    >
    > > --- a/drivers/message/fusion/mptbase.c
    > > +++ b/drivers/message/fusion/mptbase.c
    > > @@ -79,9 +79,22 @@ MODULE_VERSION(my_VERSION);
    > > /*
    > > * cmd line parameters
    > > */
    > > -static int mpt_msi_enable = -1;
    > > -module_param(mpt_msi_enable, int, 0);
    > > -MODULE_PARM_DESC(mpt_msi_enable, " MSI Support Enable (default=0)");
    > > +
    > > +static int mpt_msi_enable_spi;
    > > +module_param(mpt_msi_enable_spi, int, 0);
    > > +MODULE_PARM_DESC(mpt_msi_enable_spi, " Enable MSI Support for SPI \
    > > + controllers (default=0)");
    > > +
    > > +static int mpt_msi_enable_fc;
    > > +module_param(mpt_msi_enable_fc, int, 0);
    > > +MODULE_PARM_DESC(mpt_msi_enable_fc, " Enable MSI Support for FC \
    > > + controllers (default=0)");
    > > +
    > > +static int mpt_msi_enable_sas;
    > > +module_param(mpt_msi_enable_sas, int, 1);
    > > +MODULE_PARM_DESC(mpt_msi_enable_sas, " Enable MSI Support for SAS \
    > > + controllers (default=1)");
    > > +
    > >
    > > ...
    >
    > Those MODULE_PARM_DESC strings are going to come out funny-looking,
    > with extra tabs and stuff.

    Yes ... the compiler complained about a few of the other updates, which
    I told them to redo ... I didn't notice these.

    > > +MODULE_PARM_DESC(mpt_debug_level, " debug level - refer to mptdebug.h \
    > > + - (default=0)");
    > > +
    > > +int mpt_fwfault_debug;
    > > +EXPORT_SYMBOL(mpt_fwfault_debug);
    > > +module_param_call(mpt_fwfault_debug, param_set_int, param_get_int,
    > > + &mpt_fwfault_debug, 0600);
    > > +MODULE_PARM_DESC(mpt_fwfault_debug, "Enable detection of Firmware fault"
    > > + " and halt Firmware on fault - (default=0)");
    >
    > That one got it right.
    >
    > >
    > > #ifdef MFCNT
    > > static int mfcounter = 0;
    > > @@ -1751,16 +1774,25 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
    > > ioc->bus_type = SAS;
    > > }
    > >
    > > - if (mpt_msi_enable == -1) {
    > > - /* Enable on SAS, disable on FC and SPI */
    > > - if (ioc->bus_type == SAS)
    > > - ioc->msi_enable = 1;
    > > - else
    > > - ioc->msi_enable = 0;
    > > - } else
    > > - /* follow flag: 0 - disable; 1 - enable */
    > > - ioc->msi_enable = mpt_msi_enable;
    > >
    > > + switch (ioc->bus_type) {
    > > +
    > > + case SAS:
    > > + ioc->msi_enable = mpt_msi_enable_sas;
    > > + break;
    > > +
    > > + case SPI:
    > > + ioc->msi_enable = mpt_msi_enable_spi;
    > > + break;
    > > +
    > > + case FC:
    > > + ioc->msi_enable = mpt_msi_enable_fc;
    > > + break;
    > > +
    > > + default:
    > > + ioc->msi_enable = 0;
    > > + break;
    > > + }
    > > if (ioc->errata_flag_1064)
    > > pci_disable_io_access(pdev);
    > >
    > > @@ -6313,6 +6345,33 @@ mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh
    > > *size = y;
    > > }
    > >
    > > +
    > > +/**
    > > + * mpt_halt_firmware - Halts the firmware if it is operational and panic
    > > + * the kernel
    > > + * @ioc: Pointer to MPT_ADAPTER structure
    > > + *
    > > + **/
    > > +void
    > > +mpt_halt_firmware(MPT_ADAPTER *ioc)
    > > +{
    > > + u32 ioc_raw_state;
    > > +
    > > + ioc_raw_state = mpt_GetIocState(ioc, 0);
    > > +
    > > + if ((ioc_raw_state & MPI_IOC_STATE_MASK) == MPI_IOC_STATE_FAULT) {
    > > + printk(MYIOC_s_ERR_FMT "IOC is in FAULT state (%04xh)!!!\n",
    > > + ioc->name, ioc_raw_state & MPI_DOORBELL_DATA_MASK);
    > > + panic("%s: IOC Fault (%04xh)!!!\n", ioc->name,
    > > + ioc_raw_state & MPI_DOORBELL_DATA_MASK);
    > > + } else {
    > > + CHIPREG_WRITE32(&ioc->chip->Doorbell, 0xC0FFEE00);
    > > + panic("%s: Firmware is halted due to command timeout\n",
    > > + ioc->name);
    > > + }
    > > +}
    > > +EXPORT_SYMBOL(mpt_halt_firmware);
    >
    > Doing a panic() after we've already detected an error is plain nasty.
    > Is there no way in which we can allow the kernel to continue?

    There's a long thread discussing this very point on linux-scsi. The
    short version is "no, it's only used for debugging firmware and if
    you've corrupted your firmware to this point you need the machine
    halting".

    >
    > > /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
    > > /*
    > > * Reset Handling
    > > @@ -6345,6 +6404,8 @@ mpt_HardResetHandler(MPT_ADAPTER *ioc, int sleepFlag)
    > > printk(MYIOC_s_INFO_FMT "HardResetHandler Entered!\n", ioc->name);
    > > printk("MF count 0x%x !\n", ioc->mfcnt);
    > > #endif
    > > + if (mpt_fwfault_debug)
    > > + mpt_halt_firmware(ioc);
    > >
    > > /* Reset the adapter. Prevent more than 1 call to
    > > * mpt_do_ioc_recovery at any instant in time.
    > > diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h
    > > index dff048c..b3e981d 100644
    > > --- a/drivers/message/fusion/mptbase.h
    > > +++ b/drivers/message/fusion/mptbase.h
    > > @@ -922,11 +922,14 @@ extern void mpt_free_fw_memory(MPT_ADAPTER *ioc);
    > > extern int mpt_findImVolumes(MPT_ADAPTER *ioc);
    > > extern int mptbase_sas_persist_operation(MPT_ADAPTER *ioc, u8 persist_opcode);
    > > extern int mpt_raid_phys_disk_pg0(MPT_ADAPTER *ioc, u8 phys_disk_num, pRaidPhysDiskPage0_t phys_disk);
    > > +extern void mpt_halt_firmware(MPT_ADAPTER *ioc);
    > > +
    > >
    > > /*
    > > * Public data decl's...
    > > */
    > > extern struct list_head ioc_list;
    > > +extern int mpt_fwfault_debug;
    > >
    > > /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
    > > #endif /* } __KERNEL__ */
    > > diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c
    > > index ee09041..e62c6bc 100644
    > > --- a/drivers/message/fusion/mptscsih.c
    > > +++ b/drivers/message/fusion/mptscsih.c
    > > @@ -1846,6 +1846,9 @@ mptscsih_abort(struct scsi_cmnd * SCpnt)
    > > if (hd->timeouts < -1)
    > > hd->timeouts++;
    > >
    > > + if (mpt_fwfault_debug)
    > > + mpt_halt_firmware(ioc);
    >
    > So one single global flag affects all controllers in the system?
    >
    > I guess that's OK if it's just a debug/diag thing.

    Yes, that's correct.

    > > /* Most important! Set TaskMsgContext to SCpnt's MsgContext!
    > > * (the IO to be ABORT'd)
    > > *
    > > diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
    > > index a745f91..e7705d3 100644
    > > --- a/drivers/scsi/libiscsi_tcp.c
    > > +++ b/drivers/scsi/libiscsi_tcp.c
    > > @@ -177,7 +177,6 @@ int iscsi_tcp_segment_done(struct iscsi_tcp_conn *tcp_conn,
    > > struct iscsi_segment *segment, int recv,
    > > unsigned copied)
    > > {
    > > - static unsigned char padbuf[ISCSI_PAD_LEN];
    > > struct scatterlist sg;
    > > unsigned int pad;
    > >
    > > @@ -233,7 +232,7 @@ int iscsi_tcp_segment_done(struct iscsi_tcp_conn *tcp_conn,
    > > debug_tcp("consume %d pad bytes\n", pad);
    > > segment->total_size += pad;
    > > segment->size = pad;
    > > - segment->data = padbuf;
    > > + segment->data = segment->padbuf;
    > > return 0;
    > > }
    > > }
    > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
    > > index 2d4f32b..9ad4d09 100644
    > > --- a/drivers/scsi/qla2xxx/qla_init.c
    > > +++ b/drivers/scsi/qla2xxx/qla_init.c
    > > @@ -1258,35 +1258,48 @@ qla2x00_init_rings(scsi_qla_host_t *vha)
    > > {
    > > int rval;
    > > unsigned long flags = 0;
    > > - int cnt;
    > > + int cnt, que;
    > > struct qla_hw_data *ha = vha->hw;
    > > - struct req_que *req = ha->req_q_map[0];
    > > - struct rsp_que *rsp = ha->rsp_q_map[0];
    > > + struct req_que *req;
    > > + struct rsp_que *rsp;
    > > + struct scsi_qla_host *vp;
    > > struct mid_init_cb_24xx *mid_init_cb =
    > > (struct mid_init_cb_24xx *) ha->init_cb;
    >
    > This cast worries me. It's a cast between two complex data structures
    > which appear to have nothing to do with each other.

    Actually, it's a C++ type construct. ha->init_cb is of type init_cb_t,
    mid_init_cb actually contains this (via a second indirection) as the
    first element, so what it's doing is dynamically casting out based on
    the board type.

    > Oh well. It _is_ a scsi driver :(

    James




    \
     
     \ /
      Last update: 2009-01-16 19:51    [W:0.047 / U:30.376 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site