| Message ID | 20171009132641.27169-3-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | nvmem: patches set-1 for v4.15 | expand |
On Mon, Oct 09, 2017 at 03:26:31PM +0200, srinivas.kandagatla@linaro.org wrote: > From: Masahiro Yamada <yamada.masahiro@socionext.com> > > nvmem_register() copies all the members of nvmem_config to > nvmem_device. So, nvmem_config is one-time use data during > probing. There is no point to keep it until the driver detach. > Using stack should be no problem because nvmem_config is pretty > small. Same objection as previous patch, what is wrong with it as-is? thanks, greg k-h
2017-10-20 22:34 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: > On Mon, Oct 09, 2017 at 03:26:31PM +0200, srinivas.kandagatla@linaro.org wrote: >> From: Masahiro Yamada <yamada.masahiro@socionext.com> >> >> nvmem_register() copies all the members of nvmem_config to >> nvmem_device. So, nvmem_config is one-time use data during >> probing. There is no point to keep it until the driver detach. >> Using stack should be no problem because nvmem_config is pretty >> small. > > Same objection as previous patch, what is wrong with it as-is? > It is wasting memory. -- Best Regards Masahiro Yamada
On Fri, Oct 20, 2017 at 10:47:28PM +0900, Masahiro Yamada wrote: > 2017-10-20 22:34 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: > > On Mon, Oct 09, 2017 at 03:26:31PM +0200, srinivas.kandagatla@linaro.org wrote: > >> From: Masahiro Yamada <yamada.masahiro@socionext.com> > >> > >> nvmem_register() copies all the members of nvmem_config to > >> nvmem_device. So, nvmem_config is one-time use data during > >> probing. There is no point to keep it until the driver detach. > >> Using stack should be no problem because nvmem_config is pretty > >> small. > > > > Same objection as previous patch, what is wrong with it as-is? > > > > It is wasting memory. The memory is freed again, after the call, right? thanks, greg k-h
2017-10-20 22:54 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: > On Fri, Oct 20, 2017 at 10:47:28PM +0900, Masahiro Yamada wrote: >> 2017-10-20 22:34 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: >> > On Mon, Oct 09, 2017 at 03:26:31PM +0200, srinivas.kandagatla@linaro.org wrote: >> >> From: Masahiro Yamada <yamada.masahiro@socionext.com> >> >> >> >> nvmem_register() copies all the members of nvmem_config to >> >> nvmem_device. So, nvmem_config is one-time use data during >> >> probing. There is no point to keep it until the driver detach. >> >> Using stack should be no problem because nvmem_config is pretty >> >> small. >> > >> > Same objection as previous patch, what is wrong with it as-is? >> > >> >> It is wasting memory. > > The memory is freed again, after the call, right? > I assume your "as-is" meant devm_kzalloc(). The memory is freed when the driver is detached. It is generally quite long time that this struct occupies the memory for no good reason. -- Best Regards Masahiro Yamada
On Fri, Oct 20, 2017 at 10:59:45PM +0900, Masahiro Yamada wrote: > 2017-10-20 22:54 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: > > On Fri, Oct 20, 2017 at 10:47:28PM +0900, Masahiro Yamada wrote: > >> 2017-10-20 22:34 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: > >> > On Mon, Oct 09, 2017 at 03:26:31PM +0200, srinivas.kandagatla@linaro.org wrote: > >> >> From: Masahiro Yamada <yamada.masahiro@socionext.com> > >> >> > >> >> nvmem_register() copies all the members of nvmem_config to > >> >> nvmem_device. So, nvmem_config is one-time use data during > >> >> probing. There is no point to keep it until the driver detach. > >> >> Using stack should be no problem because nvmem_config is pretty > >> >> small. > >> > > >> > Same objection as previous patch, what is wrong with it as-is? > >> > > >> > >> It is wasting memory. > > > > The memory is freed again, after the call, right? > > > > I assume your "as-is" meant devm_kzalloc(). > > The memory is freed when the driver is detached. > > It is generally quite long time > that this struct occupies the memory for no good reason. Ah, ok, a simple kmalloc() might be better, but ok, if you are _sure_ it's ok taking that much stack space up... :) thanks, greg k-h
diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c index 32fd572e18c5..fa7a0f66b37e 100644 --- a/drivers/nvmem/mtk-efuse.c +++ b/drivers/nvmem/mtk-efuse.c @@ -49,7 +49,7 @@ static int mtk_efuse_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct resource *res; struct nvmem_device *nvmem; - struct nvmem_config *econfig; + struct nvmem_config econfig = {}; void __iomem *base; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -57,19 +57,15 @@ static int mtk_efuse_probe(struct platform_device *pdev) if (IS_ERR(base)) return PTR_ERR(base); - econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL); - if (!econfig) - return -ENOMEM; - - econfig->stride = 4; - econfig->word_size = 4; - econfig->reg_read = mtk_reg_read; - econfig->reg_write = mtk_reg_write; - econfig->size = resource_size(res); - econfig->priv = base; - econfig->dev = dev; - econfig->owner = THIS_MODULE; - nvmem = nvmem_register(econfig); + econfig.stride = 4; + econfig.word_size = 4; + econfig.reg_read = mtk_reg_read; + econfig.reg_write = mtk_reg_write; + econfig.size = resource_size(res); + econfig.priv = base; + econfig.dev = dev; + econfig.owner = THIS_MODULE; + nvmem = nvmem_register(&econfig); if (IS_ERR(nvmem)) return PTR_ERR(nvmem);