Skip to contentSkip to conversation

dma: ste_dma40: Remove VLA usage

PATCH In review Kees Cook 5 participants 7 messages Jun 29, 2018 – Jul 3, 2018

Subsystems (2)
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 dma: ste_dma40: Remove VLA usage

In the quest to remove all stack VLA usage from the kernel[1], this switches to using a pre-allocated scratch register space, set up with all other other allocations.

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

---
 drivers/dma/ste_dma40.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/dma/ste_dma40.c +13 -2
index 1bc149af990e..f4edfc56f34e 100644
@@ -555,6 +555,7 @@ struct d40_gen_dmac {
  * @reg_val_backup_v4: Backup of registers that only exits on dma40 v3 and
  * later
  * @reg_val_backup_chan: Backup data for standard channel parameter registers.
+ * @regs_interrupt: Scratch space for registers during interrupt.
  * @gcc_pwr_off_mask: Mask to maintain the channels that can be turned off.
  * @gen_dmac: the struct for generic registers values to represent u8500/8540
  * DMA controller
@@ -592,6 +593,7 @@ struct d40_base {
 	u32				  reg_val_backup[BACKUP_REGS_SZ];
 	u32				  reg_val_backup_v4[BACKUP_REGS_SZ_MAX];
 	u32				 *reg_val_backup_chan;
+	u32				 *regs_interrupt;
 	u16				  gcc_pwr_off_mask;
 	struct d40_gen_dmac		  gen_dmac;
 };
@@ -1637,7 +1639,7 @@ static irqreturn_t d40_handle_interrupt(int irq, void *data)
 	struct d40_chan *d40c;
 	unsigned long flags;
 	struct d40_base *base = data;
-	u32 regs[base->gen_dmac.il_size];
+	u32 *regs = base->regs_interrupt;
 	struct d40_interrupt_lookup *il = base->gen_dmac.il;
 	u32 il_size = base->gen_dmac.il_size;
 
@@ -3258,13 +3260,22 @@ static struct d40_base * __init d40_hw_detect_init(struct platform_device *pdev)
 	if (!base->lcla_pool.alloc_map)
 		goto free_backup_chan;
 
+	base->regs_interrupt = kmalloc_array(base->gen_dmac.il_size,
+					     sizeof(*base->regs_interrupt),
+					     GFP_KERNEL);
+	if (!base->regs_interrupt)
+		goto free_map;
+
 	base->desc_slab = kmem_cache_create(D40_NAME, sizeof(struct d40_desc),
 					    0, SLAB_HWCACHE_ALIGN,
 					    NULL);
 	if (base->desc_slab == NULL)
-		goto free_map;
+		goto free_regs;
+
 
 	return base;
+ free_regs:
+	kfree(base->regs_interrupt);
  free_map:
 	kfree(base->lcla_pool.alloc_map);
  free_backup_chan:
Reviewed-by on v1 1/1
#1 lore raw
Quoted 4 lines · Kees Cook

In the quest to remove all stack VLA usage from the kernel[1], this switches to using a pre-allocated scratch register space, set up with all other other allocations.

Quoted 8 lines

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

I looked too long at this one, in short:

- Your change looks correct to me
- It could be done better in many ways, all of which would involve cleaning up some of the existing code in the process
- Nobody uses this driver in practice, as the hardware platform was a dead end

---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#5 lore raw
In reply to Kees Cook
Quoted 3 lines · Kees Cook

In the quest to remove all stack VLA usage from the kernel[1], this switches to using a pre-allocated scratch register space, set up with all other other allocations.

Applied after fixing subsystem tag, thanks

Reviewed-by on v1 1/1
#6 lore raw
In reply to Kees Cook
Quoted 4 lines · Kees Cook

In the quest to remove all stack VLA usage from the kernel[1], this switches to using a pre-allocated scratch register space, set up with all other other allocations.

Quoted 8 lines

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

Yours,
Linus Walleij
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
In reply to Arnd Bergmann
Quoted 2 lines · Arnd Bergmann

- Nobody uses this driver in practice, as the hardware platform was a dead end

Depends what you mean with dead end.

The hardware was deployed in a few million handsets from Samsung and Sony.

Some have been picked up as targets for PostmarketOS:
https://wiki.postmarketos.org/wiki/Samsung_Galaxy_S_Advance_(samsung-i9070)
https://wiki.postmarketos.org/wiki/Samsung_Galaxy_SIII_mini_(samsung-i8190)

So it is no more dead end than anything else discontinued, and it has a living community who want it suppored by the mainline kernel if possible.

Yours,
Linus Walleij
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 10 lines · Linus Walleij
Quoted 2 lines · Arnd Bergmann

- Nobody uses this driver in practice, as the hardware platform was a dead end

Depends what you mean with dead end.

The hardware was deployed in a few million handsets from Samsung and Sony.

Quoted 7 lines

Some have been picked up as targets for PostmarketOS:
https://wiki.postmarketos.org/wiki/Samsung_Galaxy_S_Advance_(samsung-i9070)
https://wiki.postmarketos.org/wiki/Samsung_Galaxy_SIII_mini_(samsung-i8190)

So it is no more dead end than anything else discontinued, and it has a living community who want it suppored by the mainline kernel if possible.

Mixup with SuperH-based ST40?

Gr{oetje,eeting}s,

Geert

#4 lore raw
Quoted 11 lines · Geert Uytterhoeven
Quoted 10 lines · Linus Walleij
Quoted 2 lines · Arnd Bergmann

- Nobody uses this driver in practice, as the hardware platform was a dead end

Depends what you mean with dead end.

The hardware was deployed in a few million handsets from Samsung and Sony.

Quoted 7 lines
Quoted 7 lines

Some have been picked up as targets for PostmarketOS:
https://wiki.postmarketos.org/wiki/Samsung_Galaxy_S_Advance_(samsung-i9070)
https://wiki.postmarketos.org/wiki/Samsung_Galaxy_SIII_mini_(samsung-i8190)

So it is no more dead end than anything else discontinued, and it has a living community who want it suppored by the mainline kernel if possible.

I meant that when ST-Ericsson imploded, STMicroelectronics inherited the SoC family but never did any follow-up chips. We only really ever supported the original U8500 but not the later models (which have partial support), and that is very unlikely to change, so all the devices that can run this driver are already
known. This is only relevant because the dynamic allocation size depends on the chip model.

Quoted 1 line

Mixup with SuperH-based ST40?

No, but I wouldn't be surprised to find an ST40 inside the D40 engine ;-)

Arnd
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html