[PATCH 0/3] Some fixes for slave-dma stuff for spi-rockchip

[PATCH 0/3] Some fixes for slave-dma stuff for spi-rockchip

[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

[PATCH 1/3] spi: rockchip: check return value of dmaengine_prep_slave_sg

[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

[PATCH 2/3] spi: rockchip: migrate to dmaengine_terminate_async

[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

[PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

[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

Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

[Doug Anderson]

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

Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

[Shawn Lin]

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

Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

[Doug Anderson]

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

Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

[Shawn Lin]

+ 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

Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

[Doug Anderson]

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

Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

[Vladimir Zapolskiy]

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

Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

[Vinod Koul]

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

Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

[Dan Carpenter]

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