diff mbox series

tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers

Message ID 20190222124329.23046-1-noralf@tronnes.org (mailing list archive)
State New, archived
Headers show
Series tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers | expand

Commit Message

Noralf Trønnes Feb. 22, 2019, 12:43 p.m. UTC
Buffers passed to spi_sync() must be dma-safe even for tiny buffers since
some SPI controllers use DMA for all transfers.

Example splat with CONFIG_DMA_API_DEBUG enabled:

[   23.750467] DMA-API: dw_dmac_pci 0000:00:15.0: device driver maps memory from stack [probable addr=000000001e49185d]
[   23.750529] WARNING: CPU: 1 PID: 1296 at kernel/dma/debug.c:1161 check_for_stack+0xb7/0x190
[   23.750533] Modules linked in: mmc_block(+) spi_pxa2xx_platform(+) pwm_lpss_pci pwm_lpss spi_pxa2xx_pci sdhci_pci cqhci intel_mrfld_pwrbtn extcon_intel_mrfld sdhci intel_mrfld_adc led_class mmc_core ili9341 mipi_dbi tinydrm backlight ti_ads7950 industrialio_triggered_buffer kfifo_buf intel_soc_pmic_mrfld hci_uart btbcm
[   23.750599] CPU: 1 PID: 1296 Comm: modprobe Not tainted 5.0.0-rc7+ #236
[   23.750605] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
[   23.750620] RIP: 0010:check_for_stack+0xb7/0x190
[   23.750630] Code: 8b 6d 50 4d 85 ed 75 04 4c 8b 6d 10 48 89 ef e8 2f 8b 44 00 48 89 c6 4a 8d 0c 23 4c 89 ea 48 c7 c7 88 d0 82 b4 e8 40 7c f9 ff <0f> 0b 8b 05 79 00 4b 01 85 c0 74 07 5b 5d 41 5c 41 5d c3 8b 05 54
[   23.750637] RSP: 0000:ffff97bbc0292fa0 EFLAGS: 00010286
[   23.750646] RAX: 0000000000000000 RBX: ffff97bbc0290000 RCX: 0000000000000006
[   23.750652] RDX: 0000000000000007 RSI: 0000000000000002 RDI: ffff94b33e115450
[   23.750658] RBP: ffff94b33c8578b0 R08: 0000000000000002 R09: 00000000000201c0
[   23.750664] R10: 00000006ecb0ccc6 R11: 0000000000034f38 R12: 000000000000316c
[   23.750670] R13: ffff94b33c84b250 R14: ffff94b33dedd5a0 R15: 0000000000000001
[   23.750679] FS:  0000000000000000(0000) GS:ffff94b33e100000(0063) knlGS:00000000f7faf690
[   23.750686] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[   23.750691] CR2: 00000000f7f54faf CR3: 000000000722c000 CR4: 00000000001006e0
[   23.750696] Call Trace:
[   23.750713]  debug_dma_map_sg+0x100/0x340
[   23.750727]  ? dma_direct_map_sg+0x3b/0xb0
[   23.750739]  spi_map_buf+0x25a/0x300
[   23.750751]  __spi_pump_messages+0x2a4/0x680
[   23.750762]  __spi_sync+0x1dd/0x1f0
[   23.750773]  spi_sync+0x26/0x40
[   23.750790]  mipi_dbi_typec3_command_read+0x14d/0x240 [mipi_dbi]
[   23.750802]  ? spi_finalize_current_transfer+0x10/0x10
[   23.750821]  mipi_dbi_typec3_command+0x1bc/0x1d0 [mipi_dbi]

Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tinydrm/ili9225.c  |  6 ++--
 drivers/gpu/drm/tinydrm/mipi-dbi.c | 58 +++++++++++++++++++++---------
 include/drm/tinydrm/mipi-dbi.h     |  5 +--
 3 files changed, 48 insertions(+), 21 deletions(-)

Comments

Andy Shevchenko Feb. 22, 2019, 3:58 p.m. UTC | #1
On Fri, Feb 22, 2019 at 01:43:29PM +0100, Noralf Trønnes wrote:
> Buffers passed to spi_sync() must be dma-safe even for tiny buffers since
> some SPI controllers use DMA for all transfers.
> 
> Example splat with CONFIG_DMA_API_DEBUG enabled:
> 
> [   23.750467] DMA-API: dw_dmac_pci 0000:00:15.0: device driver maps memory from stack [probable addr=000000001e49185d]
> [   23.750529] WARNING: CPU: 1 PID: 1296 at kernel/dma/debug.c:1161 check_for_stack+0xb7/0x190
> [   23.750533] Modules linked in: mmc_block(+) spi_pxa2xx_platform(+) pwm_lpss_pci pwm_lpss spi_pxa2xx_pci sdhci_pci cqhci intel_mrfld_pwrbtn extcon_intel_mrfld sdhci intel_mrfld_adc led_class mmc_core ili9341 mipi_dbi tinydrm backlight ti_ads7950 industrialio_triggered_buffer kfifo_buf intel_soc_pmic_mrfld hci_uart btbcm
> [   23.750599] CPU: 1 PID: 1296 Comm: modprobe Not tainted 5.0.0-rc7+ #236
> [   23.750605] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
> [   23.750620] RIP: 0010:check_for_stack+0xb7/0x190
> [   23.750630] Code: 8b 6d 50 4d 85 ed 75 04 4c 8b 6d 10 48 89 ef e8 2f 8b 44 00 48 89 c6 4a 8d 0c 23 4c 89 ea 48 c7 c7 88 d0 82 b4 e8 40 7c f9 ff <0f> 0b 8b 05 79 00 4b 01 85 c0 74 07 5b 5d 41 5c 41 5d c3 8b 05 54
> [   23.750637] RSP: 0000:ffff97bbc0292fa0 EFLAGS: 00010286
> [   23.750646] RAX: 0000000000000000 RBX: ffff97bbc0290000 RCX: 0000000000000006
> [   23.750652] RDX: 0000000000000007 RSI: 0000000000000002 RDI: ffff94b33e115450
> [   23.750658] RBP: ffff94b33c8578b0 R08: 0000000000000002 R09: 00000000000201c0
> [   23.750664] R10: 00000006ecb0ccc6 R11: 0000000000034f38 R12: 000000000000316c
> [   23.750670] R13: ffff94b33c84b250 R14: ffff94b33dedd5a0 R15: 0000000000000001
> [   23.750679] FS:  0000000000000000(0000) GS:ffff94b33e100000(0063) knlGS:00000000f7faf690
> [   23.750686] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> [   23.750691] CR2: 00000000f7f54faf CR3: 000000000722c000 CR4: 00000000001006e0
> [   23.750696] Call Trace:
> [   23.750713]  debug_dma_map_sg+0x100/0x340
> [   23.750727]  ? dma_direct_map_sg+0x3b/0xb0
> [   23.750739]  spi_map_buf+0x25a/0x300
> [   23.750751]  __spi_pump_messages+0x2a4/0x680
> [   23.750762]  __spi_sync+0x1dd/0x1f0
> [   23.750773]  spi_sync+0x26/0x40
> [   23.750790]  mipi_dbi_typec3_command_read+0x14d/0x240 [mipi_dbi]
> [   23.750802]  ? spi_finalize_current_transfer+0x10/0x10
> [   23.750821]  mipi_dbi_typec3_command+0x1bc/0x1d0 [mipi_dbi]
> 

