Stop using num_physpages in aacraid

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 <[email protected]>

drivers/scsi/aacraid/aachba.c +0 -9

index 460d402..95e524a 100644
@@ -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;

drivers/scsi/aacraid/aachba.c +7 -4

@@ -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)) {

drivers/scsi/aacraid/aachba.c +2 -0

+                          !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",

drivers/scsi/aacraid/aachba.c +3 -0

+                                       dev->name, dev->id);
+                               rcode = -ENOMEM;
+                       }
                       dev->dac_support = 0;
               } else {
                       printk(KERN_WARNING"%s%d: No suitable DMA  

available.\n",

drivers/scsi/aacraid/aachba.c +0 -0

@@ -1398,11 +1399,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
        * Deal with configuring for the individualized limits of  

each packet
* interface.
*/

drivers/scsi/aacraid/aachba.c +0 -2

-       dev->a_ops.adapter_scsi = (dev->dac_support)
-         ? ((aac_get_driver_ident(dev->cardtype)->quirks &  

AAC_QUIRK_SCSI_32)

drivers/scsi/aacraid/aachba.c +1 -3

-                               ? 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."

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 <[email protected]>

drivers/scsi/aacraid/aachba.c +2 -3

index 460d402..cd0865d 100644
@@ -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);

}

drivers/scsi/aacraid/aachba.c +3 -0

@@ -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.

drivers/scsi/aacraid/aacraid.h +1 -0

index 113ca9c..a06c817 100644
@@ -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

Quoted 8 lines · 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.

Quoted 6 lines

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.

Quoted 20 lines · Matthew Wilcox

Quoted 8 lines · 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.

Quoted 6 lines

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.

Quoted 8 lines

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

---

drivers/scsi/aacraid/aachba.c +1 -3

index aa4e77c..15e60cb 100644
@@ -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);

}

drivers/scsi/aacraid/aachba.c +13 -1

@@ -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) {

drivers/scsi/aacraid/aacraid.h +1 -0

index 73916ad..39070c5 100644
@@ -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

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.

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.

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 <[email protected]>

drivers/scsi/aacraid/aachba.c +0 -9

index 460d402..95e524a 100644
@@ -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;

drivers/scsi/aacraid/aachba.c +7 -4

@@ -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)) {

drivers/scsi/aacraid/aachba.c +2 -0

+                          !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",

drivers/scsi/aacraid/aachba.c +3 -0

+                                       dev->name, dev->id);
+                               rcode = -ENOMEM;
+                       }
                       dev->dac_support = 0;
               } else {
                       printk(KERN_WARNING"%s%d: No suitable DMA  

available.\n",

drivers/scsi/aacraid/aachba.c +0 -0

@@ -1398,11 +1399,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
        * Deal with configuring for the individualized limits of  

each packet
* interface.
*/

drivers/scsi/aacraid/aachba.c +0 -2

-       dev->a_ops.adapter_scsi = (dev->dac_support)
-         ? ((aac_get_driver_ident(dev->cardtype)->quirks &  

AAC_QUIRK_SCSI_32)

drivers/scsi/aacraid/aachba.c +1 -3

-                               ? 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."