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
We should check return value of dmaengine_prep_slave_sg, otherwise
we take risk of null pointer.
Signed-off-by: Shawn Lin <[email protected]>
---
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
dmaengine_terminate_all is deprecated, let's use
dmaengine_terminate_async for interrupt handling.
Signed-off-by: Shawn Lin <[email protected]>
---
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
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 <[email protected]>
---
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
Shawn,
On Wed, Mar 9, 2016 at 12:11 AM, Shawn Lin <[email protected]> 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 <[email protected]>
> ---
>
> 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
Hi Doug,
On 2016/3/22 7:33, Doug Anderson wrote:
> Shawn,
>
> On Wed, Mar 9, 2016 at 12:11 AM, Shawn Lin <[email protected]> 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 <[email protected]>
>> ---
>>
>> 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
Shawn,
On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <[email protected]> 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
+ Vinod
On 2016/3/22 10:33, Doug Anderson wrote:
> Shawn,
>
> On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <[email protected]> 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
Shawn,
On Mon, Mar 21, 2016 at 7:53 PM, Shawn Lin <[email protected]> wrote:
> + Vinod
>
>
> On 2016/3/22 10:33, Doug Anderson wrote:
>>
>> Shawn,
>>
>> On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <[email protected]>
>> 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
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
Hi Doug,
On 22.03.2016 05:33, Doug Anderson wrote:
> Shawn,
>
> On Mon, Mar 21, 2016 at 7:53 PM, Shawn Lin <[email protected]> wrote:
>> + Vinod
>>
>>
>> On 2016/3/22 10:33, Doug Anderson wrote:
>>>
>>> Shawn,
>>>
>>> On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <[email protected]>
>>> 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
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