Date:	Fri, 16 Jan 2009 09:09:28 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	James Bottomley <James.Bottomley@...senPartnership.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PATCH] firs round of SCSI bug fixes for 2.6.29-rc1

On Fri, 16 Jan 2009 09:56:11 -0500 James Bottomley <James.Bottomley@...senPartnership.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.

> +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?


>  /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
>  /*
>   *	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.

>  	/* 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.

Oh well.  It _is_ a scsi driver :(


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists