* [PATCH 0/3] Some fixes for slave-dma stuff for spi-rockchip @ 2016-03-09 8:10 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-09 8:10 UTC (permalink / raw) To: Mark Brown; +Cc: linux-spi, linux-kernel, Shawn Lin This patchset mainly do the following three things: (1) Defer probing driver when geting EPROBE_DEFER (2) Check the return value of dmaengine_prep_slave_sg (3) Migrate dmaengine_terminate_all to dmaengine_terminate_async Shawn Lin (3): spi: rockchip: check return value of dmaengine_prep_slave_sg spi: rockchip: migrate to dmaengine_terminate_async spi: rockchip: check requesting dma channel with EPROBE_DEFER drivers/spi/spi-rockchip.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) -- 2.3.7 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 0/3] Some fixes for slave-dma stuff for spi-rockchip @ 2016-03-09 8:10 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-09 8:10 UTC (permalink / raw) To: Mark Brown Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Shawn Lin This patchset mainly do the following three things: (1) Defer probing driver when geting EPROBE_DEFER (2) Check the return value of dmaengine_prep_slave_sg (3) Migrate dmaengine_terminate_all to dmaengine_terminate_async Shawn Lin (3): spi: rockchip: check return value of dmaengine_prep_slave_sg spi: rockchip: migrate to dmaengine_terminate_async spi: rockchip: check requesting dma channel with EPROBE_DEFER drivers/spi/spi-rockchip.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) -- 2.3.7 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/3] spi: rockchip: check return value of dmaengine_prep_slave_sg @ 2016-03-09 8:11 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-09 8:11 UTC (permalink / raw) To: Mark Brown; +Cc: linux-spi, linux-kernel, Shawn Lin We should check return value of dmaengine_prep_slave_sg, otherwise we take risk of null pointer. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/spi/spi-rockchip.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index eb1bd45..91f44a3 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -437,7 +437,7 @@ static void rockchip_spi_dma_txcb(void *data) spin_unlock_irqrestore(&rs->lock, flags); } -static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) +static int rockchip_spi_prepare_dma(struct rockchip_spi *rs) { unsigned long flags; struct dma_slave_config rxconf, txconf; @@ -463,6 +463,8 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) rs->dma_rx.ch, rs->rx_sg.sgl, rs->rx_sg.nents, rs->dma_rx.direction, DMA_PREP_INTERRUPT); + if (!rxdesc) + return -EINVAL; rxdesc->callback = rockchip_spi_dma_rxcb; rxdesc->callback_param = rs; @@ -483,6 +485,11 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) rs->dma_tx.ch, rs->tx_sg.sgl, rs->tx_sg.nents, rs->dma_tx.direction, DMA_PREP_INTERRUPT); + if (!txdesc) { + if (rxdesc) + dmaengine_terminate_sync(rs->dma_rx.ch); + return -EINVAL; + } txdesc->callback = rockchip_spi_dma_txcb; txdesc->callback_param = rs; @@ -504,6 +511,8 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) dmaengine_submit(txdesc); dma_async_issue_pending(rs->dma_tx.ch); } + + return 0; } static void rockchip_spi_config(struct rockchip_spi *rs) @@ -617,12 +626,12 @@ static int rockchip_spi_transfer_one( if (rs->use_dma) { if (rs->tmode == CR0_XFM_RO) { /* rx: dma must be prepared first */ - rockchip_spi_prepare_dma(rs); + ret = rockchip_spi_prepare_dma(rs); spi_enable_chip(rs, 1); } else { /* tx or tr: spi must be enabled first */ spi_enable_chip(rs, 1); - rockchip_spi_prepare_dma(rs); + ret = rockchip_spi_prepare_dma(rs); } } else { spi_enable_chip(rs, 1); -- 2.3.7 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 1/3] spi: rockchip: check return value of dmaengine_prep_slave_sg @ 2016-03-09 8:11 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-09 8:11 UTC (permalink / raw) To: Mark Brown Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Shawn Lin We should check return value of dmaengine_prep_slave_sg, otherwise we take risk of null pointer. Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> --- drivers/spi/spi-rockchip.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index eb1bd45..91f44a3 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -437,7 +437,7 @@ static void rockchip_spi_dma_txcb(void *data) spin_unlock_irqrestore(&rs->lock, flags); } -static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) +static int rockchip_spi_prepare_dma(struct rockchip_spi *rs) { unsigned long flags; struct dma_slave_config rxconf, txconf; @@ -463,6 +463,8 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) rs->dma_rx.ch, rs->rx_sg.sgl, rs->rx_sg.nents, rs->dma_rx.direction, DMA_PREP_INTERRUPT); + if (!rxdesc) + return -EINVAL; rxdesc->callback = rockchip_spi_dma_rxcb; rxdesc->callback_param = rs; @@ -483,6 +485,11 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) rs->dma_tx.ch, rs->tx_sg.sgl, rs->tx_sg.nents, rs->dma_tx.direction, DMA_PREP_INTERRUPT); + if (!txdesc) { + if (rxdesc) + dmaengine_terminate_sync(rs->dma_rx.ch); + return -EINVAL; + } txdesc->callback = rockchip_spi_dma_txcb; txdesc->callback_param = rs; @@ -504,6 +511,8 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) dmaengine_submit(txdesc); dma_async_issue_pending(rs->dma_tx.ch); } + + return 0; } static void rockchip_spi_config(struct rockchip_spi *rs) @@ -617,12 +626,12 @@ static int rockchip_spi_transfer_one( if (rs->use_dma) { if (rs->tmode == CR0_XFM_RO) { /* rx: dma must be prepared first */ - rockchip_spi_prepare_dma(rs); + ret = rockchip_spi_prepare_dma(rs); spi_enable_chip(rs, 1); } else { /* tx or tr: spi must be enabled first */ spi_enable_chip(rs, 1); - rockchip_spi_prepare_dma(rs); + ret = rockchip_spi_prepare_dma(rs); } } else { spi_enable_chip(rs, 1); -- 2.3.7 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <1457511075-2134-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>]
* Applied "spi: rockchip: check return value of dmaengine_prep_slave_sg" to the spi tree [not found] ` <1457511075-2134-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> @ 2016-03-10 3:39 ` Mark Brown 0 siblings, 0 replies; 28+ messages in thread From: Mark Brown @ 2016-03-10 3:39 UTC (permalink / raw) To: Shawn Lin, Mark Brown; +Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA The patch spi: rockchip: check return value of dmaengine_prep_slave_sg has been applied to the spi tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From ea98491133439773b69345eb9a314fc5f15e07a4 Mon Sep 17 00:00:00 2001 From: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> Date: Wed, 9 Mar 2016 16:11:15 +0800 Subject: [PATCH] spi: rockchip: check return value of dmaengine_prep_slave_sg We should check return value of dmaengine_prep_slave_sg, otherwise we take risk of null pointer. Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> --- drivers/spi/spi-rockchip.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index 9a5c51764833..aa8528e9840c 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -436,7 +436,7 @@ static void rockchip_spi_dma_txcb(void *data) spin_unlock_irqrestore(&rs->lock, flags); } -static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) +static int rockchip_spi_prepare_dma(struct rockchip_spi *rs) { unsigned long flags; struct dma_slave_config rxconf, txconf; @@ -459,6 +459,8 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) rs->dma_rx.ch, rs->rx_sg.sgl, rs->rx_sg.nents, rs->dma_rx.direction, DMA_PREP_INTERRUPT); + if (!rxdesc) + return -EINVAL; rxdesc->callback = rockchip_spi_dma_rxcb; rxdesc->callback_param = rs; @@ -476,6 +478,11 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) rs->dma_tx.ch, rs->tx_sg.sgl, rs->tx_sg.nents, rs->dma_tx.direction, DMA_PREP_INTERRUPT); + if (!txdesc) { + if (rxdesc) + dmaengine_terminate_sync(rs->dma_rx.ch); + return -EINVAL; + } txdesc->callback = rockchip_spi_dma_txcb; txdesc->callback_param = rs; @@ -497,6 +504,8 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs) dmaengine_submit(txdesc); dma_async_issue_pending(rs->dma_tx.ch); } + + return 0; } static void rockchip_spi_config(struct rockchip_spi *rs) @@ -610,12 +619,12 @@ static int rockchip_spi_transfer_one( if (rs->use_dma) { if (rs->tmode == CR0_XFM_RO) { /* rx: dma must be prepared first */ - rockchip_spi_prepare_dma(rs); + ret = rockchip_spi_prepare_dma(rs); spi_enable_chip(rs, 1); } else { /* tx or tr: spi must be enabled first */ spi_enable_chip(rs, 1); - rockchip_spi_prepare_dma(rs); + ret = rockchip_spi_prepare_dma(rs); } } else { spi_enable_chip(rs, 1); -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/3] spi: rockchip: migrate to dmaengine_terminate_async @ 2016-03-09 8:11 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-09 8:11 UTC (permalink / raw) To: Mark Brown; +Cc: linux-spi, linux-kernel, Shawn Lin dmaengine_terminate_all is deprecated, let's use dmaengine_terminate_async for interrupt handling. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/spi/spi-rockchip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index 91f44a3..ca4f4e0 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -323,12 +323,12 @@ static void rockchip_spi_handle_err(struct spi_master *master, */ if (rs->use_dma) { if (rs->state & RXBUSY) { - dmaengine_terminate_all(rs->dma_rx.ch); + dmaengine_terminate_async(rs->dma_rx.ch); flush_fifo(rs); } if (rs->state & TXBUSY) - dmaengine_terminate_all(rs->dma_tx.ch); + dmaengine_terminate_async(rs->dma_tx.ch); } spin_unlock_irqrestore(&rs->lock, flags); -- 2.3.7 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/3] spi: rockchip: migrate to dmaengine_terminate_async @ 2016-03-09 8:11 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-09 8:11 UTC (permalink / raw) To: Mark Brown Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Shawn Lin dmaengine_terminate_all is deprecated, let's use dmaengine_terminate_async for interrupt handling. Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> --- drivers/spi/spi-rockchip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index 91f44a3..ca4f4e0 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -323,12 +323,12 @@ static void rockchip_spi_handle_err(struct spi_master *master, */ if (rs->use_dma) { if (rs->state & RXBUSY) { - dmaengine_terminate_all(rs->dma_rx.ch); + dmaengine_terminate_async(rs->dma_rx.ch); flush_fifo(rs); } if (rs->state & TXBUSY) - dmaengine_terminate_all(rs->dma_tx.ch); + dmaengine_terminate_async(rs->dma_tx.ch); } spin_unlock_irqrestore(&rs->lock, flags); -- 2.3.7 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <1457511083-2175-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>]
* Applied "spi: rockchip: migrate to dmaengine_terminate_async" to the spi tree [not found] ` <1457511083-2175-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> @ 2016-03-10 3:39 ` Mark Brown 0 siblings, 0 replies; 28+ messages in thread From: Mark Brown @ 2016-03-10 3:39 UTC (permalink / raw) To: Shawn Lin, Mark Brown; +Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA The patch spi: rockchip: migrate to dmaengine_terminate_async has been applied to the spi tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From 557b7ea34b9a10a5e13f4d7fd58ac965d801e3bd Mon Sep 17 00:00:00 2001 From: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> Date: Wed, 9 Mar 2016 16:11:23 +0800 Subject: [PATCH] spi: rockchip: migrate to dmaengine_terminate_async dmaengine_terminate_all is deprecated, let's use dmaengine_terminate_async for interrupt handling. Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> --- drivers/spi/spi-rockchip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index aa8528e9840c..b6b8a0bb5b2f 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -322,12 +322,12 @@ static void rockchip_spi_handle_err(struct spi_master *master, */ if (rs->use_dma) { if (rs->state & RXBUSY) { - dmaengine_terminate_all(rs->dma_rx.ch); + dmaengine_terminate_async(rs->dma_rx.ch); flush_fifo(rs); } if (rs->state & TXBUSY) - dmaengine_terminate_all(rs->dma_tx.ch); + dmaengine_terminate_async(rs->dma_tx.ch); } spin_unlock_irqrestore(&rs->lock, flags); -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-09 8:11 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-09 8:11 UTC (permalink / raw) To: Mark Brown; +Cc: linux-spi, linux-kernel, Shawn Lin Let's defer probing the driver if the return value of dma_request_slave_channel is ERR_PTR(-EPROBE_DEFER) instead of disabling dma capability directly. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/spi/spi-rockchip.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index ca4f4e0..75fa990 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -737,8 +737,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) master->handle_err = rockchip_spi_handle_err; rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (!rs->dma_tx.ch) + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + /* Check tx to see if we need defer probing driver */ + if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { + ret = -EPROBE_DEFER; + goto err_get_fifo_len; + } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + } rs->dma_rx.ch = dma_request_slave_channel(rs->dev, "rx"); if (!rs->dma_rx.ch) { -- 2.3.7 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-09 8:11 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-09 8:11 UTC (permalink / raw) To: Mark Brown Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Shawn Lin Let's defer probing the driver if the return value of dma_request_slave_channel is ERR_PTR(-EPROBE_DEFER) instead of disabling dma capability directly. Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> --- drivers/spi/spi-rockchip.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index ca4f4e0..75fa990 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -737,8 +737,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) master->handle_err = rockchip_spi_handle_err; rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (!rs->dma_tx.ch) + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + /* Check tx to see if we need defer probing driver */ + if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { + ret = -EPROBE_DEFER; + goto err_get_fifo_len; + } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + } rs->dma_rx.ch = dma_request_slave_channel(rs->dev, "rx"); if (!rs->dma_rx.ch) { -- 2.3.7 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <1457511092-2216-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>]
* Applied "spi: rockchip: check requesting dma channel with EPROBE_DEFER" to the spi tree [not found] ` <1457511092-2216-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> @ 2016-03-10 3:39 ` Mark Brown 0 siblings, 0 replies; 28+ messages in thread From: Mark Brown @ 2016-03-10 3:39 UTC (permalink / raw) To: Shawn Lin, Mark Brown; +Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA The patch spi: rockchip: check requesting dma channel with EPROBE_DEFER has been applied to the spi tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From 61cadcf46cfdb9f7eec70527968c2b91e9823786 Mon Sep 17 00:00:00 2001 From: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> Date: Wed, 9 Mar 2016 16:11:32 +0800 Subject: [PATCH] spi: rockchip: check requesting dma channel with EPROBE_DEFER Let's defer probing the driver if the return value of dma_request_slave_channel is ERR_PTR(-EPROBE_DEFER) instead of disabling dma capability directly. Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> --- drivers/spi/spi-rockchip.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index b6b8a0bb5b2f..795fd191faa7 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -730,8 +730,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) master->handle_err = rockchip_spi_handle_err; rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (!rs->dma_tx.ch) + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + /* Check tx to see if we need defer probing driver */ + if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { + ret = -EPROBE_DEFER; + goto err_get_fifo_len; + } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + } rs->dma_rx.ch = dma_request_slave_channel(rs->dev, "rx"); if (!rs->dma_rx.ch) { -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER [not found] ` <1457511092-2216-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> @ 2016-03-21 23:33 ` Doug Anderson 0 siblings, 0 replies; 28+ messages in thread From: Doug Anderson @ 2016-03-21 23:33 UTC (permalink / raw) To: Shawn Lin Cc: Mark Brown, linux-spi, linux-kernel, Dan Carpenter, open list:ARM/Rockchip SoC... Shawn, On Wed, Mar 9, 2016 at 12:11 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > Let's defer probing the driver if the return value of > dma_request_slave_channel is ERR_PTR(-EPROBE_DEFER) instead > of disabling dma capability directly. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/spi/spi-rockchip.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c > index ca4f4e0..75fa990 100644 > --- a/drivers/spi/spi-rockchip.c > +++ b/drivers/spi/spi-rockchip.c > @@ -737,8 +737,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) > master->handle_err = rockchip_spi_handle_err; > > rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* Check tx to see if we need defer probing driver */ > + if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { > + ret = -EPROBE_DEFER; > + goto err_get_fifo_len; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); Presumably Dan would be happy if you just add this right after the dev_warn(): rs->dma_tx.ch = NULL; Presumably from Dan's email it would also be wise to make sure you don't pass NULL to PTR_ERR, which you could probably do by just using ERR_PTR instead of PTR_ERR. I think you could structure like this: rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (!rs->dma_tx.ch) + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + /* Check tx to see if we need defer probing driver */ + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { + ret = -EPROBE_DEFER; + goto err_get_fifo_len; + } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + rs->dma_tx.ch = NULL; + } With that change your patch should be happy, I think. If some new unknown error return gets added to dma_request_slave_channel() then your code will continue to work properly. Such a change is simple and safe, so presumably you could just spin your patch with that fix. Although unlikely, it's probably good to check for IS_ERR_OR_NULL() when requesting the "rx" channel too. ...but, looking at this, presumably before landing any patch that made dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify _all_ users of dma_request_slave_channel to handle error pointers being returned. Right now dma_request_slave_channel() says it returns a pointer to a channel or NULL and the function explicitly avoids returning any errors. That might be possible, but it's a big change... -Doug ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-21 23:33 ` Doug Anderson 0 siblings, 0 replies; 28+ messages in thread From: Doug Anderson @ 2016-03-21 23:33 UTC (permalink / raw) To: Shawn Lin Cc: Mark Brown, linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dan Carpenter, open list:ARM/Rockchip SoC... Shawn, On Wed, Mar 9, 2016 at 12:11 AM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote: > Let's defer probing the driver if the return value of > dma_request_slave_channel is ERR_PTR(-EPROBE_DEFER) instead > of disabling dma capability directly. > > Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> > --- > > drivers/spi/spi-rockchip.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c > index ca4f4e0..75fa990 100644 > --- a/drivers/spi/spi-rockchip.c > +++ b/drivers/spi/spi-rockchip.c > @@ -737,8 +737,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) > master->handle_err = rockchip_spi_handle_err; > > rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* Check tx to see if we need defer probing driver */ > + if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { > + ret = -EPROBE_DEFER; > + goto err_get_fifo_len; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); Presumably Dan would be happy if you just add this right after the dev_warn(): rs->dma_tx.ch = NULL; Presumably from Dan's email it would also be wise to make sure you don't pass NULL to PTR_ERR, which you could probably do by just using ERR_PTR instead of PTR_ERR. I think you could structure like this: rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (!rs->dma_tx.ch) + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + /* Check tx to see if we need defer probing driver */ + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { + ret = -EPROBE_DEFER; + goto err_get_fifo_len; + } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + rs->dma_tx.ch = NULL; + } With that change your patch should be happy, I think. If some new unknown error return gets added to dma_request_slave_channel() then your code will continue to work properly. Such a change is simple and safe, so presumably you could just spin your patch with that fix. Although unlikely, it's probably good to check for IS_ERR_OR_NULL() when requesting the "rx" channel too. ...but, looking at this, presumably before landing any patch that made dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify _all_ users of dma_request_slave_channel to handle error pointers being returned. Right now dma_request_slave_channel() says it returns a pointer to a channel or NULL and the function explicitly avoids returning any errors. That might be possible, but it's a big change... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 2:03 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-22 2:03 UTC (permalink / raw) To: Doug Anderson Cc: shawn.lin, Mark Brown, linux-spi, linux-kernel, Dan Carpenter, open list:ARM/Rockchip SoC... Hi Doug, On 2016/3/22 7:33, Doug Anderson wrote: > Shawn, > > On Wed, Mar 9, 2016 at 12:11 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> Let's defer probing the driver if the return value of >> dma_request_slave_channel is ERR_PTR(-EPROBE_DEFER) instead >> of disabling dma capability directly. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/spi/spi-rockchip.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c >> index ca4f4e0..75fa990 100644 >> --- a/drivers/spi/spi-rockchip.c >> +++ b/drivers/spi/spi-rockchip.c >> @@ -737,8 +737,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) >> master->handle_err = rockchip_spi_handle_err; >> >> rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); >> - if (!rs->dma_tx.ch) >> + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { >> + /* Check tx to see if we need defer probing driver */ >> + if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { >> + ret = -EPROBE_DEFER; >> + goto err_get_fifo_len; >> + } >> dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > > Presumably Dan would be happy if you just add this right after the dev_warn(): > rs->dma_tx.ch = NULL; > > Presumably from Dan's email it would also be wise to make sure you > don't pass NULL to PTR_ERR, which you could probably do by just using > ERR_PTR instead of PTR_ERR. I think you could structure like this: > > rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* Check tx to see if we need defer probing driver */ > + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { > + ret = -EPROBE_DEFER; > + goto err_get_fifo_len; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > + rs->dma_tx.ch = NULL; > + } > > > With that change your patch should be happy, I think. If some new > unknown error return gets added to dma_request_slave_channel() then > your code will continue to work properly. Such a change is simple and > safe, so presumably you could just spin your patch with that fix. > Although unlikely, it's probably good to check for IS_ERR_OR_NULL() > when requesting the "rx" channel too. Thanks for reminding it. I was planing to fix it, so give me a little more time. :) > > ...but, looking at this, presumably before landing any patch that made > dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify > _all_ users of dma_request_slave_channel to handle error pointers > being returned. Right now dma_request_slave_channel() says it returns > a pointer to a channel or NULL and the function explicitly avoids > returning any errors. That might be possible, but it's a big > change... At first glance, it's a big change, but maybe not really. Almost all of them use the templet like: ch = dma_request_slave_channel if (!ch) balabala.... It's same for all the non-null return pointer/non-zero value ? So from my view, we can safely change dma_request_slave_channel, and leave the caller here. I presumably the respective drivers will graduately migrate to check the return value with EPROBE_DEFER if they do care this issue. Otherwise, we believe they don't suffer the changes we make, just as what they did in the past. Does that make sense? > > > -Doug > > > -- Best Regards Shawn Lin ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 2:03 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-22 2:03 UTC (permalink / raw) To: Doug Anderson Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw, Mark Brown, linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dan Carpenter, open list:ARM/Rockchip SoC... Hi Doug, On 2016/3/22 7:33, Doug Anderson wrote: > Shawn, > > On Wed, Mar 9, 2016 at 12:11 AM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote: >> Let's defer probing the driver if the return value of >> dma_request_slave_channel is ERR_PTR(-EPROBE_DEFER) instead >> of disabling dma capability directly. >> >> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> >> --- >> >> drivers/spi/spi-rockchip.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c >> index ca4f4e0..75fa990 100644 >> --- a/drivers/spi/spi-rockchip.c >> +++ b/drivers/spi/spi-rockchip.c >> @@ -737,8 +737,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) >> master->handle_err = rockchip_spi_handle_err; >> >> rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); >> - if (!rs->dma_tx.ch) >> + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { >> + /* Check tx to see if we need defer probing driver */ >> + if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { >> + ret = -EPROBE_DEFER; >> + goto err_get_fifo_len; >> + } >> dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > > Presumably Dan would be happy if you just add this right after the dev_warn(): > rs->dma_tx.ch = NULL; > > Presumably from Dan's email it would also be wise to make sure you > don't pass NULL to PTR_ERR, which you could probably do by just using > ERR_PTR instead of PTR_ERR. I think you could structure like this: > > rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* Check tx to see if we need defer probing driver */ > + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { > + ret = -EPROBE_DEFER; > + goto err_get_fifo_len; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > + rs->dma_tx.ch = NULL; > + } > > > With that change your patch should be happy, I think. If some new > unknown error return gets added to dma_request_slave_channel() then > your code will continue to work properly. Such a change is simple and > safe, so presumably you could just spin your patch with that fix. > Although unlikely, it's probably good to check for IS_ERR_OR_NULL() > when requesting the "rx" channel too. Thanks for reminding it. I was planing to fix it, so give me a little more time. :) > > ...but, looking at this, presumably before landing any patch that made > dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify > _all_ users of dma_request_slave_channel to handle error pointers > being returned. Right now dma_request_slave_channel() says it returns > a pointer to a channel or NULL and the function explicitly avoids > returning any errors. That might be possible, but it's a big > change... At first glance, it's a big change, but maybe not really. Almost all of them use the templet like: ch = dma_request_slave_channel if (!ch) balabala.... It's same for all the non-null return pointer/non-zero value ? So from my view, we can safely change dma_request_slave_channel, and leave the caller here. I presumably the respective drivers will graduately migrate to check the return value with EPROBE_DEFER if they do care this issue. Otherwise, we believe they don't suffer the changes we make, just as what they did in the past. Does that make sense? > > > -Doug > > > -- Best Regards Shawn Lin -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 2:33 ` Doug Anderson 0 siblings, 0 replies; 28+ messages in thread From: Doug Anderson @ 2016-03-22 2:33 UTC (permalink / raw) To: Shawn Lin Cc: Mark Brown, linux-spi, linux-kernel, Dan Carpenter, open list:ARM/Rockchip SoC... Shawn, On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> ...but, looking at this, presumably before landing any patch that made >> dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify >> _all_ users of dma_request_slave_channel to handle error pointers >> being returned. Right now dma_request_slave_channel() says it returns >> a pointer to a channel or NULL and the function explicitly avoids >> returning any errors. That might be possible, but it's a big >> change... > > > At first glance, it's a big change, but maybe not really. > Almost all of them use the templet like: > ch = dma_request_slave_channel > if (!ch) > balabala.... > > It's same for all the non-null return pointer/non-zero value ? > > So from my view, we can safely change dma_request_slave_channel, > and leave the caller here. I presumably the respective > drivers will graduately migrate to check the return value with > EPROBE_DEFER if they do care this issue. Otherwise, we believe > they don't suffer the changes we make, just as what they did in the > past. Does that make sense? ...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing callers, then existing callers will think you've returned a valid pointer when you really returned an error pointer. They'll pass this error pointer around like it's a valid "struct dma_chan", won't then? Actually, could your code just call dma_request_slave_channel_reason(). Oh, looks like that's exactly what you want. See commit 0ad7c00057dc ("dma: add channel request API that supports deferred probe"). Oh, but I'm looking at 4.4. Looking at linuxnext, it looks like this got renamed to dma_request_chan(). ...so you need to use that, no? Strange, but on 4.4 there was some extra code in dma_request_slave_channel() that wasn't in dma_request_slave_channel_reason(). ...but looks like that all got cleaned up in the same CL that added the new name. -Doug ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 2:33 ` Doug Anderson 0 siblings, 0 replies; 28+ messages in thread From: Doug Anderson @ 2016-03-22 2:33 UTC (permalink / raw) To: Shawn Lin Cc: Mark Brown, linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dan Carpenter, open list:ARM/Rockchip SoC... Shawn, On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote: >> ...but, looking at this, presumably before landing any patch that made >> dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify >> _all_ users of dma_request_slave_channel to handle error pointers >> being returned. Right now dma_request_slave_channel() says it returns >> a pointer to a channel or NULL and the function explicitly avoids >> returning any errors. That might be possible, but it's a big >> change... > > > At first glance, it's a big change, but maybe not really. > Almost all of them use the templet like: > ch = dma_request_slave_channel > if (!ch) > balabala.... > > It's same for all the non-null return pointer/non-zero value ? > > So from my view, we can safely change dma_request_slave_channel, > and leave the caller here. I presumably the respective > drivers will graduately migrate to check the return value with > EPROBE_DEFER if they do care this issue. Otherwise, we believe > they don't suffer the changes we make, just as what they did in the > past. Does that make sense? ...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing callers, then existing callers will think you've returned a valid pointer when you really returned an error pointer. They'll pass this error pointer around like it's a valid "struct dma_chan", won't then? Actually, could your code just call dma_request_slave_channel_reason(). Oh, looks like that's exactly what you want. See commit 0ad7c00057dc ("dma: add channel request API that supports deferred probe"). Oh, but I'm looking at 4.4. Looking at linuxnext, it looks like this got renamed to dma_request_chan(). ...so you need to use that, no? Strange, but on 4.4 there was some extra code in dma_request_slave_channel() that wasn't in dma_request_slave_channel_reason(). ...but looks like that all got cleaned up in the same CL that added the new name. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 2:53 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-22 2:53 UTC (permalink / raw) To: Doug Anderson Cc: shawn.lin, Vinod Koul, Mark Brown, linux-spi, linux-kernel, Dan Carpenter, open list:ARM/Rockchip SoC... + Vinod On 2016/3/22 10:33, Doug Anderson wrote: > Shawn, > > On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>> ...but, looking at this, presumably before landing any patch that made >>> dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify >>> _all_ users of dma_request_slave_channel to handle error pointers >>> being returned. Right now dma_request_slave_channel() says it returns >>> a pointer to a channel or NULL and the function explicitly avoids >>> returning any errors. That might be possible, but it's a big >>> change... >> >> >> At first glance, it's a big change, but maybe not really. >> Almost all of them use the templet like: >> ch = dma_request_slave_channel >> if (!ch) >> balabala.... >> >> It's same for all the non-null return pointer/non-zero value ? >> >> So from my view, we can safely change dma_request_slave_channel, >> and leave the caller here. I presumably the respective >> drivers will graduately migrate to check the return value with >> EPROBE_DEFER if they do care this issue. Otherwise, we believe >> they don't suffer the changes we make, just as what they did in the >> past. Does that make sense? > > ...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing > callers, then existing callers will think you've returned a valid > pointer when you really returned an error pointer. They'll pass this > error pointer around like it's a valid "struct dma_chan", won't then? > possibly, it depends on how caller deal with it. Should check it case by case for each caller. > Actually, could your code just call > dma_request_slave_channel_reason(). Oh, looks like that's exactly > what you want. See commit 0ad7c00057dc ("dma: add channel request API > that supports deferred probe"). Oh, but I'm looking at 4.4. Looking > at linuxnext, it looks like this got renamed to dma_request_chan(). > ...so you need to use that, no? > > Strange, but on 4.4 there was some extra code in > dma_request_slave_channel() that wasn't in > dma_request_slave_channel_reason(). ...but looks like that all got > cleaned up in the same CL that added the new name. dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but dma_request_slave_channel ignore this and rewrite it to be NULL. Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel request API that supports deferred probe") did the right thing, but what happened then? It was drop for some reasons? Hello Vinod, Could you please elaborate some more infomation to commit 0ad7c00057dc ("dma: add channel request API that supports deferred probe") :) ? > > > -Doug > > > -- Best Regards Shawn Lin ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 2:53 ` Shawn Lin 0 siblings, 0 replies; 28+ messages in thread From: Shawn Lin @ 2016-03-22 2:53 UTC (permalink / raw) To: Doug Anderson Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw, Vinod Koul, Mark Brown, linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dan Carpenter, open list:ARM/Rockchip SoC... + Vinod On 2016/3/22 10:33, Doug Anderson wrote: > Shawn, > > On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote: >>> ...but, looking at this, presumably before landing any patch that made >>> dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify >>> _all_ users of dma_request_slave_channel to handle error pointers >>> being returned. Right now dma_request_slave_channel() says it returns >>> a pointer to a channel or NULL and the function explicitly avoids >>> returning any errors. That might be possible, but it's a big >>> change... >> >> >> At first glance, it's a big change, but maybe not really. >> Almost all of them use the templet like: >> ch = dma_request_slave_channel >> if (!ch) >> balabala.... >> >> It's same for all the non-null return pointer/non-zero value ? >> >> So from my view, we can safely change dma_request_slave_channel, >> and leave the caller here. I presumably the respective >> drivers will graduately migrate to check the return value with >> EPROBE_DEFER if they do care this issue. Otherwise, we believe >> they don't suffer the changes we make, just as what they did in the >> past. Does that make sense? > > ...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing > callers, then existing callers will think you've returned a valid > pointer when you really returned an error pointer. They'll pass this > error pointer around like it's a valid "struct dma_chan", won't then? > possibly, it depends on how caller deal with it. Should check it case by case for each caller. > Actually, could your code just call > dma_request_slave_channel_reason(). Oh, looks like that's exactly > what you want. See commit 0ad7c00057dc ("dma: add channel request API > that supports deferred probe"). Oh, but I'm looking at 4.4. Looking > at linuxnext, it looks like this got renamed to dma_request_chan(). > ...so you need to use that, no? > > Strange, but on 4.4 there was some extra code in > dma_request_slave_channel() that wasn't in > dma_request_slave_channel_reason(). ...but looks like that all got > cleaned up in the same CL that added the new name. dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but dma_request_slave_channel ignore this and rewrite it to be NULL. Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel request API that supports deferred probe") did the right thing, but what happened then? It was drop for some reasons? Hello Vinod, Could you please elaborate some more infomation to commit 0ad7c00057dc ("dma: add channel request API that supports deferred probe") :) ? > > > -Doug > > > -- Best Regards Shawn Lin -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 3:33 ` Doug Anderson 0 siblings, 0 replies; 28+ messages in thread From: Doug Anderson @ 2016-03-22 3:33 UTC (permalink / raw) To: Shawn Lin Cc: Vinod Koul, Mark Brown, linux-spi, linux-kernel, Dan Carpenter, open list:ARM/Rockchip SoC... Shawn, On Mon, Mar 21, 2016 at 7:53 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > + Vinod > > > On 2016/3/22 10:33, Doug Anderson wrote: >> >> Shawn, >> >> On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin@rock-chips.com> >> wrote: >>>> >>>> ...but, looking at this, presumably before landing any patch that made >>>> dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify >>>> _all_ users of dma_request_slave_channel to handle error pointers >>>> being returned. Right now dma_request_slave_channel() says it returns >>>> a pointer to a channel or NULL and the function explicitly avoids >>>> returning any errors. That might be possible, but it's a big >>>> change... >>> >>> >>> >>> At first glance, it's a big change, but maybe not really. >>> Almost all of them use the templet like: >>> ch = dma_request_slave_channel >>> if (!ch) >>> balabala.... >>> >>> It's same for all the non-null return pointer/non-zero value ? >>> >>> So from my view, we can safely change dma_request_slave_channel, >>> and leave the caller here. I presumably the respective >>> drivers will graduately migrate to check the return value with >>> EPROBE_DEFER if they do care this issue. Otherwise, we believe >>> they don't suffer the changes we make, just as what they did in the >>> past. Does that make sense? >> >> >> ...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing >> callers, then existing callers will think you've returned a valid >> pointer when you really returned an error pointer. They'll pass this >> error pointer around like it's a valid "struct dma_chan", won't then? >> > > possibly, it depends on how caller deal with it. Should check it case by > case for each caller. > >> Actually, could your code just call >> dma_request_slave_channel_reason(). Oh, looks like that's exactly >> what you want. See commit 0ad7c00057dc ("dma: add channel request API >> that supports deferred probe"). Oh, but I'm looking at 4.4. Looking >> at linuxnext, it looks like this got renamed to dma_request_chan(). >> ...so you need to use that, no? >> >> Strange, but on 4.4 there was some extra code in >> dma_request_slave_channel() that wasn't in >> dma_request_slave_channel_reason(). ...but looks like that all got >> cleaned up in the same CL that added the new name. > > > dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but > dma_request_slave_channel ignore this and rewrite it to be NULL. > Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel > request API that supports deferred probe") did the right thing, but > what happened then? It was drop for some reasons? > > Hello Vinod, > > Could you please elaborate some more infomation to commit 0ad7c00057dc > ("dma: add channel request API that supports deferred probe") :) ? I think it's relatively straightforward. The scheme they came up with allows them to more easily update one client at a time. AKA: * If your code has been updated to handle ERR_PTR() returns, you call dma_request_slave_channel_reason(). * If your code hasn't been updated, it will still call dma_request_slave_channel(). In this case EPROBE_DEFER is treated like any other failure. That's not ideal but better than the alternative. * In recent kernels dma_request_slave_channel() was renamed to dma_request_chan(). Old code can still use dma_request_slave_channel_reason() but presumably they want you to use dma_request_chan() for new code. They are equivalent: > #define dma_request_slave_channel_reason(dev, name) dma_request_chan(dev, name) So your patch should be: - rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (!rs->dma_tx.ch) + rs->dma_tx.ch = dma_request_slave_chan(rs->dev, "tx"); + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + /* Check tx to see if we need defer probing driver */ + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { + ret = -EPROBE_DEFER; + goto err_get_fifo_len; + } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + rs->dma_tx.ch = NULL; + } ...and then a similar patch for the "rx" side of things. -Doug ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 3:33 ` Doug Anderson 0 siblings, 0 replies; 28+ messages in thread From: Doug Anderson @ 2016-03-22 3:33 UTC (permalink / raw) To: Shawn Lin Cc: Vinod Koul, Mark Brown, linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dan Carpenter, open list:ARM/Rockchip SoC... Shawn, On Mon, Mar 21, 2016 at 7:53 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote: > + Vinod > > > On 2016/3/22 10:33, Doug Anderson wrote: >> >> Shawn, >> >> On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> >> wrote: >>>> >>>> ...but, looking at this, presumably before landing any patch that made >>>> dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify >>>> _all_ users of dma_request_slave_channel to handle error pointers >>>> being returned. Right now dma_request_slave_channel() says it returns >>>> a pointer to a channel or NULL and the function explicitly avoids >>>> returning any errors. That might be possible, but it's a big >>>> change... >>> >>> >>> >>> At first glance, it's a big change, but maybe not really. >>> Almost all of them use the templet like: >>> ch = dma_request_slave_channel >>> if (!ch) >>> balabala.... >>> >>> It's same for all the non-null return pointer/non-zero value ? >>> >>> So from my view, we can safely change dma_request_slave_channel, >>> and leave the caller here. I presumably the respective >>> drivers will graduately migrate to check the return value with >>> EPROBE_DEFER if they do care this issue. Otherwise, we believe >>> they don't suffer the changes we make, just as what they did in the >>> past. Does that make sense? >> >> >> ...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing >> callers, then existing callers will think you've returned a valid >> pointer when you really returned an error pointer. They'll pass this >> error pointer around like it's a valid "struct dma_chan", won't then? >> > > possibly, it depends on how caller deal with it. Should check it case by > case for each caller. > >> Actually, could your code just call >> dma_request_slave_channel_reason(). Oh, looks like that's exactly >> what you want. See commit 0ad7c00057dc ("dma: add channel request API >> that supports deferred probe"). Oh, but I'm looking at 4.4. Looking >> at linuxnext, it looks like this got renamed to dma_request_chan(). >> ...so you need to use that, no? >> >> Strange, but on 4.4 there was some extra code in >> dma_request_slave_channel() that wasn't in >> dma_request_slave_channel_reason(). ...but looks like that all got >> cleaned up in the same CL that added the new name. > > > dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but > dma_request_slave_channel ignore this and rewrite it to be NULL. > Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel > request API that supports deferred probe") did the right thing, but > what happened then? It was drop for some reasons? > > Hello Vinod, > > Could you please elaborate some more infomation to commit 0ad7c00057dc > ("dma: add channel request API that supports deferred probe") :) ? I think it's relatively straightforward. The scheme they came up with allows them to more easily update one client at a time. AKA: * If your code has been updated to handle ERR_PTR() returns, you call dma_request_slave_channel_reason(). * If your code hasn't been updated, it will still call dma_request_slave_channel(). In this case EPROBE_DEFER is treated like any other failure. That's not ideal but better than the alternative. * In recent kernels dma_request_slave_channel() was renamed to dma_request_chan(). Old code can still use dma_request_slave_channel_reason() but presumably they want you to use dma_request_chan() for new code. They are equivalent: > #define dma_request_slave_channel_reason(dev, name) dma_request_chan(dev, name) So your patch should be: - rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (!rs->dma_tx.ch) + rs->dma_tx.ch = dma_request_slave_chan(rs->dev, "tx"); + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + /* Check tx to see if we need defer probing driver */ + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { + ret = -EPROBE_DEFER; + goto err_get_fifo_len; + } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + rs->dma_tx.ch = NULL; + } ...and then a similar patch for the "rx" side of things. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 13:12 ` Vladimir Zapolskiy 0 siblings, 0 replies; 28+ messages in thread From: Vladimir Zapolskiy @ 2016-03-22 13:12 UTC (permalink / raw) To: Doug Anderson, Shawn Lin Cc: Vinod Koul, Mark Brown, linux-spi, linux-kernel, Dan Carpenter, open list:ARM/Rockchip SoC... Hi Doug, On 22.03.2016 05:33, Doug Anderson wrote: > Shawn, > > On Mon, Mar 21, 2016 at 7:53 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> + Vinod >> >> >> On 2016/3/22 10:33, Doug Anderson wrote: >>> >>> Shawn, >>> >>> On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin@rock-chips.com> >>> wrote: >>>>> >>>>> ...but, looking at this, presumably before landing any patch that made >>>>> dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify >>>>> _all_ users of dma_request_slave_channel to handle error pointers >>>>> being returned. Right now dma_request_slave_channel() says it returns >>>>> a pointer to a channel or NULL and the function explicitly avoids >>>>> returning any errors. That might be possible, but it's a big >>>>> change... >>>> >>>> >>>> >>>> At first glance, it's a big change, but maybe not really. >>>> Almost all of them use the templet like: >>>> ch = dma_request_slave_channel >>>> if (!ch) >>>> balabala.... >>>> >>>> It's same for all the non-null return pointer/non-zero value ? >>>> >>>> So from my view, we can safely change dma_request_slave_channel, >>>> and leave the caller here. I presumably the respective >>>> drivers will graduately migrate to check the return value with >>>> EPROBE_DEFER if they do care this issue. Otherwise, we believe >>>> they don't suffer the changes we make, just as what they did in the >>>> past. Does that make sense? >>> >>> >>> ...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing >>> callers, then existing callers will think you've returned a valid >>> pointer when you really returned an error pointer. They'll pass this >>> error pointer around like it's a valid "struct dma_chan", won't then? >>> >> >> possibly, it depends on how caller deal with it. Should check it case by >> case for each caller. >> >>> Actually, could your code just call >>> dma_request_slave_channel_reason(). Oh, looks like that's exactly >>> what you want. See commit 0ad7c00057dc ("dma: add channel request API >>> that supports deferred probe"). Oh, but I'm looking at 4.4. Looking >>> at linuxnext, it looks like this got renamed to dma_request_chan(). >>> ...so you need to use that, no? >>> >>> Strange, but on 4.4 there was some extra code in >>> dma_request_slave_channel() that wasn't in >>> dma_request_slave_channel_reason(). ...but looks like that all got >>> cleaned up in the same CL that added the new name. >> >> >> dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but >> dma_request_slave_channel ignore this and rewrite it to be NULL. >> Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel >> request API that supports deferred probe") did the right thing, but >> what happened then? It was drop for some reasons? >> >> Hello Vinod, >> >> Could you please elaborate some more infomation to commit 0ad7c00057dc >> ("dma: add channel request API that supports deferred probe") :) ? > > I think it's relatively straightforward. > > The scheme they came up with allows them to more easily update one > client at a time. AKA: > > * If your code has been updated to handle ERR_PTR() returns, you call > dma_request_slave_channel_reason(). > > * If your code hasn't been updated, it will still call > dma_request_slave_channel(). In this case EPROBE_DEFER is treated > like any other failure. That's not ideal but better than the > alternative. > > * In recent kernels dma_request_slave_channel() was renamed to > dma_request_chan(). Old code can still use > dma_request_slave_channel_reason() but presumably they want you to use > dma_request_chan() for new code. They are equivalent: > >> #define dma_request_slave_channel_reason(dev, name) dma_request_chan(dev, name) > > > So your patch should be: > > - rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + rs->dma_tx.ch = dma_request_slave_chan(rs->dev, "tx"); > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* Check tx to see if we need defer probing driver */ > + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { > + ret = -EPROBE_DEFER; > + goto err_get_fifo_len; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > + rs->dma_tx.ch = NULL; > + } > referencing my answer to v2 for clarity here is my version: - rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); + if (IS_ERR(rs->dma_tx.ch)) { /* Check tx to see if we need defer probing driver */ if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { ret = -EPROBE_DEFER; goto err_get_fifo_len; } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + rs->dma_tx.ch = NULL; } You may also add some tweaks like checking for IS_ERR(rs->dma_tx.ch) in the following code instead of checking for NULL (then you don't need to do "rs->dma_tx.ch = NULL" on error), then skip "rx" channel request, if "tx" channel request failed and so on. > ...and then a similar patch for the "rx" side of things. > -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 13:12 ` Vladimir Zapolskiy 0 siblings, 0 replies; 28+ messages in thread From: Vladimir Zapolskiy @ 2016-03-22 13:12 UTC (permalink / raw) To: Doug Anderson, Shawn Lin Cc: Vinod Koul, Mark Brown, linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dan Carpenter, open list:ARM/Rockchip SoC... Hi Doug, On 22.03.2016 05:33, Doug Anderson wrote: > Shawn, > > On Mon, Mar 21, 2016 at 7:53 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote: >> + Vinod >> >> >> On 2016/3/22 10:33, Doug Anderson wrote: >>> >>> Shawn, >>> >>> On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> >>> wrote: >>>>> >>>>> ...but, looking at this, presumably before landing any patch that made >>>>> dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify >>>>> _all_ users of dma_request_slave_channel to handle error pointers >>>>> being returned. Right now dma_request_slave_channel() says it returns >>>>> a pointer to a channel or NULL and the function explicitly avoids >>>>> returning any errors. That might be possible, but it's a big >>>>> change... >>>> >>>> >>>> >>>> At first glance, it's a big change, but maybe not really. >>>> Almost all of them use the templet like: >>>> ch = dma_request_slave_channel >>>> if (!ch) >>>> balabala.... >>>> >>>> It's same for all the non-null return pointer/non-zero value ? >>>> >>>> So from my view, we can safely change dma_request_slave_channel, >>>> and leave the caller here. I presumably the respective >>>> drivers will graduately migrate to check the return value with >>>> EPROBE_DEFER if they do care this issue. Otherwise, we believe >>>> they don't suffer the changes we make, just as what they did in the >>>> past. Does that make sense? >>> >>> >>> ...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing >>> callers, then existing callers will think you've returned a valid >>> pointer when you really returned an error pointer. They'll pass this >>> error pointer around like it's a valid "struct dma_chan", won't then? >>> >> >> possibly, it depends on how caller deal with it. Should check it case by >> case for each caller. >> >>> Actually, could your code just call >>> dma_request_slave_channel_reason(). Oh, looks like that's exactly >>> what you want. See commit 0ad7c00057dc ("dma: add channel request API >>> that supports deferred probe"). Oh, but I'm looking at 4.4. Looking >>> at linuxnext, it looks like this got renamed to dma_request_chan(). >>> ...so you need to use that, no? >>> >>> Strange, but on 4.4 there was some extra code in >>> dma_request_slave_channel() that wasn't in >>> dma_request_slave_channel_reason(). ...but looks like that all got >>> cleaned up in the same CL that added the new name. >> >> >> dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but >> dma_request_slave_channel ignore this and rewrite it to be NULL. >> Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel >> request API that supports deferred probe") did the right thing, but >> what happened then? It was drop for some reasons? >> >> Hello Vinod, >> >> Could you please elaborate some more infomation to commit 0ad7c00057dc >> ("dma: add channel request API that supports deferred probe") :) ? > > I think it's relatively straightforward. > > The scheme they came up with allows them to more easily update one > client at a time. AKA: > > * If your code has been updated to handle ERR_PTR() returns, you call > dma_request_slave_channel_reason(). > > * If your code hasn't been updated, it will still call > dma_request_slave_channel(). In this case EPROBE_DEFER is treated > like any other failure. That's not ideal but better than the > alternative. > > * In recent kernels dma_request_slave_channel() was renamed to > dma_request_chan(). Old code can still use > dma_request_slave_channel_reason() but presumably they want you to use > dma_request_chan() for new code. They are equivalent: > >> #define dma_request_slave_channel_reason(dev, name) dma_request_chan(dev, name) > > > So your patch should be: > > - rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + rs->dma_tx.ch = dma_request_slave_chan(rs->dev, "tx"); > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* Check tx to see if we need defer probing driver */ > + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { > + ret = -EPROBE_DEFER; > + goto err_get_fifo_len; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > + rs->dma_tx.ch = NULL; > + } > referencing my answer to v2 for clarity here is my version: - rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); + if (IS_ERR(rs->dma_tx.ch)) { /* Check tx to see if we need defer probing driver */ if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { ret = -EPROBE_DEFER; goto err_get_fifo_len; } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + rs->dma_tx.ch = NULL; } You may also add some tweaks like checking for IS_ERR(rs->dma_tx.ch) in the following code instead of checking for NULL (then you don't need to do "rs->dma_tx.ch = NULL" on error), then skip "rx" channel request, if "tx" channel request failed and so on. > ...and then a similar patch for the "rx" side of things. > -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 13:12 ` Vladimir Zapolskiy 0 siblings, 0 replies; 28+ messages in thread From: Vladimir Zapolskiy @ 2016-03-22 13:12 UTC (permalink / raw) To: Doug Anderson, Shawn Lin Cc: Vinod Koul, Mark Brown, linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dan Carpenter, open list:ARM/Rockchip SoC... Hi Doug, On 22.03.2016 05:33, Doug Anderson wrote: > Shawn, > > On Mon, Mar 21, 2016 at 7:53 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote: >> + Vinod >> >> >> On 2016/3/22 10:33, Doug Anderson wrote: >>> >>> Shawn, >>> >>> On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> >>> wrote: >>>>> >>>>> ...but, looking at this, presumably before landing any patch that made >>>>> dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify >>>>> _all_ users of dma_request_slave_channel to handle error pointers >>>>> being returned. Right now dma_request_slave_channel() says it returns >>>>> a pointer to a channel or NULL and the function explicitly avoids >>>>> returning any errors. That might be possible, but it's a big >>>>> change... >>>> >>>> >>>> >>>> At first glance, it's a big change, but maybe not really. >>>> Almost all of them use the templet like: >>>> ch = dma_request_slave_channel >>>> if (!ch) >>>> balabala.... >>>> >>>> It's same for all the non-null return pointer/non-zero value ? >>>> >>>> So from my view, we can safely change dma_request_slave_channel, >>>> and leave the caller here. I presumably the respective >>>> drivers will graduately migrate to check the return value with >>>> EPROBE_DEFER if they do care this issue. Otherwise, we believe >>>> they don't suffer the changes we make, just as what they did in the >>>> past. Does that make sense? >>> >>> >>> ...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing >>> callers, then existing callers will think you've returned a valid >>> pointer when you really returned an error pointer. They'll pass this >>> error pointer around like it's a valid "struct dma_chan", won't then? >>> >> >> possibly, it depends on how caller deal with it. Should check it case by >> case for each caller. >> >>> Actually, could your code just call >>> dma_request_slave_channel_reason(). Oh, looks like that's exactly >>> what you want. See commit 0ad7c00057dc ("dma: add channel request API >>> that supports deferred probe"). Oh, but I'm looking at 4.4. Looking >>> at linuxnext, it looks like this got renamed to dma_request_chan(). >>> ...so you need to use that, no? >>> >>> Strange, but on 4.4 there was some extra code in >>> dma_request_slave_channel() that wasn't in >>> dma_request_slave_channel_reason(). ...but looks like that all got >>> cleaned up in the same CL that added the new name. >> >> >> dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but >> dma_request_slave_channel ignore this and rewrite it to be NULL. >> Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel >> request API that supports deferred probe") did the right thing, but >> what happened then? It was drop for some reasons? >> >> Hello Vinod, >> >> Could you please elaborate some more infomation to commit 0ad7c00057dc >> ("dma: add channel request API that supports deferred probe") :) ? > > I think it's relatively straightforward. > > The scheme they came up with allows them to more easily update one > client at a time. AKA: > > * If your code has been updated to handle ERR_PTR() returns, you call > dma_request_slave_channel_reason(). > > * If your code hasn't been updated, it will still call > dma_request_slave_channel(). In this case EPROBE_DEFER is treated > like any other failure. That's not ideal but better than the > alternative. > > * In recent kernels dma_request_slave_channel() was renamed to > dma_request_chan(). Old code can still use > dma_request_slave_channel_reason() but presumably they want you to use > dma_request_chan() for new code. They are equivalent: > >> #define dma_request_slave_channel_reason(dev, name) dma_request_chan(dev, name) > > > So your patch should be: > > - rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + rs->dma_tx.ch = dma_request_slave_chan(rs->dev, "tx"); > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* Check tx to see if we need defer probing driver */ > + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { > + ret = -EPROBE_DEFER; > + goto err_get_fifo_len; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > + rs->dma_tx.ch = NULL; > + } > referencing my answer to v2 for clarity here is my version: - rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); + if (IS_ERR(rs->dma_tx.ch)) { /* Check tx to see if we need defer probing driver */ if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { ret = -EPROBE_DEFER; goto err_get_fifo_len; } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + rs->dma_tx.ch = NULL; } You may also add some tweaks like checking for IS_ERR(rs->dma_tx.ch) in the following code instead of checking for NULL (then you don't need to do "rs->dma_tx.ch = NULL" on error), then skip "rx" channel request, if "tx" channel request failed and so on. > ...and then a similar patch for the "rx" side of things. > -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-04-05 18:08 ` Vinod Koul 0 siblings, 0 replies; 28+ messages in thread From: Vinod Koul @ 2016-04-05 18:08 UTC (permalink / raw) To: Shawn Lin Cc: Doug Anderson, Mark Brown, linux-spi, linux-kernel, Dan Carpenter, open list:ARM/Rockchip SoC... On Tue, Mar 22, 2016 at 10:53:21AM +0800, Shawn Lin wrote: > >Strange, but on 4.4 there was some extra code in > >dma_request_slave_channel() that wasn't in > >dma_request_slave_channel_reason(). ...but looks like that all got > >cleaned up in the same CL that added the new name. > > dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but > dma_request_slave_channel ignore this and rewrite it to be NULL. > Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel > request API that supports deferred probe") did the right thing, but > what happened then? It was drop for some reasons? > > Hello Vinod, > > Could you please elaborate some more infomation to commit 0ad7c00057dc > ("dma: add channel request API that supports deferred probe") :) ? The request API was cleaned up here and please see the Documentation on how to do this. See the commit a8135d0d7 - dmaengine: core: Introduce new, universal API to request a channel Thanks -- ~Vinod ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-04-05 18:08 ` Vinod Koul 0 siblings, 0 replies; 28+ messages in thread From: Vinod Koul @ 2016-04-05 18:08 UTC (permalink / raw) To: Shawn Lin Cc: Doug Anderson, Mark Brown, linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dan Carpenter, open list:ARM/Rockchip SoC... On Tue, Mar 22, 2016 at 10:53:21AM +0800, Shawn Lin wrote: > >Strange, but on 4.4 there was some extra code in > >dma_request_slave_channel() that wasn't in > >dma_request_slave_channel_reason(). ...but looks like that all got > >cleaned up in the same CL that added the new name. > > dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but > dma_request_slave_channel ignore this and rewrite it to be NULL. > Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel > request API that supports deferred probe") did the right thing, but > what happened then? It was drop for some reasons? > > Hello Vinod, > > Could you please elaborate some more infomation to commit 0ad7c00057dc > ("dma: add channel request API that supports deferred probe") :) ? The request API was cleaned up here and please see the Documentation on how to do this. See the commit a8135d0d7 - dmaengine: core: Introduce new, universal API to request a channel Thanks -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 11:59 ` Dan Carpenter 0 siblings, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2016-03-22 11:59 UTC (permalink / raw) To: Doug Anderson Cc: Shawn Lin, Mark Brown, linux-spi, linux-kernel, open list:ARM/Rockchip SoC... On Mon, Mar 21, 2016 at 04:33:32PM -0700, Doug Anderson wrote: > Presumably Dan would be happy if you just add this right after the dev_warn(): > rs->dma_tx.ch = NULL; > Yes. Thanks. > Presumably from Dan's email it would also be wise to make sure you > don't pass NULL to PTR_ERR, which you could probably do by just using > ERR_PTR instead of PTR_ERR. I think you could structure like this: > > rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* Check tx to see if we need defer probing driver */ > + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { > + ret = -EPROBE_DEFER; > + goto err_get_fifo_len; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > + rs->dma_tx.ch = NULL; > + } My check doesn't care if you pass something to PTR_ERR() that might be NULL, it complains when PTR_ERR() can only be zero. And it's ok to have the warning for a while and silence it in another patch when we update dma_request_slave_channel(). But this also works well. regards, dan carpenter ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER @ 2016-03-22 11:59 ` Dan Carpenter 0 siblings, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2016-03-22 11:59 UTC (permalink / raw) To: Doug Anderson Cc: open list:ARM/Rockchip SoC..., Shawn Lin, Mark Brown, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-spi-u79uwXL29TY76Z2rM5mHXA On Mon, Mar 21, 2016 at 04:33:32PM -0700, Doug Anderson wrote: > Presumably Dan would be happy if you just add this right after the dev_warn(): > rs->dma_tx.ch = NULL; > Yes. Thanks. > Presumably from Dan's email it would also be wise to make sure you > don't pass NULL to PTR_ERR, which you could probably do by just using > ERR_PTR instead of PTR_ERR. I think you could structure like this: > > rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* Check tx to see if we need defer probing driver */ > + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { > + ret = -EPROBE_DEFER; > + goto err_get_fifo_len; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > + rs->dma_tx.ch = NULL; > + } My check doesn't care if you pass something to PTR_ERR() that might be NULL, it complains when PTR_ERR() can only be zero. And it's ok to have the warning for a while and silence it in another patch when we update dma_request_slave_channel(). But this also works well. regards, dan carpenter ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2016-04-05 18:09 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-09 8:10 [PATCH 0/3] Some fixes for slave-dma stuff for spi-rockchip Shawn Lin 2016-03-09 8:10 ` Shawn Lin 2016-03-09 8:11 ` [PATCH 1/3] spi: rockchip: check return value of dmaengine_prep_slave_sg Shawn Lin 2016-03-09 8:11 ` Shawn Lin [not found] ` <1457511075-2134-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> 2016-03-10 3:39 ` Applied "spi: rockchip: check return value of dmaengine_prep_slave_sg" to the spi tree Mark Brown 2016-03-09 8:11 ` [PATCH 2/3] spi: rockchip: migrate to dmaengine_terminate_async Shawn Lin 2016-03-09 8:11 ` Shawn Lin [not found] ` <1457511083-2175-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> 2016-03-10 3:39 ` Applied "spi: rockchip: migrate to dmaengine_terminate_async" to the spi tree Mark Brown 2016-03-09 8:11 ` [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER Shawn Lin 2016-03-09 8:11 ` Shawn Lin [not found] ` <1457511092-2216-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> 2016-03-10 3:39 ` Applied "spi: rockchip: check requesting dma channel with EPROBE_DEFER" to the spi tree Mark Brown 2016-03-21 23:33 ` [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER Doug Anderson 2016-03-21 23:33 ` Doug Anderson 2016-03-22 2:03 ` Shawn Lin 2016-03-22 2:03 ` Shawn Lin 2016-03-22 2:33 ` Doug Anderson 2016-03-22 2:33 ` Doug Anderson 2016-03-22 2:53 ` Shawn Lin 2016-03-22 2:53 ` Shawn Lin 2016-03-22 3:33 ` Doug Anderson 2016-03-22 3:33 ` Doug Anderson 2016-03-22 13:12 ` Vladimir Zapolskiy 2016-03-22 13:12 ` Vladimir Zapolskiy 2016-03-22 13:12 ` Vladimir Zapolskiy 2016-04-05 18:08 ` Vinod Koul 2016-04-05 18:08 ` Vinod Koul 2016-03-22 11:59 ` Dan Carpenter 2016-03-22 11:59 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.