linux-kernel - Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 25 Jul 2011 13:08:42 -0700
From:	"Williams, Dan J" <dan.j.williams@...el.com>
To:	Jaswinder Singh <jaswinder.singh@...aro.org>
Cc:	Linus Walleij <linus.walleij@...aro.org>,
	linux-kernel@...r.kernel.org, vinod.koul@...el.com,
	rmk+kernel@....linux.org.uk, linus.walleij@...ricsson.com,
	per.friden@...ricsson.com, wei.zhang@...escale.com,
	ebony.zhu@...escale.com, iws@...o.caltech.edu,
	s.hauer@...gutronix.de, maciej.sosnowski@...el.com,
	saeed@...vell.com, shawn.guo@...escale.com, yur@...raft.com,
	agust@...x.de, iwamatsu.nobuhiro@...esas.com,
	per.forlin@...ricsson.com, jonas.aberg@...ricsson.com,
	anemo@....ocn.ne.jp
Subject: Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

On Mon, Jul 25, 2011 at 12:17 PM, Jaswinder Singh
<jaswinder.singh@...aro.org> wrote:
> On 26 July 2011 00:25, Williams, Dan J <dan.j.williams@...el.com> wrote:
>>>> Unless you guaranteed that every id is globally unique I don't see how
>>>> they are generically usable by common clients?
>>>>
>>> Yes. And the first step is to allow DMAC drivers to freely set chan_id value.
>>> Platform could pass the list and mapping of supported 'global channels'
>>> via platform_data(?) which the DMAC drivers could set in chan_id
>>> And I am not sure of defining a new variable for that, because chan_id
>>> is actually used only by some dmac drivers for internal purpose only -
>>> which they could do by private variables.
>>
>> Yes, it does appear to have grown a lot of dubious usage in
>> drivers/dma/ for something that is described as just a "channel ID for
>> sysfs".  The intent was for drivers that need to maintain their own
>> hardware specific identification to define a local id scheme (like
>> iop-adma does in the iop3xx case).
>
> You mean chan_id is meant to be used for _internal_ purposes by DMAC drivers ?
>
> Shouldn't there instead be just something like 'void *dmac_data' for
> DMAC drivers to hang their private info on?
>
> Sorry, I couldn't find chan_id in drivers/dma/iop-adma.c
> Do I misunderstand ?

/**
 * struct iop_adma_device - internal representation of an ADMA device
 * @pdev: Platform device
 * @id: HW ADMA Device selector
 * @dma_desc_pool: base of DMA descriptor region (DMA address)
 * @dma_desc_pool_virt: base of DMA descriptor region (CPU address)
 * @common: embedded struct dma_device
 */
struct iop_adma_device {
        struct platform_device *pdev;
        int id;
        dma_addr_t dma_desc_pool;
        void *dma_desc_pool_virt;
        struct dma_device common;
};

Where that 'id' is the internal identifier and chan_id from struct
dma_chan is ignored.

>> I can sort of see why it is attractive for the goal of having clients
>> being able to talk to multiple DMACs since it is a field that all
>> channels already implement.  But, I don't think we should be mixing
>> sysfs presentation details with a capability that indicates a certain
>> compatibility level in a DMAC driver implementation.  If the goal is
>> to be able to use a single client with multiple drivers that, to me,
>> is asking for a new capability bit (dma_transaction_type) rather than
>> an id.  Do you have an example of the client and the DMACs that would
>> first take advantage of such a cross DMAC compatibility?
>>
> Apparently I fail to explain my well. let me ask you this....
>
> Some DMAC drivers initialize chan_id before calling dma_async_device_register().
> And dma_async_device_register() overwrites the chan_id _always_.
>
> Clearly only _one_ of them should be setting chan_id. Which was meant to be?

Correct, it is meant that chan_id is only a sysfs property.  Any
driver usage that is assuming chan_id is anything more than a
guaranteed unique number within a given dma_device's list of channels
is probably inferring too much.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/