Messages in this thread | | | From | Masahiro Yamada <> | Date | Fri, 20 Oct 2017 23:26:16 +0900 | Subject | Re: [PATCH 01/12] nvmem: imx-iim: use stack for nvmem_config instead of malloc'ing it |
| |
2017-10-20 22:54 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: > On Fri, Oct 20, 2017 at 10:47:16PM +0900, Masahiro Yamada wrote: >> Hi Greg, >> >> 2017-10-20 22:32 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: >> > On Mon, Oct 09, 2017 at 03:26:30PM +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. >> >> >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> --- >> >> drivers/nvmem/imx-iim.c | 27 ++++++++++++--------------- >> >> 1 file changed, 12 insertions(+), 15 deletions(-) >> >> >> >> diff --git a/drivers/nvmem/imx-iim.c b/drivers/nvmem/imx-iim.c >> >> index 52ff65e0673f..a5992602709a 100644 >> >> --- a/drivers/nvmem/imx-iim.c >> >> +++ b/drivers/nvmem/imx-iim.c >> >> @@ -34,7 +34,6 @@ struct imx_iim_drvdata { >> >> struct iim_priv { >> >> void __iomem *base; >> >> struct clk *clk; >> >> - struct nvmem_config nvmem; >> >> }; >> >> >> >> static int imx_iim_read(void *context, unsigned int offset, >> >> @@ -108,7 +107,7 @@ static int imx_iim_probe(struct platform_device *pdev) >> >> struct resource *res; >> >> struct iim_priv *iim; >> >> struct nvmem_device *nvmem; >> >> - struct nvmem_config *cfg; >> >> + struct nvmem_config cfg = {}; >> > >> > You do realize you are now not zeroing out this structure, and have to >> > explicitly initialize all of the fields, right? >> >> Why? >> >> I am surely zeroing out the structure. >> >> Did you miss "= {};" in my code? > > Are you sure that does zero it out? I know we have had issues with this > in the past...
Do you have a reference for that?
All members that are not specified in the initializer are set to 0 (or NULL).
"git show c7836d1593b87cb813c58cf64e08b052ebbe2a78" and do you agree that this is correct?
>> > What is the real problem with doing a dynamic allocation for this? >> > Putting structures on the stack is a "bad idea" for all of the obvious >> > reasons (small stack in the kernel, initialized data, lower layers >> > expect it to be dma-able, etc.) >> >> >> Why is this a problem? >> >> Did you really understand this patch? >> >> - This structure is very small. >> struct uart_8250_port is five times bigger >> and it is allocated in the stack and it is fine. >> >> - All data are initialized. >> >> - Why DMA? >> Please do not exaggerate things by introducing unrelated topic. > > I just want you to realize the change, the initialized is the big thing. > > And keeping structures off of the stack is a good thing, if this is not > a performance issue, I suggest keeping it as-is, right? >
I do not see logical explanation in your comment.
The structure is initialized. Other subsystem use stack for such a small structure. Why is (devm_)kzalloc necessary?
-- Best Regards Masahiro Yamada
| |