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

Andrew Morton
2009-02-19T00:30:37Z

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;
> +}
> +