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@xxxxxxxxxxxxxx> 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