Re: [PATCH] Stop using num_physpages in aacraid

[Mark Salyzyn <Mark_Salyzyn@adaptec.com>]

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