Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932902AbcCVNMg (ORCPT ); Tue, 22 Mar 2016 09:12:36 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:58948 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932761AbcCVNMK (ORCPT ); Tue, 22 Mar 2016 09:12:10 -0400 Subject: Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER To: Doug Anderson , Shawn Lin References: <1457511043-2058-1-git-send-email-shawn.lin@rock-chips.com> <1457511092-2216-1-git-send-email-shawn.lin@rock-chips.com> <56F0A7F0.5000900@rock-chips.com> <56F0B3A1.9080509@rock-chips.com> CC: Vinod Koul , Mark Brown , , "linux-kernel@vger.kernel.org" , Dan Carpenter , "open list:ARM/Rockchip SoC..." From: Vladimir Zapolskiy Message-ID: <56F144A3.7070309@mentor.com> Date: Tue, 22 Mar 2016 15:12:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Icedove/38.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [137.202.0.76] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5341 Lines: 139 Hi Doug, On 22.03.2016 05:33, Doug Anderson wrote: > Shawn, > > On Mon, Mar 21, 2016 at 7:53 PM, Shawn Lin wrote: >> + Vinod >> >> >> On 2016/3/22 10:33, Doug Anderson wrote: >>> >>> Shawn, >>> >>> On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin >>> 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