Re: [PATCH 5/9] scsi: megaraid_sas - Add memory support required by 0x73 controller

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 17 Feb 2009 08:37:00 -0800
"Yang, Bo" <Bo.Yang@lsi.com> wrote:

> Add memory support required by 0x73 controller
> 
> ...
> 
> +static int
> +megasas_skinny_mem_alloc(struct megasas_instance *instance)
> +{
> +       u8              i = 0, mem_alloc_done = 0;
> +       u32             *virt, ind, num_bk = 0;
> +       dma_addr_t      paddr;
> +       u32              mem_alloc_size = MEGASAS_SKINNY_SZ_MEM_BK * 256;
> +       u32             *mem_tb = instance->skinny_mm_tb;
> +
> +       instance->skinny_mm_tb = pci_alloc_consistent(instance->pdev,
> +                                MEGASAS_SKINNY_MAX_NUM_MEM_BK*sizeof(u32),
> +                                &instance->skinny_mm_tb_h);
> +
> +       memset(instance->skinny_mm_tb, 0,
> +               MEGASAS_SKINNY_MAX_NUM_MEM_BK * sizeof(u32));

This will crash the kernel if pci_alloc_consistent() failed.

> +       instance->skinny_mm_alloc_ind = 0;
> +
> +       while (!mem_alloc_done) {
> +
> +                       virt = pci_alloc_consistent(instance->pdev,
> +                                                        mem_alloc_size,
> +                                                        &paddr);

This code would be less of a mess if this block wasn't using 16-column
indenting.

Also, the entire patch appears to be using spacespacespacespace for
indenting.  Kernel code should use hard tabs.


> +                       if (virt) {
> +                               num_bk = (mem_alloc_size/
> +                                               MEGASAS_SKINNY_SZ_MEM_BK);
> +
> +                               for (ind = 0; ind < num_bk; ind++) {
> +                                       if (instance->skinny_mm_alloc_ind <
> +                                               MEGASAS_SKINNY_MAX_NUM_MEM_BK) {
> +                                               *mem_tb = (u32)(paddr+
> +                                               (MEGASAS_SKINNY_SZ_MEM_BK*ind));
> +                                               mem_tb++;
> +                                               instance->skinny_mm_alloc_ind++;

geeze that is hard to read.

> +                                       } else {
> +                                               mem_alloc_done = 1;
> +                                       }
> +                               }
> +
> +                               instance->skinny_mm_bk[i].vir   = virt;
> +                               instance->skinny_mm_bk[i].paddr = paddr;
> +                               instance->skinny_mm_bk[i].sz = mem_alloc_size;
> +                       } else {
> +                               if (mem_alloc_size > MEGASAS_SKINNY_SZ_MEM_BK)
> +                                       mem_alloc_size = mem_alloc_size / 2;
> +                       }
> +       }

The problem with this sort of decreasing-allocation-size loop is that
the earlier, large-sized allocations will generate large stack traces
from within the page allocator if they fail.

Normally we would just suppress those expected-to-happen failures with
__GFP_NOWARN.  But thanks to pci_alloc_consistent() interface
deficiencies, this cannot be done here.

> +       if (instance->skinny_mm_alloc_ind == 0)
> +               return -1;
> +
> +       return 0;
> +}
> +