On Sat, Jan 24, 2015 at 3:48 AM, Ong, Boon Leong <boon.leong....@intel.com> wrote:
>>+static int imr_enabled(struct imr_regs *imr) > Do we want to make it inline perhaps since it is 1 liner? Since it is declared static I would even suggest the new name is_imr_enabled(). [] >>+int imr_remove_range(int reg, unsigned long base, unsigned long size) >>+{ >>+ struct imr_regs imr; >>+ int found = 0, i, ret = 0; > Please make each of the defined variables as individual line here.. I would suggest to type i as unsigned int and found as bool. [] >>+ if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) { >>+ ret = -ENODEV; >>+ goto done; >>+ } >>+ found = 1; >>+ Redundant empty line. >>+ } else { >>+ /* Search for match based on address range */ >>+ for (i = 0; i < imr_dev.max_imr; i++) { >>+ ret = imr_read(reg, &imr); > A serious bug here.... 'reg' should be 'i' . We enter this branch if reg=-1 > Is there a miss in your test case? > >>+ if (ret) >>+ goto done; >>+ >>+ if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) >>+ continue; >>+ >>+ if ((imr_to_phys(imr.addr_lo) == base) && >>+ (imr_to_phys(imr.addr_hi) == max)) { > I think we need to take care of the size that has been fix-up here ... > >>+ found = 1; >>+ reg = i; According to your comment this line becomes redundant. [] >>+static void __init imr_fixup_memmap(void) >>+{ >>+ unsigned long base = virt_to_phys(&_text); >>+ unsigned long size = virt_to_phys(&__end_rodata) - base; > What about the size fixup to be consistent? > We should not guard more than it is needed ..... > >>+ int i, ret; > Two int declaration line here. unsigned int i; ? -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/