Messages in this thread | | | Date | Fri, 20 Oct 2017 15:54:06 +0200 | From | Greg KH <> | Subject | Re: [PATCH 01/12] nvmem: imx-iim: use stack for nvmem_config instead of malloc'ing it |
| |
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...
> > 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?
thanks,
greg k-h
| |