After few runs I don't see the warning anymore. So,
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Though I would like to give few more days to monitor the state.
(I believe it's fixed)

> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/tinydrm/ili9225.c  |  6 ++--
>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 58 +++++++++++++++++++++---------
>  include/drm/tinydrm/mipi-dbi.h     |  5 +--
>  3 files changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
> index 3f59cfbd31ba..a87078667e04 100644
> --- a/drivers/gpu/drm/tinydrm/ili9225.c
> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> @@ -299,7 +299,7 @@ static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
>  	mipi->enabled = false;
>  }
>  
> -static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
> +static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
>  			       size_t num)
>  {
>  	struct spi_device *spi = mipi->spi;
> @@ -309,11 +309,11 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
>  
>  	gpiod_set_value_cansleep(mipi->dc, 0);
>  	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
> -	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
>  	if (ret || !num)
>  		return ret;
>  
> -	if (cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
> +	if (*cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
>  		bpw = 16;
>  
>  	gpiod_set_value_cansleep(mipi->dc, 1);
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index 246e708a9ff7..4a4cd7e72938 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -153,16 +153,42 @@ EXPORT_SYMBOL(mipi_dbi_command_read);
>   */
>  int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
>  {
> +	u8 *cmdbuf;
>  	int ret;
>  
> +	/* SPI requires dma-safe buffers */
> +	cmdbuf = kmemdup(&cmd, 1, GFP_KERNEL);
> +	if (!cmdbuf)
> +		return -ENOMEM;
> +
>  	mutex_lock(&mipi->cmdlock);
> -	ret = mipi->command(mipi, cmd, data, len);
> +	ret = mipi->command(mipi, cmdbuf, data, len);
>  	mutex_unlock(&mipi->cmdlock);
>  
> +	kfree(cmdbuf);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(mipi_dbi_command_buf);
>  
> +/* This should only be used by mipi_dbi_command() */
> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
> +{
> +	u8 *buf;
> +	int ret;
> +
> +	buf = kmemdup(data, len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = mipi_dbi_command_buf(mipi, cmd, buf, len);
> +
> +	kfree(buf);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
> +
>  /**
>   * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
>   * @dst: The destination buffer
> @@ -772,18 +798,18 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *mipi, int dc,
>  	return 0;
>  }
>  
> -static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
> +static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 *cmd,
>  				   u8 *parameters, size_t num)
>  {
> -	unsigned int bpw = (cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
> +	unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
>  	int ret;
>  
> -	if (mipi_dbi_command_is_read(mipi, cmd))
> +	if (mipi_dbi_command_is_read(mipi, *cmd))
>  		return -ENOTSUPP;
>  
> -	MIPI_DBI_DEBUG_COMMAND(cmd, parameters, num);
> +	MIPI_DBI_DEBUG_COMMAND(*cmd, parameters, num);
>  
> -	ret = mipi_dbi_spi1_transfer(mipi, 0, &cmd, 1, 8);
> +	ret = mipi_dbi_spi1_transfer(mipi, 0, cmd, 1, 8);
>  	if (ret || !num)
>  		return ret;
>  
> @@ -792,7 +818,7 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
>  
>  /* MIPI DBI Type C Option 3 */
>  
> -static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
> +static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 *cmd,
>  					u8 *data, size_t len)
>  {
>  	struct spi_device *spi = mipi->spi;
> @@ -801,7 +827,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>  	struct spi_transfer tr[2] = {
>  		{
>  			.speed_hz = speed_hz,
> -			.tx_buf = &cmd,
> +			.tx_buf = cmd,
>  			.len = 1,
>  		}, {
>  			.speed_hz = speed_hz,
> @@ -819,8 +845,8 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>  	 * Support non-standard 24-bit and 32-bit Nokia read commands which
>  	 * start with a dummy clock, so we need to read an extra byte.
>  	 */
> -	if (cmd == MIPI_DCS_GET_DISPLAY_ID ||
> -	    cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
> +	if (*cmd == MIPI_DCS_GET_DISPLAY_ID ||
> +	    *cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
>  		if (!(len == 3 || len == 4))
>  			return -EINVAL;
>  
> @@ -850,7 +876,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>  			data[i] = (buf[i] << 1) | !!(buf[i + 1] & BIT(7));
>  	}
>  
> -	MIPI_DBI_DEBUG_COMMAND(cmd, data, len);
> +	MIPI_DBI_DEBUG_COMMAND(*cmd, data, len);
>  
>  err_free:
>  	kfree(buf);
> @@ -858,7 +884,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>  	return ret;
>  }
>  
> -static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
> +static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd,
>  				   u8 *par, size_t num)
>  {
>  	struct spi_device *spi = mipi->spi;
> @@ -866,18 +892,18 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>  	u32 speed_hz;
>  	int ret;
>  
> -	if (mipi_dbi_command_is_read(mipi, cmd))
> +	if (mipi_dbi_command_is_read(mipi, *cmd))
>  		return mipi_dbi_typec3_command_read(mipi, cmd, par, num);
>  
> -	MIPI_DBI_DEBUG_COMMAND(cmd, par, num);
> +	MIPI_DBI_DEBUG_COMMAND(*cmd, par, num);
>  
>  	gpiod_set_value_cansleep(mipi->dc, 0);
>  	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
> -	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
>  	if (ret || !num)
>  		return ret;
>  
> -	if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
>  		bpw = 16;
>  
>  	gpiod_set_value_cansleep(mipi->dc, 1);
> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
> index ad7e6bedab5f..464a47d1013d 100644
> --- a/include/drm/tinydrm/mipi-dbi.h
> +++ b/include/drm/tinydrm/mipi-dbi.h
> @@ -43,7 +43,7 @@ struct mipi_dbi {
>  	struct spi_device *spi;
>  	bool enabled;
>  	struct mutex cmdlock;
> -	int (*command)(struct mipi_dbi *mipi, u8 cmd, u8 *param, size_t num);
> +	int (*command)(struct mipi_dbi *mipi, u8 *cmd, u8 *param, size_t num);
>  	const u8 *read_commands;
>  	struct gpio_desc *dc;
>  	u16 *tx_buf;
> @@ -83,6 +83,7 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
>  
>  int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
>  int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
>  int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  		      struct drm_rect *clip, bool swap);
>  /**
> @@ -100,7 +101,7 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  #define mipi_dbi_command(mipi, cmd, seq...) \
>  ({ \
>  	u8 d[] = { seq }; \
> -	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
> +	mipi_dbi_command_stackbuf(mipi, cmd, d, ARRAY_SIZE(d)); \
>  })
>  
>  #ifdef CONFIG_DEBUG_FS
> -- 
> 2.20.1
>
Noralf Trønnes March 4, 2019, 2:45 p.m. UTC | #2
Den 22.02.2019 16.58, skrev Andy Shevchenko:
> On Fri, Feb 22, 2019 at 01:43:29PM +0100, Noralf Trønnes wrote:
>> Buffers passed to spi_sync() must be dma-safe even for tiny buffers since
>> some SPI controllers use DMA for all transfers.
>>
>> Example splat with CONFIG_DMA_API_DEBUG enabled:
>>
>> [   23.750467] DMA-API: dw_dmac_pci 0000:00:15.0: device driver maps memory from stack [probable addr=000000001e49185d]
>> [   23.750529] WARNING: CPU: 1 PID: 1296 at kernel/dma/debug.c:1161 check_for_stack+0xb7/0x190
>> [   23.750533] Modules linked in: mmc_block(+) spi_pxa2xx_platform(+) pwm_lpss_pci pwm_lpss spi_pxa2xx_pci sdhci_pci cqhci intel_mrfld_pwrbtn extcon_intel_mrfld sdhci intel_mrfld_adc led_class mmc_core ili9341 mipi_dbi tinydrm backlight ti_ads7950 industrialio_triggered_buffer kfifo_buf intel_soc_pmic_mrfld hci_uart btbcm
>> [   23.750599] CPU: 1 PID: 1296 Comm: modprobe Not tainted 5.0.0-rc7+ #236
>> [   23.750605] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
>> [   23.750620] RIP: 0010:check_for_stack+0xb7/0x190
>> [   23.750630] Code: 8b 6d 50 4d 85 ed 75 04 4c 8b 6d 10 48 89 ef e8 2f 8b 44 00 48 89 c6 4a 8d 0c 23 4c 89 ea 48 c7 c7 88 d0 82 b4 e8 40 7c f9 ff <0f> 0b 8b 05 79 00 4b 01 85 c0 74 07 5b 5d 41 5c 41 5d c3 8b 05 54
>> [   23.750637] RSP: 0000:ffff97bbc0292fa0 EFLAGS: 00010286
>> [   23.750646] RAX: 0000000000000000 RBX: ffff97bbc0290000 RCX: 0000000000000006
>> [   23.750652] RDX: 0000000000000007 RSI: 0000000000000002 RDI: ffff94b33e115450
>> [   23.750658] RBP: ffff94b33c8578b0 R08: 0000000000000002 R09: 00000000000201c0
>> [   23.750664] R10: 00000006ecb0ccc6 R11: 0000000000034f38 R12: 000000000000316c
>> [   23.750670] R13: ffff94b33c84b250 R14: ffff94b33dedd5a0 R15: 0000000000000001
>> [   23.750679] FS:  0000000000000000(0000) GS:ffff94b33e100000(0063) knlGS:00000000f7faf690
>> [   23.750686] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
>> [   23.750691] CR2: 00000000f7f54faf CR3: 000000000722c000 CR4: 00000000001006e0
>> [   23.750696] Call Trace:
>> [   23.750713]  debug_dma_map_sg+0x100/0x340
>> [   23.750727]  ? dma_direct_map_sg+0x3b/0xb0
>> [   23.750739]  spi_map_buf+0x25a/0x300
>> [   23.750751]  __spi_pump_messages+0x2a4/0x680
>> [   23.750762]  __spi_sync+0x1dd/0x1f0
>> [   23.750773]  spi_sync+0x26/0x40
>> [   23.750790]  mipi_dbi_typec3_command_read+0x14d/0x240 [mipi_dbi]
>> [   23.750802]  ? spi_finalize_current_transfer+0x10/0x10
>> [   23.750821]  mipi_dbi_typec3_command+0x1bc/0x1d0 [mipi_dbi]
>>
> 
> After few runs I don't see the warning anymore. So,
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 

Can I have an ack as well if you're satisfied with it?

Noralf.

> Though I would like to give few more days to monitor the state.
> (I believe it's fixed)
> 
>> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>  drivers/gpu/drm/tinydrm/ili9225.c  |  6 ++--
>>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 58 +++++++++++++++++++++---------
>>  include/drm/tinydrm/mipi-dbi.h     |  5 +--
>>  3 files changed, 48 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
>> index 3f59cfbd31ba..a87078667e04 100644
>> --- a/drivers/gpu/drm/tinydrm/ili9225.c
>> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
>> @@ -299,7 +299,7 @@ static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
>>  	mipi->enabled = false;
>>  }
>>  
>> -static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
>> +static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
>>  			       size_t num)
>>  {
>>  	struct spi_device *spi = mipi->spi;
>> @@ -309,11 +309,11 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
>>  
>>  	gpiod_set_value_cansleep(mipi->dc, 0);
>>  	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
>> -	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
>> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
>>  	if (ret || !num)
>>  		return ret;
>>  
>> -	if (cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
>> +	if (*cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
>>  		bpw = 16;
>>  
>>  	gpiod_set_value_cansleep(mipi->dc, 1);
>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> index 246e708a9ff7..4a4cd7e72938 100644
>> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> @@ -153,16 +153,42 @@ EXPORT_SYMBOL(mipi_dbi_command_read);
>>   */
>>  int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
>>  {
>> +	u8 *cmdbuf;
>>  	int ret;
>>  
>> +	/* SPI requires dma-safe buffers */
>> +	cmdbuf = kmemdup(&cmd, 1, GFP_KERNEL);
>> +	if (!cmdbuf)
>> +		return -ENOMEM;
>> +
>>  	mutex_lock(&mipi->cmdlock);
>> -	ret = mipi->command(mipi, cmd, data, len);
>> +	ret = mipi->command(mipi, cmdbuf, data, len);
>>  	mutex_unlock(&mipi->cmdlock);
>>  
>> +	kfree(cmdbuf);
>> +
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL(mipi_dbi_command_buf);
>>  
>> +/* This should only be used by mipi_dbi_command() */
>> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
>> +{
>> +	u8 *buf;
>> +	int ret;
>> +
>> +	buf = kmemdup(data, len, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	ret = mipi_dbi_command_buf(mipi, cmd, buf, len);
>> +
>> +	kfree(buf);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>> +
>>  /**
>>   * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
>>   * @dst: The destination buffer
>> @@ -772,18 +798,18 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *mipi, int dc,
>>  	return 0;
>>  }
>>  
>> -static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
>> +static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 *cmd,
>>  				   u8 *parameters, size_t num)
>>  {
>> -	unsigned int bpw = (cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
>> +	unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
>>  	int ret;
>>  
>> -	if (mipi_dbi_command_is_read(mipi, cmd))
>> +	if (mipi_dbi_command_is_read(mipi, *cmd))
>>  		return -ENOTSUPP;
>>  
>> -	MIPI_DBI_DEBUG_COMMAND(cmd, parameters, num);
>> +	MIPI_DBI_DEBUG_COMMAND(*cmd, parameters, num);
>>  
>> -	ret = mipi_dbi_spi1_transfer(mipi, 0, &cmd, 1, 8);
>> +	ret = mipi_dbi_spi1_transfer(mipi, 0, cmd, 1, 8);
>>  	if (ret || !num)
>>  		return ret;
>>  
>> @@ -792,7 +818,7 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
>>  
>>  /* MIPI DBI Type C Option 3 */
>>  
>> -static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>> +static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 *cmd,
>>  					u8 *data, size_t len)
>>  {
>>  	struct spi_device *spi = mipi->spi;
>> @@ -801,7 +827,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>>  	struct spi_transfer tr[2] = {
>>  		{
>>  			.speed_hz = speed_hz,
>> -			.tx_buf = &cmd,
>> +			.tx_buf = cmd,
>>  			.len = 1,
>>  		}, {
>>  			.speed_hz = speed_hz,
>> @@ -819,8 +845,8 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>>  	 * Support non-standard 24-bit and 32-bit Nokia read commands which
>>  	 * start with a dummy clock, so we need to read an extra byte.
>>  	 */
>> -	if (cmd == MIPI_DCS_GET_DISPLAY_ID ||
>> -	    cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
>> +	if (*cmd == MIPI_DCS_GET_DISPLAY_ID ||
>> +	    *cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
>>  		if (!(len == 3 || len == 4))
>>  			return -EINVAL;
>>  
>> @@ -850,7 +876,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>>  			data[i] = (buf[i] << 1) | !!(buf[i + 1] & BIT(7));
>>  	}
>>  
>> -	MIPI_DBI_DEBUG_COMMAND(cmd, data, len);
>> +	MIPI_DBI_DEBUG_COMMAND(*cmd, data, len);
>>  
>>  err_free:
>>  	kfree(buf);
>> @@ -858,7 +884,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>>  	return ret;
>>  }
>>  
>> -static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>> +static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd,
>>  				   u8 *par, size_t num)
>>  {
>>  	struct spi_device *spi = mipi->spi;
>> @@ -866,18 +892,18 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>>  	u32 speed_hz;
>>  	int ret;
>>  
>> -	if (mipi_dbi_command_is_read(mipi, cmd))
>> +	if (mipi_dbi_command_is_read(mipi, *cmd))
>>  		return mipi_dbi_typec3_command_read(mipi, cmd, par, num);
>>  
>> -	MIPI_DBI_DEBUG_COMMAND(cmd, par, num);
>> +	MIPI_DBI_DEBUG_COMMAND(*cmd, par, num);
>>  
>>  	gpiod_set_value_cansleep(mipi->dc, 0);
>>  	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
>> -	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
>> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
>>  	if (ret || !num)
>>  		return ret;
>>  
>> -	if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
>> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
>>  		bpw = 16;
>>  
>>  	gpiod_set_value_cansleep(mipi->dc, 1);
>> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
>> index ad7e6bedab5f..464a47d1013d 100644
>> --- a/include/drm/tinydrm/mipi-dbi.h
>> +++ b/include/drm/tinydrm/mipi-dbi.h
>> @@ -43,7 +43,7 @@ struct mipi_dbi {
>>  	struct spi_device *spi;
>>  	bool enabled;
>>  	struct mutex cmdlock;
>> -	int (*command)(struct mipi_dbi *mipi, u8 cmd, u8 *param, size_t num);
>> +	int (*command)(struct mipi_dbi *mipi, u8 *cmd, u8 *param, size_t num);
>>  	const u8 *read_commands;
>>  	struct gpio_desc *dc;
>>  	u16 *tx_buf;
>> @@ -83,6 +83,7 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
>>  
>>  int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
>>  int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
>> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
>>  int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>>  		      struct drm_rect *clip, bool swap);
>>  /**
>> @@ -100,7 +101,7 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>>  #define mipi_dbi_command(mipi, cmd, seq...) \
>>  ({ \
>>  	u8 d[] = { seq }; \
>> -	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
>> +	mipi_dbi_command_stackbuf(mipi, cmd, d, ARRAY_SIZE(d)); \
>>  })
>>  
>>  #ifdef CONFIG_DEBUG_FS
>> -- 
>> 2.20.1
>>
>
Andy Shevchenko March 4, 2019, 3:10 p.m. UTC | #3
On Mon, Mar 04, 2019 at 03:45:56PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 22.02.2019 16.58, skrev Andy Shevchenko:
> > On Fri, Feb 22, 2019 at 01:43:29PM +0100, Noralf Trønnes wrote:
> >> Buffers passed to spi_sync() must be dma-safe even for tiny buffers since
> >> some SPI controllers use DMA for all transfers.
> >>
> >> Example splat with CONFIG_DMA_API_DEBUG enabled:
> >>
> >> [   23.750467] DMA-API: dw_dmac_pci 0000:00:15.0: device driver maps memory from stack [probable addr=000000001e49185d]
> >> [   23.750529] WARNING: CPU: 1 PID: 1296 at kernel/dma/debug.c:1161 check_for_stack+0xb7/0x190
> >> [   23.750533] Modules linked in: mmc_block(+) spi_pxa2xx_platform(+) pwm_lpss_pci pwm_lpss spi_pxa2xx_pci sdhci_pci cqhci intel_mrfld_pwrbtn extcon_intel_mrfld sdhci intel_mrfld_adc led_class mmc_core ili9341 mipi_dbi tinydrm backlight ti_ads7950 industrialio_triggered_buffer kfifo_buf intel_soc_pmic_mrfld hci_uart btbcm
> >> [   23.750599] CPU: 1 PID: 1296 Comm: modprobe Not tainted 5.0.0-rc7+ #236
> >> [   23.750605] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
> >> [   23.750620] RIP: 0010:check_for_stack+0xb7/0x190
> >> [   23.750630] Code: 8b 6d 50 4d 85 ed 75 04 4c 8b 6d 10 48 89 ef e8 2f 8b 44 00 48 89 c6 4a 8d 0c 23 4c 89 ea 48 c7 c7 88 d0 82 b4 e8 40 7c f9 ff <0f> 0b 8b 05 79 00 4b 01 85 c0 74 07 5b 5d 41 5c 41 5d c3 8b 05 54
> >> [   23.750637] RSP: 0000:ffff97bbc0292fa0 EFLAGS: 00010286
> >> [   23.750646] RAX: 0000000000000000 RBX: ffff97bbc0290000 RCX: 0000000000000006
> >> [   23.750652] RDX: 0000000000000007 RSI: 0000000000000002 RDI: ffff94b33e115450
> >> [   23.750658] RBP: ffff94b33c8578b0 R08: 0000000000000002 R09: 00000000000201c0
> >> [   23.750664] R10: 00000006ecb0ccc6 R11: 0000000000034f38 R12: 000000000000316c
> >> [   23.750670] R13: ffff94b33c84b250 R14: ffff94b33dedd5a0 R15: 0000000000000001
> >> [   23.750679] FS:  0000000000000000(0000) GS:ffff94b33e100000(0063) knlGS:00000000f7faf690
> >> [   23.750686] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> >> [   23.750691] CR2: 00000000f7f54faf CR3: 000000000722c000 CR4: 00000000001006e0
> >> [   23.750696] Call Trace:
> >> [   23.750713]  debug_dma_map_sg+0x100/0x340
> >> [   23.750727]  ? dma_direct_map_sg+0x3b/0xb0
> >> [   23.750739]  spi_map_buf+0x25a/0x300
> >> [   23.750751]  __spi_pump_messages+0x2a4/0x680
> >> [   23.750762]  __spi_sync+0x1dd/0x1f0
> >> [   23.750773]  spi_sync+0x26/0x40
> >> [   23.750790]  mipi_dbi_typec3_command_read+0x14d/0x240 [mipi_dbi]
> >> [   23.750802]  ? spi_finalize_current_transfer+0x10/0x10
> >> [   23.750821]  mipi_dbi_typec3_command+0x1bc/0x1d0 [mipi_dbi]
> >>
> > 
> > After few runs I don't see the warning anymore. So,
> > Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> 
> Can I have an ack as well if you're satisfied with it?

Sure.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I hope to see this in v5.1-rc1.
> 
> Noralf.
> 
> > Though I would like to give few more days to monitor the state.
> > (I believe it's fixed)
> > 
> >> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> ---
> >>  drivers/gpu/drm/tinydrm/ili9225.c  |  6 ++--
> >>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 58 +++++++++++++++++++++---------
> >>  include/drm/tinydrm/mipi-dbi.h     |  5 +--
> >>  3 files changed, 48 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
> >> index 3f59cfbd31ba..a87078667e04 100644
> >> --- a/drivers/gpu/drm/tinydrm/ili9225.c
> >> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> >> @@ -299,7 +299,7 @@ static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
> >>  	mipi->enabled = false;
> >>  }
> >>  
> >> -static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
> >> +static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
> >>  			       size_t num)
> >>  {
> >>  	struct spi_device *spi = mipi->spi;
> >> @@ -309,11 +309,11 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
> >>  
> >>  	gpiod_set_value_cansleep(mipi->dc, 0);
> >>  	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
> >> -	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
> >> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
> >>  	if (ret || !num)
> >>  		return ret;
> >>  
> >> -	if (cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
> >> +	if (*cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
> >>  		bpw = 16;
> >>  
> >>  	gpiod_set_value_cansleep(mipi->dc, 1);
> >> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> >> index 246e708a9ff7..4a4cd7e72938 100644
> >> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> >> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> >> @@ -153,16 +153,42 @@ EXPORT_SYMBOL(mipi_dbi_command_read);
> >>   */
> >>  int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
> >>  {
> >> +	u8 *cmdbuf;
> >>  	int ret;
> >>  
> >> +	/* SPI requires dma-safe buffers */
> >> +	cmdbuf = kmemdup(&cmd, 1, GFP_KERNEL);
> >> +	if (!cmdbuf)
> >> +		return -ENOMEM;
> >> +
> >>  	mutex_lock(&mipi->cmdlock);
> >> -	ret = mipi->command(mipi, cmd, data, len);
> >> +	ret = mipi->command(mipi, cmdbuf, data, len);
> >>  	mutex_unlock(&mipi->cmdlock);
> >>  
> >> +	kfree(cmdbuf);
> >> +
> >>  	return ret;
> >>  }
> >>  EXPORT_SYMBOL(mipi_dbi_command_buf);
> >>  
> >> +/* This should only be used by mipi_dbi_command() */
> >> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
> >> +{
> >> +	u8 *buf;
> >> +	int ret;
> >> +
> >> +	buf = kmemdup(data, len, GFP_KERNEL);
> >> +	if (!buf)
> >> +		return -ENOMEM;
> >> +
> >> +	ret = mipi_dbi_command_buf(mipi, cmd, buf, len);
> >> +
> >> +	kfree(buf);
> >> +
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
> >> +
> >>  /**
> >>   * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
> >>   * @dst: The destination buffer
> >> @@ -772,18 +798,18 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *mipi, int dc,
> >>  	return 0;
> >>  }
> >>  
> >> -static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
> >> +static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 *cmd,
> >>  				   u8 *parameters, size_t num)
> >>  {
> >> -	unsigned int bpw = (cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
> >> +	unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
> >>  	int ret;
> >>  
> >> -	if (mipi_dbi_command_is_read(mipi, cmd))
> >> +	if (mipi_dbi_command_is_read(mipi, *cmd))
> >>  		return -ENOTSUPP;
> >>  
> >> -	MIPI_DBI_DEBUG_COMMAND(cmd, parameters, num);
> >> +	MIPI_DBI_DEBUG_COMMAND(*cmd, parameters, num);
> >>  
> >> -	ret = mipi_dbi_spi1_transfer(mipi, 0, &cmd, 1, 8);
> >> +	ret = mipi_dbi_spi1_transfer(mipi, 0, cmd, 1, 8);
> >>  	if (ret || !num)
> >>  		return ret;
> >>  
> >> @@ -792,7 +818,7 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
> >>  
> >>  /* MIPI DBI Type C Option 3 */
> >>  
> >> -static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
> >> +static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 *cmd,
> >>  					u8 *data, size_t len)
> >>  {
> >>  	struct spi_device *spi = mipi->spi;
> >> @@ -801,7 +827,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
> >>  	struct spi_transfer tr[2] = {
> >>  		{
> >>  			.speed_hz = speed_hz,
> >> -			.tx_buf = &cmd,
> >> +			.tx_buf = cmd,
> >>  			.len = 1,
> >>  		}, {
> >>  			.speed_hz = speed_hz,
> >> @@ -819,8 +845,8 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
> >>  	 * Support non-standard 24-bit and 32-bit Nokia read commands which
> >>  	 * start with a dummy clock, so we need to read an extra byte.
> >>  	 */
> >> -	if (cmd == MIPI_DCS_GET_DISPLAY_ID ||
> >> -	    cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
> >> +	if (*cmd == MIPI_DCS_GET_DISPLAY_ID ||
> >> +	    *cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
> >>  		if (!(len == 3 || len == 4))
> >>  			return -EINVAL;
> >>  
> >> @@ -850,7 +876,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
> >>  			data[i] = (buf[i] << 1) | !!(buf[i + 1] & BIT(7));
> >>  	}
> >>  
> >> -	MIPI_DBI_DEBUG_COMMAND(cmd, data, len);
> >> +	MIPI_DBI_DEBUG_COMMAND(*cmd, data, len);
> >>  
> >>  err_free:
> >>  	kfree(buf);
> >> @@ -858,7 +884,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
> >>  	return ret;
> >>  }
> >>  
> >> -static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
> >> +static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd,
> >>  				   u8 *par, size_t num)
> >>  {
> >>  	struct spi_device *spi = mipi->spi;
> >> @@ -866,18 +892,18 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
> >>  	u32 speed_hz;
> >>  	int ret;
> >>  
> >> -	if (mipi_dbi_command_is_read(mipi, cmd))
> >> +	if (mipi_dbi_command_is_read(mipi, *cmd))
> >>  		return mipi_dbi_typec3_command_read(mipi, cmd, par, num);
> >>  
> >> -	MIPI_DBI_DEBUG_COMMAND(cmd, par, num);
> >> +	MIPI_DBI_DEBUG_COMMAND(*cmd, par, num);
> >>  
> >>  	gpiod_set_value_cansleep(mipi->dc, 0);
> >>  	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
> >> -	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
> >> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
> >>  	if (ret || !num)
> >>  		return ret;
> >>  
> >> -	if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> >> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> >>  		bpw = 16;
> >>  
> >>  	gpiod_set_value_cansleep(mipi->dc, 1);
> >> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
> >> index ad7e6bedab5f..464a47d1013d 100644
> >> --- a/include/drm/tinydrm/mipi-dbi.h
> >> +++ b/include/drm/tinydrm/mipi-dbi.h
> >> @@ -43,7 +43,7 @@ struct mipi_dbi {
> >>  	struct spi_device *spi;
> >>  	bool enabled;
> >>  	struct mutex cmdlock;
> >> -	int (*command)(struct mipi_dbi *mipi, u8 cmd, u8 *param, size_t num);
> >> +	int (*command)(struct mipi_dbi *mipi, u8 *cmd, u8 *param, size_t num);
> >>  	const u8 *read_commands;
> >>  	struct gpio_desc *dc;
> >>  	u16 *tx_buf;
> >> @@ -83,6 +83,7 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
> >>  
> >>  int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
> >>  int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
> >> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
> >>  int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
> >>  		      struct drm_rect *clip, bool swap);
> >>  /**
> >> @@ -100,7 +101,7 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
> >>  #define mipi_dbi_command(mipi, cmd, seq...) \
> >>  ({ \
> >>  	u8 d[] = { seq }; \
> >> -	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
> >> +	mipi_dbi_command_stackbuf(mipi, cmd, d, ARRAY_SIZE(d)); \
> >>  })
> >>  
> >>  #ifdef CONFIG_DEBUG_FS
> >> -- 
> >> 2.20.1
> >>
> >
Noralf Trønnes March 4, 2019, 5:51 p.m. UTC | #4
Den 04.03.2019 16.10, skrev Andy Shevchenko:
> On Mon, Mar 04, 2019 at 03:45:56PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 22.02.2019 16.58, skrev Andy Shevchenko:
>>> On Fri, Feb 22, 2019 at 01:43:29PM +0100, Noralf Trønnes wrote:
>>>> Buffers passed to spi_sync() must be dma-safe even for tiny buffers since
>>>> some SPI controllers use DMA for all transfers.
>>>>
>>>> Example splat with CONFIG_DMA_API_DEBUG enabled:
>>>>
>>>> [   23.750467] DMA-API: dw_dmac_pci 0000:00:15.0: device driver maps memory from stack [probable addr=000000001e49185d]
>>>> [   23.750529] WARNING: CPU: 1 PID: 1296 at kernel/dma/debug.c:1161 check_for_stack+0xb7/0x190
>>>> [   23.750533] Modules linked in: mmc_block(+) spi_pxa2xx_platform(+) pwm_lpss_pci pwm_lpss spi_pxa2xx_pci sdhci_pci cqhci intel_mrfld_pwrbtn extcon_intel_mrfld sdhci intel_mrfld_adc led_class mmc_core ili9341 mipi_dbi tinydrm backlight ti_ads7950 industrialio_triggered_buffer kfifo_buf intel_soc_pmic_mrfld hci_uart btbcm
>>>> [   23.750599] CPU: 1 PID: 1296 Comm: modprobe Not tainted 5.0.0-rc7+ #236
>>>> [   23.750605] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
>>>> [   23.750620] RIP: 0010:check_for_stack+0xb7/0x190
>>>> [   23.750630] Code: 8b 6d 50 4d 85 ed 75 04 4c 8b 6d 10 48 89 ef e8 2f 8b 44 00 48 89 c6 4a 8d 0c 23 4c 89 ea 48 c7 c7 88 d0 82 b4 e8 40 7c f9 ff <0f> 0b 8b 05 79 00 4b 01 85 c0 74 07 5b 5d 41 5c 41 5d c3 8b 05 54
>>>> [   23.750637] RSP: 0000:ffff97bbc0292fa0 EFLAGS: 00010286
>>>> [   23.750646] RAX: 0000000000000000 RBX: ffff97bbc0290000 RCX: 0000000000000006
>>>> [   23.750652] RDX: 0000000000000007 RSI: 0000000000000002 RDI: ffff94b33e115450
>>>> [   23.750658] RBP: ffff94b33c8578b0 R08: 0000000000000002 R09: 00000000000201c0
>>>> [   23.750664] R10: 00000006ecb0ccc6 R11: 0000000000034f38 R12: 000000000000316c
>>>> [   23.750670] R13: ffff94b33c84b250 R14: ffff94b33dedd5a0 R15: 0000000000000001
>>>> [   23.750679] FS:  0000000000000000(0000) GS:ffff94b33e100000(0063) knlGS:00000000f7faf690
>>>> [   23.750686] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
>>>> [   23.750691] CR2: 00000000f7f54faf CR3: 000000000722c000 CR4: 00000000001006e0
>>>> [   23.750696] Call Trace:
>>>> [   23.750713]  debug_dma_map_sg+0x100/0x340
>>>> [   23.750727]  ? dma_direct_map_sg+0x3b/0xb0
>>>> [   23.750739]  spi_map_buf+0x25a/0x300
>>>> [   23.750751]  __spi_pump_messages+0x2a4/0x680
>>>> [   23.750762]  __spi_sync+0x1dd/0x1f0
>>>> [   23.750773]  spi_sync+0x26/0x40
>>>> [   23.750790]  mipi_dbi_typec3_command_read+0x14d/0x240 [mipi_dbi]
>>>> [   23.750802]  ? spi_finalize_current_transfer+0x10/0x10
>>>> [   23.750821]  mipi_dbi_typec3_command+0x1bc/0x1d0 [mipi_dbi]
>>>>
>>>
>>> After few runs I don't see the warning anymore. So,
>>> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>
>>
>> Can I have an ack as well if you're satisfied with it?
> 
> Sure.
> 
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I hope to see this in v5.1-rc1.

The patch doesn't apply cleanly on (to be) 5.1 due to this one:

drm/tinydrm: Use struct drm_rect
https://cgit.freedesktop.org/drm/drm/commit/?id=b051b3459bbae907ef068bcd8b62f73f09ea5016

If it's just a debug warning but no ill effects, it'll show up in 5.2.
Otherwise I will have to backport it.

Noralf.

>>
>> Noralf.
>>
>>> Though I would like to give few more days to monitor the state.
>>> (I believe it's fixed)
>>>
>>>> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>  drivers/gpu/drm/tinydrm/ili9225.c  |  6 ++--
>>>>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 58 +++++++++++++++++++++---------
>>>>  include/drm/tinydrm/mipi-dbi.h     |  5 +--
>>>>  3 files changed, 48 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
>>>> index 3f59cfbd31ba..a87078667e04 100644
>>>> --- a/drivers/gpu/drm/tinydrm/ili9225.c
>>>> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
>>>> @@ -299,7 +299,7 @@ static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
>>>>  	mipi->enabled = false;
>>>>  }
>>>>  
>>>> -static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
>>>> +static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
>>>>  			       size_t num)
>>>>  {
>>>>  	struct spi_device *spi = mipi->spi;
>>>> @@ -309,11 +309,11 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
>>>>  
>>>>  	gpiod_set_value_cansleep(mipi->dc, 0);
>>>>  	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
>>>> -	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
>>>> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
>>>>  	if (ret || !num)
>>>>  		return ret;
>>>>  
>>>> -	if (cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
>>>> +	if (*cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
>>>>  		bpw = 16;
>>>>  
>>>>  	gpiod_set_value_cansleep(mipi->dc, 1);
>>>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>> index 246e708a9ff7..4a4cd7e72938 100644
>>>> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>> @@ -153,16 +153,42 @@ EXPORT_SYMBOL(mipi_dbi_command_read);
>>>>   */
>>>>  int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
>>>>  {
>>>> +	u8 *cmdbuf;
>>>>  	int ret;
>>>>  
>>>> +	/* SPI requires dma-safe buffers */
>>>> +	cmdbuf = kmemdup(&cmd, 1, GFP_KERNEL);
>>>> +	if (!cmdbuf)
>>>> +		return -ENOMEM;
>>>> +
>>>>  	mutex_lock(&mipi->cmdlock);
>>>> -	ret = mipi->command(mipi, cmd, data, len);
>>>> +	ret = mipi->command(mipi, cmdbuf, data, len);
>>>>  	mutex_unlock(&mipi->cmdlock);
>>>>  
>>>> +	kfree(cmdbuf);
>>>> +
>>>>  	return ret;
>>>>  }
>>>>  EXPORT_SYMBOL(mipi_dbi_command_buf);
>>>>  
>>>> +/* This should only be used by mipi_dbi_command() */
>>>> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
>>>> +{
>>>> +	u8 *buf;
>>>> +	int ret;
>>>> +
>>>> +	buf = kmemdup(data, len, GFP_KERNEL);
>>>> +	if (!buf)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	ret = mipi_dbi_command_buf(mipi, cmd, buf, len);
>>>> +
>>>> +	kfree(buf);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>>>> +
>>>>  /**
>>>>   * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
>>>>   * @dst: The destination buffer
>>>> @@ -772,18 +798,18 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *mipi, int dc,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
>>>> +static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 *cmd,
>>>>  				   u8 *parameters, size_t num)
>>>>  {
>>>> -	unsigned int bpw = (cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
>>>> +	unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
>>>>  	int ret;
>>>>  
>>>> -	if (mipi_dbi_command_is_read(mipi, cmd))
>>>> +	if (mipi_dbi_command_is_read(mipi, *cmd))
>>>>  		return -ENOTSUPP;
>>>>  
>>>> -	MIPI_DBI_DEBUG_COMMAND(cmd, parameters, num);
>>>> +	MIPI_DBI_DEBUG_COMMAND(*cmd, parameters, num);
>>>>  
>>>> -	ret = mipi_dbi_spi1_transfer(mipi, 0, &cmd, 1, 8);
>>>> +	ret = mipi_dbi_spi1_transfer(mipi, 0, cmd, 1, 8);
>>>>  	if (ret || !num)
>>>>  		return ret;
>>>>  
>>>> @@ -792,7 +818,7 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
>>>>  
>>>>  /* MIPI DBI Type C Option 3 */
>>>>  
>>>> -static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>>>> +static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 *cmd,
>>>>  					u8 *data, size_t len)
>>>>  {
>>>>  	struct spi_device *spi = mipi->spi;
>>>> @@ -801,7 +827,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>>>>  	struct spi_transfer tr[2] = {
>>>>  		{
>>>>  			.speed_hz = speed_hz,
>>>> -			.tx_buf = &cmd,
>>>> +			.tx_buf = cmd,
>>>>  			.len = 1,
>>>>  		}, {
>>>>  			.speed_hz = speed_hz,
>>>> @@ -819,8 +845,8 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>>>>  	 * Support non-standard 24-bit and 32-bit Nokia read commands which
>>>>  	 * start with a dummy clock, so we need to read an extra byte.
>>>>  	 */
>>>> -	if (cmd == MIPI_DCS_GET_DISPLAY_ID ||
>>>> -	    cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
>>>> +	if (*cmd == MIPI_DCS_GET_DISPLAY_ID ||
>>>> +	    *cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
>>>>  		if (!(len == 3 || len == 4))
>>>>  			return -EINVAL;
>>>>  
>>>> @@ -850,7 +876,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>>>>  			data[i] = (buf[i] << 1) | !!(buf[i + 1] & BIT(7));
>>>>  	}
>>>>  
>>>> -	MIPI_DBI_DEBUG_COMMAND(cmd, data, len);
>>>> +	MIPI_DBI_DEBUG_COMMAND(*cmd, data, len);
>>>>  
>>>>  err_free:
>>>>  	kfree(buf);
>>>> @@ -858,7 +884,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> -static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>>>> +static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd,
>>>>  				   u8 *par, size_t num)
>>>>  {
>>>>  	struct spi_device *spi = mipi->spi;
>>>> @@ -866,18 +892,18 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>>>>  	u32 speed_hz;
>>>>  	int ret;
>>>>  
>>>> -	if (mipi_dbi_command_is_read(mipi, cmd))
>>>> +	if (mipi_dbi_command_is_read(mipi, *cmd))
>>>>  		return mipi_dbi_typec3_command_read(mipi, cmd, par, num);
>>>>  
>>>> -	MIPI_DBI_DEBUG_COMMAND(cmd, par, num);
>>>> +	MIPI_DBI_DEBUG_COMMAND(*cmd, par, num);
>>>>  
>>>>  	gpiod_set_value_cansleep(mipi->dc, 0);
>>>>  	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
>>>> -	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
>>>> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
>>>>  	if (ret || !num)
>>>>  		return ret;
>>>>  
>>>> -	if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
>>>> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
>>>>  		bpw = 16;
>>>>  
>>>>  	gpiod_set_value_cansleep(mipi->dc, 1);
>>>> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
>>>> index ad7e6bedab5f..464a47d1013d 100644
>>>> --- a/include/drm/tinydrm/mipi-dbi.h
>>>> +++ b/include/drm/tinydrm/mipi-dbi.h
>>>> @@ -43,7 +43,7 @@ struct mipi_dbi {
>>>>  	struct spi_device *spi;
>>>>  	bool enabled;
>>>>  	struct mutex cmdlock;
>>>> -	int (*command)(struct mipi_dbi *mipi, u8 cmd, u8 *param, size_t num);
>>>> +	int (*command)(struct mipi_dbi *mipi, u8 *cmd, u8 *param, size_t num);
>>>>  	const u8 *read_commands;
>>>>  	struct gpio_desc *dc;
>>>>  	u16 *tx_buf;
>>>> @@ -83,6 +83,7 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
>>>>  
>>>>  int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
>>>>  int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
>>>> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
>>>>  int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>>>>  		      struct drm_rect *clip, bool swap);
>>>>  /**
>>>> @@ -100,7 +101,7 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>>>>  #define mipi_dbi_command(mipi, cmd, seq...) \
>>>>  ({ \
>>>>  	u8 d[] = { seq }; \
>>>> -	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
>>>> +	mipi_dbi_command_stackbuf(mipi, cmd, d, ARRAY_SIZE(d)); \
>>>>  })
>>>>  
>>>>  #ifdef CONFIG_DEBUG_FS
>>>> -- 
>>>> 2.20.1
>>>>
>>>
>
Andy Shevchenko March 5, 2019, 7:32 a.m. UTC | #5
On Mon, Mar 04, 2019 at 06:51:33PM +0100, Noralf Trønnes wrote:
> Den 04.03.2019 16.10, skrev Andy Shevchenko:
> > On Mon, Mar 04, 2019 at 03:45:56PM +0100, Noralf Trønnes wrote:
> >> Den 22.02.2019 16.58, skrev Andy Shevchenko:
> >>> On Fri, Feb 22, 2019 at 01:43:29PM +0100, Noralf Trønnes wrote:

> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > I hope to see this in v5.1-rc1.
> 
> The patch doesn't apply cleanly on (to be) 5.1 due to this one:
> 
> drm/tinydrm: Use struct drm_rect
> https://cgit.freedesktop.org/drm/drm/commit/?id=b051b3459bbae907ef068bcd8b62f73f09ea5016
> 
> If it's just a debug warning but no ill effects, it'll show up in 5.2.
> Otherwise I will have to backport it.

For x86 it's just a debug message, since on x86 stack memory is DMA capable.
But in general this should be fixed for any possible scenarios.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
index 3f59cfbd31ba..a87078667e04 100644
--- a/drivers/gpu/drm/tinydrm/ili9225.c
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -299,7 +299,7 @@  static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
 	mipi->enabled = false;
 }
 
-static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
+static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
 			       size_t num)
 {
 	struct spi_device *spi = mipi->spi;
@@ -309,11 +309,11 @@  static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
 
 	gpiod_set_value_cansleep(mipi->dc, 0);
 	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
-	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
+	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
 	if (ret || !num)
 		return ret;
 
-	if (cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
+	if (*cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
 		bpw = 16;
 
 	gpiod_set_value_cansleep(mipi->dc, 1);
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index 246e708a9ff7..4a4cd7e72938 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -153,16 +153,42 @@  EXPORT_SYMBOL(mipi_dbi_command_read);
  */
 int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
 {
+	u8 *cmdbuf;
 	int ret;
 
+	/* SPI requires dma-safe buffers */
+	cmdbuf = kmemdup(&cmd, 1, GFP_KERNEL);
+	if (!cmdbuf)
+		return -ENOMEM;
+
 	mutex_lock(&mipi->cmdlock);
-	ret = mipi->command(mipi, cmd, data, len);
+	ret = mipi->command(mipi, cmdbuf, data, len);
 	mutex_unlock(&mipi->cmdlock);
 
+	kfree(cmdbuf);
+
 	return ret;
 }
 EXPORT_SYMBOL(mipi_dbi_command_buf);
 
+/* This should only be used by mipi_dbi_command() */
+int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
+{
+	u8 *buf;
+	int ret;
+
+	buf = kmemdup(data, len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = mipi_dbi_command_buf(mipi, cmd, buf, len);
+
+	kfree(buf);
+
+	return ret;
+}
+EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
+
 /**
  * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
  * @dst: The destination buffer
@@ -772,18 +798,18 @@  static int mipi_dbi_spi1_transfer(struct mipi_dbi *mipi, int dc,
 	return 0;
 }
 
-static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
+static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 *cmd,
 				   u8 *parameters, size_t num)
 {
-	unsigned int bpw = (cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
+	unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
 	int ret;
 
-	if (mipi_dbi_command_is_read(mipi, cmd))
+	if (mipi_dbi_command_is_read(mipi, *cmd))
 		return -ENOTSUPP;
 
-	MIPI_DBI_DEBUG_COMMAND(cmd, parameters, num);
+	MIPI_DBI_DEBUG_COMMAND(*cmd, parameters, num);
 
-	ret = mipi_dbi_spi1_transfer(mipi, 0, &cmd, 1, 8);
+	ret = mipi_dbi_spi1_transfer(mipi, 0, cmd, 1, 8);
 	if (ret || !num)
 		return ret;
 
@@ -792,7 +818,7 @@  static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
 
 /* MIPI DBI Type C Option 3 */
 
-static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
+static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 *cmd,
 					u8 *data, size_t len)
 {
 	struct spi_device *spi = mipi->spi;
@@ -801,7 +827,7 @@  static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
 	struct spi_transfer tr[2] = {
 		{
 			.speed_hz = speed_hz,
-			.tx_buf = &cmd,
+			.tx_buf = cmd,
 			.len = 1,
 		}, {
 			.speed_hz = speed_hz,
@@ -819,8 +845,8 @@  static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
 	 * Support non-standard 24-bit and 32-bit Nokia read commands which
 	 * start with a dummy clock, so we need to read an extra byte.
 	 */
-	if (cmd == MIPI_DCS_GET_DISPLAY_ID ||
-	    cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
+	if (*cmd == MIPI_DCS_GET_DISPLAY_ID ||
+	    *cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
 		if (!(len == 3 || len == 4))
 			return -EINVAL;
 
@@ -850,7 +876,7 @@  static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
 			data[i] = (buf[i] << 1) | !!(buf[i + 1] & BIT(7));
 	}
 
-	MIPI_DBI_DEBUG_COMMAND(cmd, data, len);
+	MIPI_DBI_DEBUG_COMMAND(*cmd, data, len);
 
 err_free:
 	kfree(buf);
@@ -858,7 +884,7 @@  static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
 	return ret;
 }
 
-static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
+static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd,
 				   u8 *par, size_t num)
 {
 	struct spi_device *spi = mipi->spi;
@@ -866,18 +892,18 @@  static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
 	u32 speed_hz;
 	int ret;
 
-	if (mipi_dbi_command_is_read(mipi, cmd))
+	if (mipi_dbi_command_is_read(mipi, *cmd))
 		return mipi_dbi_typec3_command_read(mipi, cmd, par, num);
 
-	MIPI_DBI_DEBUG_COMMAND(cmd, par, num);
+	MIPI_DBI_DEBUG_COMMAND(*cmd, par, num);
 
 	gpiod_set_value_cansleep(mipi->dc, 0);
 	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
-	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
+	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
 	if (ret || !num)
 		return ret;
 
-	if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
+	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
 		bpw = 16;
 
 	gpiod_set_value_cansleep(mipi->dc, 1);
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index ad7e6bedab5f..464a47d1013d 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -43,7 +43,7 @@  struct mipi_dbi {
 	struct spi_device *spi;
 	bool enabled;
 	struct mutex cmdlock;
-	int (*command)(struct mipi_dbi *mipi, u8 cmd, u8 *param, size_t num);
+	int (*command)(struct mipi_dbi *mipi, u8 *cmd, u8 *param, size_t num);
 	const u8 *read_commands;
 	struct gpio_desc *dc;
 	u16 *tx_buf;
@@ -83,6 +83,7 @@  u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
 
 int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
 int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
+int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
 int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 		      struct drm_rect *clip, bool swap);
 /**
@@ -100,7 +101,7 @@  int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 #define mipi_dbi_command(mipi, cmd, seq...) \
 ({ \
 	u8 d[] = { seq }; \
-	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
+	mipi_dbi_command_stackbuf(mipi, cmd, d, ARRAY_SIZE(d)); \
 })
 
 #ifdef CONFIG_DEBUG_FS