Messages in this thread | | | Date | Wed, 18 Feb 2009 16:30:02 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH 5/9] scsi: megaraid_sas - Add memory support required by 0x73 controller |
| |
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; > +} > +
| |