From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759124AbcCVMAu (ORCPT ); Tue, 22 Mar 2016 08:00:50 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:45091 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758715AbcCVMAn (ORCPT ); Tue, 22 Mar 2016 08:00:43 -0400 Date: Tue, 22 Mar 2016 14:59:56 +0300 From: Dan Carpenter To: Doug Anderson Cc: Shawn Lin , Mark Brown , linux-spi@vger.kernel.org, "linux-kernel@vger.kernel.org" , "open list:ARM/Rockchip SoC..." Subject: Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER Message-ID: <20160322115956.GY5273@mwanda> References: <1457511043-2058-1-git-send-email-shawn.lin@rock-chips.com> <1457511092-2216-1-git-send-email-shawn.lin@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Subject: Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER Date: Tue, 22 Mar 2016 14:59:56 +0300 Message-ID: <20160322115956.GY5273@mwanda> References: <1457511043-2058-1-git-send-email-shawn.lin@rock-chips.com> <1457511092-2216-1-git-send-email-shawn.lin@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: "open list:ARM/Rockchip SoC..." , Shawn Lin , Mark Brown , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Doug Anderson Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: linux-spi.vger.kernel.org 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