On Mon, Aug 05, 2013 at 10:05:56AM +0300, Alexander Shishkin wrote:
> Peter Chen <peter.chen@xxxxxxxxxxxxx> writes:
>
> > - The old operation needs to call hw_alloc_regmap two times, and
> > the first operation is only used to know if the controller is
> > lpm supported, besides, there is a kfree before kzalloc, it is also
> > tricky.
>
> Why tricky? kfree() is called from either NULL or a valid pointer, both
> of which is perfectly valid.
>
I know it is valid, it is just strange that you call kfree before kmalloc.
> > - Fix the memory leak for ci->hw_bank.regmap when unload module.
>
> > ci->hw_bank.op = ci->hw_bank.cap + (ioread32(ci->hw_bank.cap) & 0xff);
> >
> > - hw_alloc_regmap(ci, false);
> > - reg = hw_read(ci, CAP_HCCPARAMS, HCCPARAMS_LEN) >>
> > - __ffs(HCCPARAMS_LEN);
> > - ci->hw_bank.lpm = reg;
> > - hw_alloc_regmap(ci, !!reg);
> > + ci->hw_bank.lpm = !!(ioread32(ci->hw_bank.cap +
> > + ci_regs_nolpm[CAP_HCCPARAMS]) & HCCPARAMS_LEN);
> > + hw_alloc_regmap(ci, ci->hw_bank.lpm);
>
> The idea of the original code is to avoid this ioread and have all the
> register offset maths done by hw_read(), since it's done there
> anyway. And it kind of looks better to me the way it's done right
> now. So unless you really want to optimize away one call to
> hw_alloc_regmap(), I'd prefer to keep it as it is.
>
I do this just because it is strange that calling hw_alloc_regmap
two times at the first glance, and the first time is just to know
if it is lpm supported, besides, calling kfree before kmalloc is a
strange thing, don't you think so?
> > @@ -602,6 +598,7 @@ static int ci_hdrc_remove(struct platform_device *pdev)
> > {
> > struct ci_hdrc *ci = platform_get_drvdata(pdev);
> >
> > + kfree(ci->hw_bank.regmap);
>
> This one seems perfectly valid.
>
> Regards,
> --
> Alex
>
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
