diff mbox series

mtd: lpddr: fix excessive stack usage with clang

Message ID 20200505140136.263461-1-arnd@arndb.de
State New
Headers show
Series mtd: lpddr: fix excessive stack usage with clang | expand

Commit Message

Arnd Bergmann May 5, 2020, 2:01 p.m. UTC 
Building lpddr2_nvm with clang can result in a giant stack usage
in one function:

drivers/mtd/lpddr/lpddr2_nvm.c:399:12: error: stack frame size of 1144 bytes in function 'lpddr2_nvm_probe' [-Werror,-Wframe-larger-than=]

The problem is that clang decides to build a copy of the mtd_info
structure on the stack and then do a memcpy() into the actual version. It
shouldn't really do it that way, but it's not strictly a bug either.

As a workaround, use a static const version of the structure to assign
most of the members upfront and then only set the few members that
require runtime knowledge at probe time.

Fixes: 96ba9dd65788 ("mtd: lpddr: add driver for LPDDR2-NVM PCM memories")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 drivers/mtd/lpddr/lpddr2_nvm.c | 35 ++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

-- 
2.26.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Comments

Nathan Chancellor May 6, 2020, 2:38 a.m. UTC | #1 
On Tue, May 05, 2020 at 04:01:16PM +0200, Arnd Bergmann wrote:
> Building lpddr2_nvm with clang can result in a giant stack usage

> in one function:

> 

> drivers/mtd/lpddr/lpddr2_nvm.c:399:12: error: stack frame size of 1144 bytes in function 'lpddr2_nvm_probe' [-Werror,-Wframe-larger-than=]

> 

> The problem is that clang decides to build a copy of the mtd_info

> structure on the stack and then do a memcpy() into the actual version. It

> shouldn't really do it that way, but it's not strictly a bug either.

> 

> As a workaround, use a static const version of the structure to assign

> most of the members upfront and then only set the few members that

> require runtime knowledge at probe time.

> 

> Fixes: 96ba9dd65788 ("mtd: lpddr: add driver for LPDDR2-NVM PCM memories")

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>


Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>


> ---

>  drivers/mtd/lpddr/lpddr2_nvm.c | 35 ++++++++++++++++++----------------

>  1 file changed, 19 insertions(+), 16 deletions(-)

> 

> diff --git a/drivers/mtd/lpddr/lpddr2_nvm.c b/drivers/mtd/lpddr/lpddr2_nvm.c

> index 0f1547f09d08..72f5c7b30079 100644

> --- a/drivers/mtd/lpddr/lpddr2_nvm.c

> +++ b/drivers/mtd/lpddr/lpddr2_nvm.c

> @@ -393,6 +393,17 @@ static int lpddr2_nvm_lock(struct mtd_info *mtd, loff_t start_add,

>  	return lpddr2_nvm_do_block_op(mtd, start_add, len, LPDDR2_NVM_LOCK);

>  }

>  

> +static const struct mtd_info lpddr2_nvm_mtd_info = {

> +	.type		= MTD_RAM,

> +	.writesize	= 1,

> +	.flags		= (MTD_CAP_NVRAM | MTD_POWERUP_LOCK),

> +	._read		= lpddr2_nvm_read,

> +	._write		= lpddr2_nvm_write,

> +	._erase		= lpddr2_nvm_erase,

> +	._unlock	= lpddr2_nvm_unlock,

> +	._lock		= lpddr2_nvm_lock,

> +};

> +

>  /*

>   * lpddr2_nvm driver probe method

>   */

> @@ -433,6 +444,7 @@ static int lpddr2_nvm_probe(struct platform_device *pdev)

>  		.pfow_base	= OW_BASE_ADDRESS,

>  		.fldrv_priv	= pcm_data,

>  	};

> +

>  	if (IS_ERR(map->virt))

>  		return PTR_ERR(map->virt);

>  

> @@ -444,22 +456,13 @@ static int lpddr2_nvm_probe(struct platform_device *pdev)

>  		return PTR_ERR(pcm_data->ctl_regs);

>  

>  	/* Populate mtd_info data structure */

> -	*mtd = (struct mtd_info) {

> -		.dev		= { .parent = &pdev->dev },

> -		.name		= pdev->dev.init_name,

> -		.type		= MTD_RAM,

> -		.priv		= map,

> -		.size		= resource_size(add_range),

> -		.erasesize	= ERASE_BLOCKSIZE * pcm_data->bus_width,

> -		.writesize	= 1,

> -		.writebufsize	= WRITE_BUFFSIZE * pcm_data->bus_width,

> -		.flags		= (MTD_CAP_NVRAM | MTD_POWERUP_LOCK),

> -		._read		= lpddr2_nvm_read,

> -		._write		= lpddr2_nvm_write,

> -		._erase		= lpddr2_nvm_erase,

> -		._unlock	= lpddr2_nvm_unlock,

> -		._lock		= lpddr2_nvm_lock,

> -	};

