[PATCH] Stop using num_physpages in aacraid

[PATCH] Stop using num_physpages in aacraid

[Matthew Wilcox]

The num_physpages variable has an interesting and varied definition
depending on the architecture and various CONFIG options, not to mention
whether memory has been hotplugged or not.  At any rate, it doesn't do
what aacraid is trying to use it for -- to determine whether a dma will
be 64-bit.

This patch removes the aac_scsi_32_64() function altogether, favouring
a failure to initialise over failing every command sent to the device.

The various quirks, options and flags are quite confusing.  I think I've
managed to get it right, but I have no hardware to test with.

My understanding:
	AAC_QUIRK_SCSI_32 means "This card cannot address above 64-bit.
	But it does support the 64-bit format."

	AAC_OPT_SGMAP_HOST64 means "This card MUST do DAC".

The patch below is intended to reflect that understanding.  If I've
misunderstood the old code, or that wasn't what the old code was supposed
to mean then the below patch is wrong.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 460d402..95e524a 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1199,15 +1199,6 @@ static int aac_scsi_32(struct fib * fib, struct scsi_cmnd * cmd)
 				  (fib_callback) aac_srb_callback, (void *) cmd);
 }
 
-static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
-{
-	if ((sizeof(dma_addr_t) > 4) &&
-	 (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
-	 (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
-		return FAILED;
-	return aac_scsi_32(fib, cmd);
-}
-
 int aac_get_adapter_info(struct aac_dev* dev)
 {
 	struct fib* fibptr;
@@ -1367,26 +1358,36 @@ int aac_get_adapter_info(struct aac_dev* dev)
 		printk(KERN_INFO "%s%d: Non-DASD support enabled.\n",dev->name, dev->id);
 
 	dev->dac_support = 0;
-	if( (sizeof(dma_addr_t) > 4) && (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)){
+	if ((sizeof(dma_addr_t) > 4) &&
+	    (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)) {
 		if (!dev->in_reset)
 			printk(KERN_INFO "%s%d: 64bit support enabled.\n",
 				dev->name, dev->id);
 		dev->dac_support = 1;
 	}
 
-	if(dacmode != -1) {
+	if (dacmode != -1) {
 		dev->dac_support = (dacmode!=0);
 	}
-	if(dev->dac_support != 0) {
+	if (dev->dac_support &&
+	    !(aac_get_driver_ident(dev->cardtype)->quirks &
+							AAC_QUIRK_SCSI_32)) {
 		if (!pci_set_dma_mask(dev->pdev, DMA_64BIT_MASK) &&
-			!pci_set_consistent_dma_mask(dev->pdev, DMA_64BIT_MASK)) {
+		    !pci_set_consistent_dma_mask(dev->pdev, DMA_64BIT_MASK)) {
 			if (!dev->in_reset)
 				printk(KERN_INFO"%s%d: 64 Bit DAC enabled\n",
 					dev->name, dev->id);
 		} else if (!pci_set_dma_mask(dev->pdev, DMA_32BIT_MASK) &&
-			!pci_set_consistent_dma_mask(dev->pdev, DMA_32BIT_MASK)) {
+			   !pci_set_consistent_dma_mask(dev->pdev,
+							DMA_32BIT_MASK)) {
 			printk(KERN_INFO"%s%d: DMA mask set failed, 64 Bit DAC disabled\n",
 				dev->name, dev->id);
+	 		if (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64) {
+				printk(KERN_INFO "%s%d: 64-bit DAC required "
+					"for this host, bailing out\n",
+					dev->name, dev->id);
+				rcode = -ENOMEM;
+			}
 			dev->dac_support = 0;
 		} else {
 			printk(KERN_WARNING"%s%d: No suitable DMA available.\n",
@@ -1398,11 +1399,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
 	 * Deal with configuring for the individualized limits of each packet
 	 * interface.
 	 */
-	dev->a_ops.adapter_scsi = (dev->dac_support)
-	  ? ((aac_get_driver_ident(dev->cardtype)->quirks & AAC_QUIRK_SCSI_32)
-				? aac_scsi_32_64
-				: aac_scsi_64)
-				: aac_scsi_32;
+	dev->a_ops.adapter_scsi = dev->dac_support ? aac_scsi_64 : aac_scsi_32;
 	if (dev->raw_io_interface) {
 		dev->a_ops.adapter_bounds = (dev->raw_io_64)
 					? aac_bounds_64

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Stop using num_physpages in aacraid

[Mark Salyzyn]

The num_physpages variable works as needed for the PERC controllers  
that are installed in vanilla x86 machines is it not? The PERC card  
would not be installed in any other arch.

AAC_QUIRK_SCSI_32 means the card can not send 64bit format commands to  
the SCSI devices (non DASD) but can send 64bit format commands to the  
logical (Array) devices. This is for a select set of old PERC cards.

AAC_OPT_SGMAP_HOST64 means this card 'can' do DAC (send 64 bit format  
commands to both SCSI and to Array devices), but sadly some that  
report this are borken (hence the Quirk AAC_QUIRK_SCSI_32).

Dropping the cards is not an option, that is the whole reason this  
workaround was put in place.

I can handle dropping these select PERC cards from the list if the  
architecture was NOT x86, since that was never a supported  
architecture for these cards, but I am sure some folks are using them  
this way ;-} I would prefer to DROP Non-DASD support for these cards  
in 64 bit kernels if one is not to use num_physpages!

Sincerely -- Mark Salyzyn

On May 2, 2008, at 2:52 PM, Matthew Wilcox wrote:

>
> The num_physpages variable has an interesting and varied definition
> depending on the architecture and various CONFIG options, not to  
> mention
> whether memory has been hotplugged or not.  At any rate, it doesn't do
> what aacraid is trying to use it for -- to determine whether a dma  
> will
> be 64-bit.
>
> This patch removes the aac_scsi_32_64() function altogether, favouring
> a failure to initialise over failing every command sent to the device.
>
> The various quirks, options and flags are quite confusing.  I think  
> I've
> managed to get it right, but I have no hardware to test with.
>
> My understanding:
>        AAC_QUIRK_SCSI_32 means "This card cannot address above 64-bit.
>        But it does support the 64-bit format."
>
>        AAC_OPT_SGMAP_HOST64 means "This card MUST do DAC".
>
> The patch below is intended to reflect that understanding.  If I've
> misunderstood the old code, or that wasn't what the old code was  
> supposed
> to mean then the below patch is wrong.
>
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
>
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/ 
> aachba.c
> index 460d402..95e524a 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -1199,15 +1199,6 @@ static int aac_scsi_32(struct fib * fib,  
> struct scsi_cmnd * cmd)
>                                  (fib_callback) aac_srb_callback,  
> (void *) cmd);
> }
>
> -static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
> -{
> -       if ((sizeof(dma_addr_t) > 4) &&
> -        (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
> -        (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
> -               return FAILED;
> -       return aac_scsi_32(fib, cmd);
> -}
> -
> int aac_get_adapter_info(struct aac_dev* dev)
> {
>        struct fib* fibptr;
> @@ -1367,26 +1358,36 @@ int aac_get_adapter_info(struct aac_dev* dev)
>                printk(KERN_INFO "%s%d: Non-DASD support enabled. 
> \n",dev->name, dev->id);
>
>        dev->dac_support = 0;
> -       if( (sizeof(dma_addr_t) > 4) && (dev->adapter_info.options &  
> AAC_OPT_SGMAP_HOST64)){
> +       if ((sizeof(dma_addr_t) > 4) &&
> +           (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)) {
>                if (!dev->in_reset)
>                        printk(KERN_INFO "%s%d: 64bit support enabled. 
> \n",
>                                dev->name, dev->id);
>                dev->dac_support = 1;
>        }
>
> -       if(dacmode != -1) {
> +       if (dacmode != -1) {
>                dev->dac_support = (dacmode!=0);
>        }
> -       if(dev->dac_support != 0) {
> +       if (dev->dac_support &&
> +           !(aac_get_driver_ident(dev->cardtype)->quirks &
> +                                                        
> AAC_QUIRK_SCSI_32)) {
>                if (!pci_set_dma_mask(dev->pdev, DMA_64BIT_MASK) &&
> -                       !pci_set_consistent_dma_mask(dev->pdev,  
> DMA_64BIT_MASK)) {
> +                   !pci_set_consistent_dma_mask(dev->pdev,  
> DMA_64BIT_MASK)) {
>                        if (!dev->in_reset)
>                                printk(KERN_INFO"%s%d: 64 Bit DAC  
> enabled\n",
>                                        dev->name, dev->id);
>                } else if (!pci_set_dma_mask(dev->pdev,  
> DMA_32BIT_MASK) &&
> -                       !pci_set_consistent_dma_mask(dev->pdev,  
> DMA_32BIT_MASK)) {
> +                          !pci_set_consistent_dma_mask(dev->pdev,
> +                                                        
> DMA_32BIT_MASK)) {
>                        printk(KERN_INFO"%s%d: DMA mask set failed,  
> 64 Bit DAC disabled\n",
>                                dev->name, dev->id);
> +                       if (dev->adapter_info.options &  
> AAC_OPT_SGMAP_HOST64) {
> +                               printk(KERN_INFO "%s%d: 64-bit DAC  
> required "
> +                                       "for this host, bailing out 
> \n",
> +                                       dev->name, dev->id);
> +                               rcode = -ENOMEM;
> +                       }
>                        dev->dac_support = 0;
>                } else {
>                        printk(KERN_WARNING"%s%d: No suitable DMA  
> available.\n",
> @@ -1398,11 +1399,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
>         * Deal with configuring for the individualized limits of  
> each packet
>         * interface.
>         */
> -       dev->a_ops.adapter_scsi = (dev->dac_support)
> -         ? ((aac_get_driver_ident(dev->cardtype)->quirks &  
> AAC_QUIRK_SCSI_32)
> -                               ? aac_scsi_32_64
> -                               : aac_scsi_64)
> -                               : aac_scsi_32;
> +       dev->a_ops.adapter_scsi = dev->dac_support ? aac_scsi_64 :  
> aac_scsi_32;
>        if (dev->raw_io_interface) {
>                dev->a_ops.adapter_bounds = (dev->raw_io_64)
>                                        ? aac_bounds_64
>
> --
> Intel are signing my paycheques ... these opinions are still mine
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."

Re: [PATCH] Stop using num_physpages in aacraid

[Matthew Wilcox]

On Fri, May 02, 2008 at 03:06:42PM -0400, Mark Salyzyn wrote:
> The num_physpages variable works as needed for the PERC controllers  
> that are installed in vanilla x86 machines is it not? The PERC card  
> would not be installed in any other arch.
> 
> AAC_QUIRK_SCSI_32 means the card can not send 64bit format commands to  
> the SCSI devices (non DASD) but can send 64bit format commands to the  
> logical (Array) devices. This is for a select set of old PERC cards.
> 
> AAC_OPT_SGMAP_HOST64 means this card 'can' do DAC (send 64 bit format  
> commands to both SCSI and to Array devices), but sadly some that  
> report this are borken (hence the Quirk AAC_QUIRK_SCSI_32).
> 
> Dropping the cards is not an option, that is the whole reason this  
> workaround was put in place.

Thanks for the explanation of the quirks and options.  I don't want to
drop support for any cards, I just want to make aacraid not rely on
the meaning of num_physpages.

The question aacraid wants to ask is "Does this device have to
do DAC to access memory?"  The answer can be affected by an IOMMU or
by memory layout.  It's far from clear what question num_physpages is
intended to answer; there are different bits of the kernel using it in
interesting ways.  But we do have a function designed to answer the
question you want to ask: dma_get_required_mask().  This may be an
expensive question to ask, so in the below patch I cache the answer in
the aac_dev.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 460d402..cd0865d 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1201,9 +1201,8 @@ static int aac_scsi_32(struct fib * fib, struct scsi_cmnd * cmd)
 
 static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
 {
-	if ((sizeof(dma_addr_t) > 4) &&
-	 (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
-	 (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
+	if ((sizeof(dma_addr_t) > 4) && fib->dev->needs_dac &&
+	    (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
 		return FAILED;
 	return aac_scsi_32(fib, cmd);
 }
@@ -1394,6 +1393,9 @@ int aac_get_adapter_info(struct aac_dev* dev)
 			rcode = -ENOMEM;
 		}
 	}
+	if (dma_get_required_mask(&dev->pdev->dev) > DMA_32BIT_MASK)
+		dev->needs_dac = 1;
+
 	/*
 	 * Deal with configuring for the individualized limits of each packet
 	 * interface.
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 113ca9c..a06c817 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -1016,6 +1016,7 @@ struct aac_dev
 	u8			jbod;
 	u8			cache_protected;
 	u8			dac_support;
+	u8			needs_dac;
 	u8			raid_scsi_mode;
 	u8			comm_interface;
 #	define AAC_COMM_PRODUCER 0

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Stop using num_physpages in aacraid

[Mark Salyzyn]

Perfect! ACK!

Thanks for the suggestion.

Sincerely -- Mark Salyzyn

On May 3, 2008, at 6:51 AM, Matthew Wilcox wrote:

> . . .
> Thanks for the explanation of the quirks and options.  I don't want to
> drop support for any cards, I just want to make aacraid not rely on
> the meaning of num_physpages.
>
> The question aacraid wants to ask is "Does this device have to
> do DAC to access memory?"  The answer can be affected by an IOMMU or
> by memory layout.  It's far from clear what question num_physpages is
> intended to answer; there are different bits of the kernel using it in
> interesting ways.  But we do have a function designed to answer the
> question you want to ask: dma_get_required_mask().  This may be an
> expensive question to ask, so in the below patch I cache the answer in
> the aac_dev.
>
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
>
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/ 
> aachba.c
> index 460d402..cd0865d 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -1201,9 +1201,8 @@ static int aac_scsi_32(struct fib * fib,  
> struct scsi_cmnd * cmd)
>
> static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
> {
> -       if ((sizeof(dma_addr_t) > 4) &&
> -        (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
> -        (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
> +       if ((sizeof(dma_addr_t) > 4) && fib->dev->needs_dac &&
> +           (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
>    return FAILED;
> return aac_scsi_32(fib, cmd);
> }
> @@ -1394,6 +1393,9 @@ int aac_get_adapter_info(struct aac_dev* dev)
>            rcode = -ENOMEM;
>    }
> }
> +       if (dma_get_required_mask(&dev->pdev->dev) > DMA_32BIT_MASK)
> +               dev->needs_dac = 1;
> +
> /*
> * Deal with configuring for the individualized limits of each packet
> * interface.
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/ 
> aacraid.h
> index 113ca9c..a06c817 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -1016,6 +1016,7 @@ struct aac_dev
> u8                      jbod;
> u8                      cache_protected;
> u8                      dac_support;
> +       u8                      needs_dac;
> u8                      raid_scsi_mode;
> u8                      comm_interface;
> #      define AAC_COMM_PRODUCER 0

Re: [PATCH] Stop using num_physpages in aacraid

[James Bottomley]

On Sat, 2008-05-03 at 04:51 -0600, Matthew Wilcox wrote:
> On Fri, May 02, 2008 at 03:06:42PM -0400, Mark Salyzyn wrote:
> > The num_physpages variable works as needed for the PERC controllers  
> > that are installed in vanilla x86 machines is it not? The PERC card  
> > would not be installed in any other arch.
> > 
> > AAC_QUIRK_SCSI_32 means the card can not send 64bit format commands to  
> > the SCSI devices (non DASD) but can send 64bit format commands to the  
> > logical (Array) devices. This is for a select set of old PERC cards.
> > 
> > AAC_OPT_SGMAP_HOST64 means this card 'can' do DAC (send 64 bit format  
> > commands to both SCSI and to Array devices), but sadly some that  
> > report this are borken (hence the Quirk AAC_QUIRK_SCSI_32).
> > 
> > Dropping the cards is not an option, that is the whole reason this  
> > workaround was put in place.
> 
> Thanks for the explanation of the quirks and options.  I don't want to
> drop support for any cards, I just want to make aacraid not rely on
> the meaning of num_physpages.
> 
> The question aacraid wants to ask is "Does this device have to
> do DAC to access memory?"  The answer can be affected by an IOMMU or
> by memory layout.  It's far from clear what question num_physpages is
> intended to answer; there are different bits of the kernel using it in
> interesting ways.  But we do have a function designed to answer the
> question you want to ask: dma_get_required_mask().  This may be an
> expensive question to ask, so in the below patch I cache the answer in
> the aac_dev.

Actually, I think we can do a bit better than this.  We really only want
to enable 64 bit fibs if they're actually needed, so we should be
checking the dma_get_required_mask at all places the driver currently
looks for sizeof(dma_addr_t) > 4.

The attached, I think is cleaner (it also explains what this quirk is
for and calls the variable something more consonant with its function).

James


---

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index aa4e77c..15e60cb 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1206,9 +1206,7 @@ static int aac_scsi_32(struct fib * fib, struct scsi_cmnd * cmd)
 
 static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
 {
-	if ((sizeof(dma_addr_t) > 4) &&
-	 (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
-	 (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
+	if (fib->dev->no_scsi_64)
 		return FAILED;
 	return aac_scsi_32(fib, cmd);
 }
@@ -1372,11 +1370,23 @@ int aac_get_adapter_info(struct aac_dev* dev)
 		printk(KERN_INFO "%s%d: Non-DASD support enabled.\n",dev->name, dev->id);
 
 	dev->dac_support = 0;
-	if( (sizeof(dma_addr_t) > 4) && (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)){
+	if (dma_get_required_mask(&dev->pdev->dev) > DMA_32BIT_MASK
+	    && (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)){
 		if (!dev->in_reset)
 			printk(KERN_INFO "%s%d: 64bit support enabled.\n",
 				dev->name, dev->id);
 		dev->dac_support = 1;
+
+		/*
+		 * Adapters with AAC_QUIRK_SCSI_32 can only send pass
+		 * through commands as 32 bit fibs (although they can
+		 * send RAID commands as 64 bit fibs) flag these here
+		 * so we fail direct SCSI commands in the special
+		 * aac_scsi_32_64 routine
+		 */
+		if (aac_get_driver_ident(dev->cardtype)->quirks
+		    & AAC_QUIRK_SCSI_32)
+			dev->no_scsi_64 = 1;
 	}
 
 	if(dacmode != -1) {
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 73916ad..39070c5 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -1020,6 +1020,7 @@ struct aac_dev
 	u8			jbod;
 	u8			cache_protected;
 	u8			dac_support;
+	u8			no_scsi_64;
 	u8			raid_scsi_mode;
 	u8			comm_interface;
 #	define AAC_COMM_PRODUCER 0

Re: [PATCH] Stop using num_physpages in aacraid

[Mark Salyzyn]

ACK

with comment, I had chosen to perform the sizeof check as it optimized  
out the code on the 32 bit platforms and could have merged the  
aac_scsi_32_64 and aac_scsi_32 functions (such considerations are my  
penance for writing peephole optimizers and BIOS code in C/C++). Never  
sacrifice clarity/maintainability for optimization if you can afford it.

Sincerely -- Mark Salyzyn

On May 8, 2008, at 5:18 PM, James Bottomley wrote:

> On Sat, 2008-05-03 at 04:51 -0600, Matthew Wilcox wrote:
>> On Fri, May 02, 2008 at 03:06:42PM -0400, Mark Salyzyn wrote:
>>> The num_physpages variable works as needed for the PERC controllers
>>> that are installed in vanilla x86 machines is it not? The PERC card
>>> would not be installed in any other arch.
>>>
>>> AAC_QUIRK_SCSI_32 means the card can not send 64bit format  
>>> commands to
>>> the SCSI devices (non DASD) but can send 64bit format commands to  
>>> the
>>> logical (Array) devices. This is for a select set of old PERC cards.
>>>
>>> AAC_OPT_SGMAP_HOST64 means this card 'can' do DAC (send 64 bit  
>>> format
>>> commands to both SCSI and to Array devices), but sadly some that
>>> report this are borken (hence the Quirk AAC_QUIRK_SCSI_32).
>>>
>>> Dropping the cards is not an option, that is the whole reason this
>>> workaround was put in place.
>>
>> Thanks for the explanation of the quirks and options.  I don't want  
>> to
>> drop support for any cards, I just want to make aacraid not rely on
>> the meaning of num_physpages.
>>
>> The question aacraid wants to ask is "Does this device have to
>> do DAC to access memory?"  The answer can be affected by an IOMMU or
>> by memory layout.  It's far from clear what question num_physpages is
>> intended to answer; there are different bits of the kernel using it  
>> in
>> interesting ways.  But we do have a function designed to answer the
>> question you want to ask: dma_get_required_mask().  This may be an
>> expensive question to ask, so in the below patch I cache the answer  
>> in
>> the aac_dev.
>
> Actually, I think we can do a bit better than this.  We really only  
> want
> to enable 64 bit fibs if they're actually needed, so we should be
> checking the dma_get_required_mask at all places the driver currently
> looks for sizeof(dma_addr_t) > 4.
>
> The attached, I think is cleaner (it also explains what this quirk is
> for and calls the variable something more consonant with its  
> function).
>
> James
>
>
> ---
>
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/ 
> aachba.c
> index aa4e77c..15e60cb 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -1206,9 +1206,7 @@ static int aac_scsi_32(struct fib * fib,  
> struct scsi_cmnd * cmd)
>
> static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
> {
> -       if ((sizeof(dma_addr_t) > 4) &&
> -        (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
> -        (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
> +       if (fib->dev->no_scsi_64)
>                return FAILED;
>        return aac_scsi_32(fib, cmd);
> }
> @@ -1372,11 +1370,23 @@ int aac_get_adapter_info(struct aac_dev* dev)
>                printk(KERN_INFO "%s%d: Non-DASD support enabled. 
> \n",dev->name, dev->id);
>
>        dev->dac_support = 0;
> -       if( (sizeof(dma_addr_t) > 4) && (dev->adapter_info.options &  
> AAC_OPT_SGMAP_HOST64)){
> +       if (dma_get_required_mask(&dev->pdev->dev) > DMA_32BIT_MASK
> +           && (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)){
>                if (!dev->in_reset)
>                        printk(KERN_INFO "%s%d: 64bit support enabled. 
> \n",
>                                dev->name, dev->id);
>                dev->dac_support = 1;
> +
> +               /*
> +                * Adapters with AAC_QUIRK_SCSI_32 can only send pass
> +                * through commands as 32 bit fibs (although they can
> +                * send RAID commands as 64 bit fibs) flag these here
> +                * so we fail direct SCSI commands in the special
> +                * aac_scsi_32_64 routine
> +                */
> +               if (aac_get_driver_ident(dev->cardtype)->quirks
> +                   & AAC_QUIRK_SCSI_32)
> +                       dev->no_scsi_64 = 1;
>        }
>
>        if(dacmode != -1) {
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/ 
> aacraid.h
> index 73916ad..39070c5 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -1020,6 +1020,7 @@ struct aac_dev
>        u8                      jbod;
>        u8                      cache_protected;
>        u8                      dac_support;
> +       u8                      no_scsi_64;
>        u8                      raid_scsi_mode;
>        u8                      comm_interface;
> #      define AAC_COMM_PRODUCER 0
>
>

Re: [PATCH] Stop using num_physpages in aacraid

[James Bottomley]

On Fri, 2008-05-09 at 02:39 -0400, Mark Salyzyn wrote:
> ACK
> 
> with comment, I had chosen to perform the sizeof check as it optimized  
> out the code on the 32 bit platforms and could have merged the  
> aac_scsi_32_64 and aac_scsi_32 functions (such considerations are my  
> penance for writing peephole optimizers and BIOS code in C/C++). Never  
> sacrifice clarity/maintainability for optimization if you can afford it.

Yes ... I wish there were some way of getting the 

dma_get_required_mask() > DMA_32BIT_MASK

to compile out in the 32 bit case.  I'll think about it.


Another thing to note is that your problem could be solved if we could
adjust the queue mask per device (you want a 64 bit mask for the raid
devices and a 32 bit one for the physical ones).  This is analagous to
the ATA MDMA problem with ATA/ATAPI (the protocol can only do 32 bits
with ATAPI but the full 64 bits with ATA).  I'll see if we can use this
as a case to drive a fix for that.

James

Re: [PATCH] Stop using num_physpages in aacraid

[James Bottomley]

On Fri, 2008-05-09 at 02:39 -0400, Mark Salyzyn wrote:
> with comment, I had chosen to perform the sizeof check as it optimized  
> out the code on the 32 bit platforms and could have merged the  
> aac_scsi_32_64 and aac_scsi_32 functions (such considerations are my  
> penance for writing peephole optimizers and BIOS code in C/C++). Never  
> sacrifice clarity/maintainability for optimization if you can afford it.

Actually, point taken on the queuecommand check and the lesson in
optimisation.  This should actually be a better optimised version,
getting rid of the check altogether.

James

---

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: [SCSI] aacraid: don't use num_physpages

aacraid uses num_physpages as a check to see if it needs to fail
commands to physical drives which can only be sent as 32 bit fibs for
certain cards (those with AAC_QUIRK_SCSI_32 set).  The correct API for
this is dma_get_required_mask(), so make it use that.  Also tidy up
the 64 bit fib enabling code so we only enable it if the kernel is 64
bit and the dma mask goes over 32 bits (i.e. it has memory above 4GB).

Acked-by: Mark Salyzyn <Mark_Salyzyn@adaptec.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/aacraid/aachba.c |   37 +++++++++++++++++++++++--------------
 1 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index aa4e77c..2cc2c09 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1204,13 +1204,14 @@ static int aac_scsi_32(struct fib * fib, struct scsi_cmnd * cmd)
 				  (fib_callback) aac_srb_callback, (void *) cmd);
 }
 
+/*
+ * This routine is only used if we have memory > 4GB and the adapter
+ * is one of the quirky ones that can't do dma above 4GB to physical
+ * disks
+ */
 static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
 {
-	if ((sizeof(dma_addr_t) > 4) &&
-	 (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
-	 (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
-		return FAILED;
-	return aac_scsi_32(fib, cmd);
+	return FAILED;
 }
 
 int aac_get_adapter_info(struct aac_dev* dev)
@@ -1372,16 +1373,29 @@ int aac_get_adapter_info(struct aac_dev* dev)
 		printk(KERN_INFO "%s%d: Non-DASD support enabled.\n",dev->name, dev->id);
 
 	dev->dac_support = 0;
-	if( (sizeof(dma_addr_t) > 4) && (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)){
+	dev->a_ops.adapter_scsi = aac_scsi_32;
+	if ((dma_get_required_mask(&dev->pdev->dev) > DMA_32BIT_MASK
+	     && (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)
+	     && dacmode == -1) ||
+	    dacmode == 1) {
 		if (!dev->in_reset)
 			printk(KERN_INFO "%s%d: 64bit support enabled.\n",
 				dev->name, dev->id);
 		dev->dac_support = 1;
-	}
+		dev->a_ops.adapter_scsi = aac_scsi_64;
 
-	if(dacmode != -1) {
-		dev->dac_support = (dacmode!=0);
+		/*
+		 * Adapters with AAC_QUIRK_SCSI_32 can only send pass
+		 * through commands as 32 bit fibs (although they can
+		 * send RAID commands as 64 bit fibs) flag these here
+		 * so we fail direct SCSI commands in the special
+		 * aac_scsi_32_64 routine
+		 */
+		if (aac_get_driver_ident(dev->cardtype)->quirks
+		    & AAC_QUIRK_SCSI_32)
+			dev->a_ops.adapter_scsi = aac_scsi_32_64;
 	}
+
 	if(dev->dac_support != 0) {
 		if (!pci_set_dma_mask(dev->pdev, DMA_64BIT_MASK) &&
 			!pci_set_consistent_dma_mask(dev->pdev, DMA_64BIT_MASK)) {
@@ -1403,11 +1417,6 @@ int aac_get_adapter_info(struct aac_dev* dev)
 	 * Deal with configuring for the individualized limits of each packet
 	 * interface.
 	 */
-	dev->a_ops.adapter_scsi = (dev->dac_support)
-	  ? ((aac_get_driver_ident(dev->cardtype)->quirks & AAC_QUIRK_SCSI_32)
-				? aac_scsi_32_64
-				: aac_scsi_64)
-				: aac_scsi_32;
 	if (dev->raw_io_interface) {
 		dev->a_ops.adapter_bounds = (dev->raw_io_64)
 					? aac_bounds_64
-- 
1.5.4.1

Re: [PATCH] Stop using num_physpages in aacraid

[Mark Salyzyn]

As a follow up:

diff -ru a/aachba.c b/aachba.c
--- a/aachba.c  2008-05-02 15:35:39.227304669 -0400
+++ b/aachba.c  2008-05-02 15:36:07.450684444 -0400
@@ -1207,7 +1207,6 @@
  static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
  {
         if ((sizeof(dma_addr_t) > 4) &&
-        (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
          (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
                 return FAILED;
         return aac_scsi_32(fib, cmd);

will turn off NON-DASD (Tapes, CDROMS etc) support, and still enable  
64 bit I/O on 64 bit operating systems to the Arrays on the defective  
controllers. At least with the code before this change, we would allow  
NON-DASD support if the memory did not hit the 32 bit boundary even if  
it was a 64 bit OS.

One can switch to 32 bit I/O to the Arrays on 64 bit operating systems  
and keep NON-DASD support:

diff -ru a/aachba.c b/aachba.c
--- a/aachba.c  2008-05-02 15:35:39.227304669 -0400
+++ b/aachba.c  2008-05-02 15:42:08.261039455 -0400
@@ -1204,15 +1204,6 @@
                                   (fib_callback) aac_srb_callback,  
(void *) cmd);
  }

-static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
-{
-       if ((sizeof(dma_addr_t) > 4) &&
-        (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
-        (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
-               return FAILED;
-       return aac_scsi_32(fib, cmd);
-}
-
  int aac_get_adapter_info(struct aac_dev* dev)
  {
         struct fib* fibptr;
@@ -1382,6 +1373,8 @@
         if(dacmode != -1) {
                 dev->dac_support = (dacmode!=0);
         }
+       if (aac_get_driver_ident(dev->cardtype)->quirks &  
AAC_QUIRK_SCSI_32)
+               dev->dac_support = 0;
         if(dev->dac_support != 0) {
                 if (!pci_set_dma_mask(dev->pdev, DMA_64BIT_MASK) &&
                         !pci_set_consistent_dma_mask(dev->pdev,  
DMA_64BIT_MASK)) {
@@ -1404,10 +1397,7 @@
          * interface.
          */
         dev->a_ops.adapter_scsi = (dev->dac_support)
-         ? ((aac_get_driver_ident(dev->cardtype)->quirks &  
AAC_QUIRK_SCSI_32)
-                               ? aac_scsi_32_64
-                               : aac_scsi_64)
-                               : aac_scsi_32;
+                               ? aac_scsi_64 : aac_scsi_32;
         if (dev->raw_io_interface) {
                 dev->a_ops.adapter_bounds = (dev->raw_io_64)
                                         ? aac_bounds_64

But you can not have BOTH, choose performance on Arrays, or NON-DASD  
functionality.

Sincerely -- Mark Salyzyn

On May 2, 2008, at 2:52 PM, Matthew Wilcox wrote:

>
> The num_physpages variable has an interesting and varied definition
> depending on the architecture and various CONFIG options, not to  
> mention
> whether memory has been hotplugged or not.  At any rate, it doesn't do
> what aacraid is trying to use it for -- to determine whether a dma  
> will
> be 64-bit.
>
> This patch removes the aac_scsi_32_64() function altogether, favouring
> a failure to initialise over failing every command sent to the device.
>
> The various quirks, options and flags are quite confusing.  I think  
> I've
> managed to get it right, but I have no hardware to test with.
>
> My understanding:
>        AAC_QUIRK_SCSI_32 means "This card cannot address above 64-bit.
>        But it does support the 64-bit format."
>
>        AAC_OPT_SGMAP_HOST64 means "This card MUST do DAC".
>
> The patch below is intended to reflect that understanding.  If I've
> misunderstood the old code, or that wasn't what the old code was  
> supposed
> to mean then the below patch is wrong.
>
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
>
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/ 
> aachba.c
> index 460d402..95e524a 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -1199,15 +1199,6 @@ static int aac_scsi_32(struct fib * fib,  
> struct scsi_cmnd * cmd)
>                                  (fib_callback) aac_srb_callback,  
> (void *) cmd);
> }
>
> -static int aac_scsi_32_64(struct fib * fib, struct scsi_cmnd * cmd)
> -{
> -       if ((sizeof(dma_addr_t) > 4) &&
> -        (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) &&
> -        (fib->dev->adapter_info.options & AAC_OPT_SGMAP_HOST64))
> -               return FAILED;
> -       return aac_scsi_32(fib, cmd);
> -}
> -
> int aac_get_adapter_info(struct aac_dev* dev)
> {
>        struct fib* fibptr;
> @@ -1367,26 +1358,36 @@ int aac_get_adapter_info(struct aac_dev* dev)
>                printk(KERN_INFO "%s%d: Non-DASD support enabled. 
> \n",dev->name, dev->id);
>
>        dev->dac_support = 0;
> -       if( (sizeof(dma_addr_t) > 4) && (dev->adapter_info.options &  
> AAC_OPT_SGMAP_HOST64)){
> +       if ((sizeof(dma_addr_t) > 4) &&
> +           (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)) {
>                if (!dev->in_reset)
>                        printk(KERN_INFO "%s%d: 64bit support enabled. 
> \n",
>                                dev->name, dev->id);
>                dev->dac_support = 1;
>        }
>
> -       if(dacmode != -1) {
> +       if (dacmode != -1) {
>                dev->dac_support = (dacmode!=0);
>        }
> -       if(dev->dac_support != 0) {
> +       if (dev->dac_support &&
> +           !(aac_get_driver_ident(dev->cardtype)->quirks &
> +                                                        
> AAC_QUIRK_SCSI_32)) {
>                if (!pci_set_dma_mask(dev->pdev, DMA_64BIT_MASK) &&
> -                       !pci_set_consistent_dma_mask(dev->pdev,  
> DMA_64BIT_MASK)) {
> +                   !pci_set_consistent_dma_mask(dev->pdev,  
> DMA_64BIT_MASK)) {
>                        if (!dev->in_reset)
>                                printk(KERN_INFO"%s%d: 64 Bit DAC  
> enabled\n",
>                                        dev->name, dev->id);
>                } else if (!pci_set_dma_mask(dev->pdev,  
> DMA_32BIT_MASK) &&
> -                       !pci_set_consistent_dma_mask(dev->pdev,  
> DMA_32BIT_MASK)) {
> +                          !pci_set_consistent_dma_mask(dev->pdev,
> +                                                        
> DMA_32BIT_MASK)) {
>                        printk(KERN_INFO"%s%d: DMA mask set failed,  
> 64 Bit DAC disabled\n",
>                                dev->name, dev->id);
> +                       if (dev->adapter_info.options &  
> AAC_OPT_SGMAP_HOST64) {
> +                               printk(KERN_INFO "%s%d: 64-bit DAC  
> required "
> +                                       "for this host, bailing out 
> \n",
> +                                       dev->name, dev->id);
> +                               rcode = -ENOMEM;
> +                       }
>                        dev->dac_support = 0;
>                } else {
>                        printk(KERN_WARNING"%s%d: No suitable DMA  
> available.\n",
> @@ -1398,11 +1399,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
>         * Deal with configuring for the individualized limits of  
> each packet
>         * interface.
>         */
> -       dev->a_ops.adapter_scsi = (dev->dac_support)
> -         ? ((aac_get_driver_ident(dev->cardtype)->quirks &  
> AAC_QUIRK_SCSI_32)
> -                               ? aac_scsi_32_64
> -                               : aac_scsi_64)
> -                               : aac_scsi_32;
> +       dev->a_ops.adapter_scsi = dev->dac_support ? aac_scsi_64 :  
> aac_scsi_32;
>        if (dev->raw_io_interface) {
>                dev->a_ops.adapter_bounds = (dev->raw_io_64)
>                                        ? aac_bounds_64
>
> --
> Intel are signing my paycheques ... these opinions are still mine
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."