Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751769AbcCVDd2 (ORCPT ); Mon, 21 Mar 2016 23:33:28 -0400 Received: from mail-vk0-f50.google.com ([209.85.213.50]:34048 "EHLO mail-vk0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751143AbcCVDd0 (ORCPT ); Mon, 21 Mar 2016 23:33:26 -0400 MIME-Version: 1.0 In-Reply-To: <56F0B3A1.9080509@rock-chips.com> 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> Date: Mon, 21 Mar 2016 20:33:24 -0700 X-Google-Sender-Auth: KcYjAAXMG6a1-Jyv7C4D9LtP9aQ Message-ID: Subject: Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER From: Doug Anderson To: Shawn Lin Cc: Vinod Koul , Mark Brown , linux-spi@vger.kernel.org, "linux-kernel@vger.kernel.org" , Dan Carpenter , "open list:ARM/Rockchip SoC..." Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4224 Lines: 111 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; + } ...and then a similar patch for the "rx" side of things. -Doug