> +	*mtd = lpddr2_nvm_mtd_info;

> +	mtd->dev.parent		= &pdev->dev;

> +	mtd->name		= pdev->dev.init_name;

> +	mtd->priv		= map;

> +	mtd->size		= resource_size(add_range);

> +	mtd->erasesize		= ERASE_BLOCKSIZE * pcm_data->bus_width;

> +	mtd->writebufsize	= WRITE_BUFFSIZE * pcm_data->bus_width;

>  

>  	/* Verify the presence of the device looking for PFOW string */

>  	if (!lpddr2_nvm_pfow_present(map)) {

> -- 

> 2.26.0

> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Miquel Raynal May 7, 2020, 10:21 a.m. UTC | #2 
Nathan Chancellor <natechancellor@gmail.com> wrote on Tue, 5 May 2020
19:38:28 -0700:

> On Tue, May 05, 2020 at 04:01:16PM +0200, Arnd Bergmann wrote:

> > Building lpddr2_nvm with clang can result in a giant stack usage

> > in one function:

> > 

> > drivers/mtd/lpddr/lpddr2_nvm.c:399:12: error: stack frame size of 1144 bytes in function 'lpddr2_nvm_probe' [-Werror,-Wframe-larger-than=]

> > 

> > The problem is that clang decides to build a copy of the mtd_info

> > structure on the stack and then do a memcpy() into the actual version. It

> > shouldn't really do it that way, but it's not strictly a bug either.

> > 

> > As a workaround, use a static const version of the structure to assign

> > most of the members upfront and then only set the few members that

> > require runtime knowledge at probe time.

> > 

> > Fixes: 96ba9dd65788 ("mtd: lpddr: add driver for LPDDR2-NVM PCM memories")

> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>  

> 

> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>


Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>


> 

> > ---

> >  drivers/mtd/lpddr/lpddr2_nvm.c | 35 ++++++++++++++++++----------------

> >  1 file changed, 19 insertions(+), 16 deletions(-)

> > 

> > diff --git a/drivers/mtd/lpddr/lpddr2_nvm.c b/drivers/mtd/lpddr/lpddr2_nvm.c

> > index 0f1547f09d08..72f5c7b30079 100644

> > --- a/drivers/mtd/lpddr/lpddr2_nvm.c

> > +++ b/drivers/mtd/lpddr/lpddr2_nvm.c

> > @@ -393,6 +393,17 @@ static int lpddr2_nvm_lock(struct mtd_info *mtd, loff_t start_add,

> >  	return lpddr2_nvm_do_block_op(mtd, start_add, len, LPDDR2_NVM_LOCK);

> >  }

> >  

> > +static const struct mtd_info lpddr2_nvm_mtd_info = {

> > +	.type		= MTD_RAM,

> > +	.writesize	= 1,

> > +	.flags		= (MTD_CAP_NVRAM | MTD_POWERUP_LOCK),

> > +	._read		= lpddr2_nvm_read,

> > +	._write		= lpddr2_nvm_write,

> > +	._erase		= lpddr2_nvm_erase,

> > +	._unlock	= lpddr2_nvm_unlock,

> > +	._lock		= lpddr2_nvm_lock,

> > +};

> > +

> >  /*

> >   * lpddr2_nvm driver probe method

> >   */

> > @@ -433,6 +444,7 @@ static int lpddr2_nvm_probe(struct platform_device *pdev)

> >  		.pfow_base	= OW_BASE_ADDRESS,

> >  		.fldrv_priv	= pcm_data,

> >  	};

> > +

> >  	if (IS_ERR(map->virt))

> >  		return PTR_ERR(map->virt);

> >  

> > @@ -444,22 +456,13 @@ static int lpddr2_nvm_probe(struct platform_device *pdev)

> >  		return PTR_ERR(pcm_data->ctl_regs);

> >  

> >  	/* Populate mtd_info data structure */

> > -	*mtd = (struct mtd_info) {

> > -		.dev		= { .parent = &pdev->dev },

> > -		.name		= pdev->dev.init_name,

> > -		.type		= MTD_RAM,

> > -		.priv		= map,

> > -		.size		= resource_size(add_range),

> > -		.erasesize	= ERASE_BLOCKSIZE * pcm_data->bus_width,

> > -		.writesize	= 1,

> > -		.writebufsize	= WRITE_BUFFSIZE * pcm_data->bus_width,

> > -		.flags		= (MTD_CAP_NVRAM | MTD_POWERUP_LOCK),

> > -		._read		= lpddr2_nvm_read,

> > -		._write		= lpddr2_nvm_write,

> > -		._erase		= lpddr2_nvm_erase,

> > -		._unlock	= lpddr2_nvm_unlock,

> > -		._lock		= lpddr2_nvm_lock,

> > -	};

