Re: Compaq Fiber Channel Array RM4000 / 2.6.16 kernel patch

[Matthew Wilcox <matthew@wil.cx>]

On Thu, Mar 30, 2006 at 04:20:25PM +0200, Ingo Flaschberger wrote:
> >> 	{"CNSi", "G8324", NULL, BLIST_SPARSELUN},	/* Chaparral G8324 
> >> 	RAID */
> >>+	{"COMPAQ", "ARRAY CONTROLLER", "2.60", BLIST_SPARSELUN | 
> >>BLIST_LARGELUN |
> >>+		BLIST_MAX_512K}, /* Compaq RA4x00 */
> >>+	{"COMPAQ", "LOGICAL VOLUME", "2.60", BLIST_MAX_512K}, /* Compaq 
> >>RA4x00 */
> >> 	{"COMPAQ", "LOGICAL VOLUME", NULL, BLIST_FORCELUN},
> >
> >I wonder if you should just be adding BLIST_MAX_512K to the current
> >entry for LOGICAL VOLUME instead?
> 
> The "LOGICAL VOLUME" was there before and I think I will break the SCSI 
> Version.

I meant to do something like:

-	{"COMPAQ", "LOGICAL VOLUME", NULL, BLIST_FORCELUN},
+	{"COMPAQ", "LOGICAL VOLUME", NULL, BLIST_FORCELUN | BLIST_MAX_512K},

> >I'm not sure that's what you meant to do, even.  How about more simply:
> >
> >	/*
> >	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not 
> >	 set.
> >	 * Also allow SCSI-2 and unknown devices if BLIST_REPORTLUN2 is set
> >	 * and host adapter supports more than 8 LUNs.
> >	 */
> >	if ((bflags & BLIST_NOREPORTLUN)
> >		return 1;
> >	if ((starget->scsi_level < SCSI_2) &&
> >			(starget->scsi_level != SCSI_UNKNOWN))
> >		return 1;
> >	if ((starget->scsi_level < SCSI_3) &&
> >			(!(bflags & BLIST_REPORTLUN2) || shost->max_lun <= 
> >			8))
> >		return 1;
> >
> >
> 
> Attached,, new patch version 3....

> --- linux-2.6.16_org/drivers/scsi/scsi.c	2006-03-20 06:53:29.000000000 +0100
> +++ linux-2.6.16/drivers/scsi/scsi.c	2006-03-30 17:29:26.000000000 +0200
> @@ -567,7 +567,7 @@
>  	/* 
>  	 * If SCSI-2 or lower, store the LUN value in cmnd.
>  	 */
> -	if (cmd->device->scsi_level <= SCSI_2) {
> +	if ((cmd->device->scsi_level <= SCSI_2) && (cmd->device->scsi_level != SCSI_UNKNOWN)) {

Oh, could you break the line so it doesn't go over 80 columns?

>  		sdev->select_no_atn = 1;
>  
>  	/*
> +	* Maximum 512K cdb transfer length
> +	* broken RA4x00 Compaq Disk Array
> +	*/
> +	sdev->max_xfer_len = 0xffff;
> +	if (*bflags & BLIST_MAX_512K) {
> +		sdev->max_xfer_len = 0x200;
> +	}

Do we need the comment?  We don't need the braces.

> +	if (starget->scsi_level < SCSI_3 &&
> +			((!(bflags & BLIST_REPORTLUN2) &&
> +			(starget->scsi_level != SCSI_UNKNOWN)) ||
> +			shost->max_lun <= 8))

Why do we need the SCSI_UNKNOWN check here?

>   *	   Support 32k/1M disks.
> + *	 - Added Compaq RA4x00 Fiber Channel Array support, 29.03.2006
> + *	   crossip communications gmbh - Ingo Flaschberger <if@xip.at>
>   *

This doesn't really describe what you've done to this file.  It's more
 - Limit maximum transfers to new field in scsi_device
... or something

> @@ -89,6 +89,7 @@
>  				 * scsi_devinfo.[hc]. For now used only to
>  				 * pass settings from slave_alloc to scsi
>  				 * core. */
> +	unsigned short max_xfer_len;	/* Maximum xfer length in one cdb */
>  	unsigned writeable:1;
>  	unsigned removable:1;
>  	unsigned changed:1;	/* Data invalid due to media change */

I think you missed my point about where to place it -- due to padding
rules, you've grown the structure by 4 bytes.  If you put it between
current_cmnd and queue_depth, you won't grow the struct at all.