From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756114AbYLOXz1 (ORCPT ); Mon, 15 Dec 2008 18:55:27 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753848AbYLOXzK (ORCPT ); Mon, 15 Dec 2008 18:55:10 -0500 Received: from yw-out-2324.google.com ([74.125.46.31]:10959 "EHLO yw-out-2324.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753622AbYLOXzI (ORCPT ); Mon, 15 Dec 2008 18:55:08 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references:x-google-sender-auth; b=wjY0HSN4jPDhi5T2NOR6x6asrdJ8jKxCH6y9QBDTiOjUumEGErxxHWaJfBRfqCwc+e Q8Z+PsEQ6VXV/87kP4mu5CIJ8tXZzZJM7BwQf8syDQnruuhvLe9wDLj/YOZpJkzhzjP5 YpCaoX33T8XO7u43ACt1FBf2ih/aClXxIOnXY= Message-ID: Date: Mon, 15 Dec 2008 16:55:06 -0700 From: "Dan Williams" To: "Sosnowski, Maciej" Subject: Re: [PATCH 07/13] dmaengine: introduce dma_request_channel and private channels Cc: "linux-kernel@vger.kernel.org" , "netdev@vger.kernel.org" , "hskinnemoen@atmel.com" , "g.liakhovetski@gmx.de" , "nicolas.ferre@atmel.com" In-Reply-To: <129600E5E5FB004392DDC3FB599660D70C8F3406@irsmsx504.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20081114213300.32354.1154.stgit@dwillia2-linux.ch.intel.com> <20081114213453.32354.53002.stgit@dwillia2-linux.ch.intel.com> <129600E5E5FB004392DDC3FB599660D70C8F3406@irsmsx504.ger.corp.intel.com> X-Google-Sender-Auth: 7700419af256e692 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 12, 2008 at 7:29 AM, Sosnowski, Maciej wrote: >> clear_bit(DMA_INTERRUPT, dma_cap_mask_all.bits); >> + clear_bit(DMA_PRIVATE, dma_cap_mask_all.bits); >> clear_bit(DMA_SLAVE, dma_cap_mask_all.bits); > > The comment above should be updated according to this change: > -/* 'interrupt' and 'slave' are channel capabilities, but are not > +/* 'interrupt', 'private' and 'slave' are channel capabilities, but are not > ok. >> +static struct dma_chan *private_candidate(dma_cap_mask_t *mask, >> struct dma_device *dev) +{ >> + struct dma_chan *chan; >> + struct dma_chan *ret = NULL; >> + >> + /* devices with multiple channels need special handling as we need >> to + * ensure that all channels are either private or public. >> + */ >> + if (dev->chancnt > 1 && !dma_has_cap(DMA_PRIVATE, dev->cap_mask)) >> + list_for_each_entry(chan, &dev->channels, device_node) { >> + /* some channels are already publicly allocated */ >> + if (chan->client_count) >> + return NULL; >> + } > > Isn't it a dangerous approach to let clients consume for their exclusive usage channels > meant for general-purpose ("pubilc" ones)? > Why not to limit private_candidate to devices with DMA_PRIVATE capability only? > This allows unused channels to be claimed by dma_request_channel(). It is not dangerous as long as ->client_count is zero. >> + >> + list_for_each_entry(chan, &dev->channels, device_node) { >> + if (!__dma_chan_satisfies_mask(chan, mask)) { >> + pr_debug("%s: %s wrong capabilities\n", >> + __func__, dev_name(&chan->dev)); >> + continue; >> + } > > As capabilities are per device, this check could be performed just once > before list_for_each_entry(chan, &dev->channels, device_node). > Yes, changed. Thanks, Dan