> > +	*mtd = lpddr2_nvm_mtd_info;

> > +	mtd->dev.parent		= &pdev->dev;

> > +	mtd->name		= pdev->dev.init_name;

> > +	mtd->priv		= map;

> > +	mtd->size		= resource_size(add_range);

> > +	mtd->erasesize		= ERASE_BLOCKSIZE * pcm_data->bus_width;

> > +	mtd->writebufsize	= WRITE_BUFFSIZE * pcm_data->bus_width;

> >  

> >  	/* Verify the presence of the device looking for PFOW string */

> >  	if (!lpddr2_nvm_pfow_present(map)) {

> > -- 

> > 2.26.0

> >   


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Miquel Raynal Sept. 7, 2020, 7:34 a.m. UTC | #3 
On Tue, 2020-05-05 at 14:01:16 UTC, Arnd Bergmann wrote:
> Building lpddr2_nvm with clang can result in a giant stack usage

> in one function:

> 

> drivers/mtd/lpddr/lpddr2_nvm.c:399:12: error: stack frame size of 1144 bytes in function 'lpddr2_nvm_probe' [-Werror,-Wframe-larger-than=]

> 

> The problem is that clang decides to build a copy of the mtd_info

> structure on the stack and then do a memcpy() into the actual version. It

> shouldn't really do it that way, but it's not strictly a bug either.

> 

> As a workaround, use a static const version of the structure to assign

> most of the members upfront and then only set the few members that

> require runtime knowledge at probe time.

> 

> Fixes: 96ba9dd65788 ("mtd: lpddr: add driver for LPDDR2-NVM PCM memories")

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>


Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
diff mbox series

Patch

diff --git a/drivers/mtd/lpddr/lpddr2_nvm.c b/drivers/mtd/lpddr/lpddr2_nvm.c
index 0f1547f09d08..72f5c7b30079 100644
--- a/drivers/mtd/lpddr/lpddr2_nvm.c
+++ b/drivers/mtd/lpddr/lpddr2_nvm.c
@@ -393,6 +393,17 @@  static int lpddr2_nvm_lock(struct mtd_info *mtd, loff_t start_add,
 	return lpddr2_nvm_do_block_op(mtd, start_add, len, LPDDR2_NVM_LOCK);
 }
 
+static const struct mtd_info lpddr2_nvm_mtd_info = {
+	.type		= MTD_RAM,
+	.writesize	= 1,
+	.flags		= (MTD_CAP_NVRAM | MTD_POWERUP_LOCK),
+	._read		= lpddr2_nvm_read,
+	._write		= lpddr2_nvm_write,
+	._erase		= lpddr2_nvm_erase,
+	._unlock	= lpddr2_nvm_unlock,
+	._lock		= lpddr2_nvm_lock,
+};
+
 /*
  * lpddr2_nvm driver probe method
  */
@@ -433,6 +444,7 @@  static int lpddr2_nvm_probe(struct platform_device *pdev)
 		.pfow_base	= OW_BASE_ADDRESS,
 		.fldrv_priv	= pcm_data,
 	};
+
 	if (IS_ERR(map->virt))
 		return PTR_ERR(map->virt);
 
@@ -444,22 +456,13 @@  static int lpddr2_nvm_probe(struct platform_device *pdev)
 		return PTR_ERR(pcm_data->ctl_regs);
 
 	/* Populate mtd_info data structure */
-	*mtd = (struct mtd_info) {
-		.dev		= { .parent = &pdev->dev },
-		.name		= pdev->dev.init_name,
-		.type		= MTD_RAM,
-		.priv		= map,
-		.size		= resource_size(add_range),
-		.erasesize	= ERASE_BLOCKSIZE * pcm_data->bus_width,
-		.writesize	= 1,
-		.writebufsize	= WRITE_BUFFSIZE * pcm_data->bus_width,
-		.flags		= (MTD_CAP_NVRAM | MTD_POWERUP_LOCK),
-		._read		= lpddr2_nvm_read,
-		._write		= lpddr2_nvm_write,
-		._erase		= lpddr2_nvm_erase,
-		._unlock	= lpddr2_nvm_unlock,
-		._lock		= lpddr2_nvm_lock,
-	};
+	*mtd = lpddr2_nvm_mtd_info;
+	mtd->dev.parent		= &pdev->dev;
+	mtd->name		= pdev->dev.init_name;
+	mtd->priv		= map;
+	mtd->size		= resource_size(add_range);
+	mtd->erasesize		= ERASE_BLOCKSIZE * pcm_data->bus_width;
+	mtd->writebufsize	= WRITE_BUFFSIZE * pcm_data->bus_width;
 
 	/* Verify the presence of the device looking for PFOW string */
 	if (!lpddr2_nvm_pfow_present(map)) {