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.
> - Fix the memory leak for ci->hw_bank.regmap when unload module.
That's a good one.
> Signed-off-by: Peter Chen <peter.chen@xxxxxxxxxxxxx>
> ---
> drivers/usb/chipidea/ci.h | 2 +-
> drivers/usb/chipidea/core.c | 11 ++++-------
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 1c94fc5..e5cb585 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -92,7 +92,7 @@ struct ci_role_driver {
> * @regmap: register lookup table
> */
> struct hw_bank {
> - unsigned lpm;
> + bool lpm;
> resource_size_t phys;
> void __iomem *abs;
> void __iomem *cap;
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 135f7f9..8d7a600 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -123,8 +123,6 @@ static int hw_alloc_regmap(struct ci_hdrc *ci, bool is_lpm)
> {
> int i;
>
> - kfree(ci->hw_bank.regmap);
> -
> ci->hw_bank.regmap = kzalloc((OP_LAST + 1) * sizeof(void *),
> GFP_KERNEL);
> if (!ci->hw_bank.regmap)
> @@ -183,11 +181,9 @@ static int hw_device_init(struct ci_hdrc *ci, void __iomem *base)
> ci->hw_bank.cap += ci->platdata->capoffset;
> 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.
> ci->hw_bank.size = ci->hw_bank.op - ci->hw_bank.abs;
> ci->hw_bank.size += OP_LAST;
> ci->hw_bank.size /= sizeof(u32);
> @@ -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
--
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
