linux-kernel - Re: [PATCH] [RFC] Xilinx MPMC SDMA subsystem

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]

Message-Id: <1271780995.3067.79.camel@iscandar.digidescorp.com>
Date:	Tue, 20 Apr 2010 11:29:55 -0500
From:	"Steven J. Magnani" <steve@...idescorp.com>
To:	Sergey Temerkhanov <temerkhanov@...dex.ru>
Cc:	linuxppc-dev@...ts.ozlabs.org,
	Grant Likely <grant.likely@...retlab.ca>,
	microblaze-uclinux@...e.uq.edu.au,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [RFC] Xilinx MPMC SDMA subsystem

Hi Sergey,

I've only just started using this in earnest, sorry for not getting back
to you sooner. It's a nice encapsulation of the MPMC/SDMA functionality,
thanks for posting it.

In order to integrate this into my system, I refactored the bus
attachment code and added hooks for platform bus. I also removed some
dead code, reformatted some things to satisfy checkpatch, tweaked
#includes to fix Microblaze compilation, and fixed a potential bug where
sdma_set_coalesce() could return without releasing a spinlock. I also
optimized the sdma_desc_* functions by moving any byte swapping from
runtime to compile-time.

Some more controversial changes / items for discussion:

1. I dropped setting the tail descriptor in the sdma_[rt]x_init()
functions since that would start DMA, which is not what I think we want.

2. I made RX and TX interrupts optional. There are use cases (DMAing
while atomic) in which interrupts are not necessary. The DMA engine only
needs RX interrupts. There is an (obscure) mode in which it might also
want TX interrupts, and in that case it's only interested in error
interrupts - normal "done" interrupts are of no interest whatsoever.
Rather than try to adapt the sdma driver to fit that case, I think I
will drop that mode from the DMA engine driver.

2A. I will need, but haven't added yet, methods to know if a SDMA
channel has RX and TX IRQ resources. I'm assuming that a simple inline
accessor is preferred over snooping struct sdma directly.

3. I changed the user[4] field of struct sdma_desc to individually-named
fields app1 - app4, to match the MPMC datasheet. I found user[0]
confusing and already had to fix a bug where I had coded user[0]
thinking it was app0, when I really should have specified stat_ctl.

4. Why have sdma_[rt]x_submit() return a value if it is always zero?

5. I would like to see the 'virt' and 'flags' fields removed from struct
sdma_desc and SDMA_ALIGNMENT reduced from 0x40 to 0x20. Neither field is
used in the sdma driver itself. I understand why 'virt' is there, but
having it in the struct will make the DMA engine driver less efficient.
Because the DMA engine operates on 'loopback' SDMA channels it always
allocates descriptors in pairs. Also the DMA engine framework already
provides storage for the 'virt' pointer. Having a larger-than-necessary
structure would force the DMA engine to do larger allocations from its
DMA pool - instead of 64 bytes per dual descriptor, it would have to
allocate 128.

6. I'm concerned that there is no concept of "allocating" a channel,
something like a sdma_device_get() / sdma_device_put() pair that would
prevent concurrent access to a SDMA device by removing the device from
consideration by sdma_find_device().

7. In that same vein, I'm curious about the need for a list of
sdma_clients. Is there a use case for this in your systems?

8. It would probably make sense to have sdma_init() fail with -EEXIST if
a SDMA device with the specified phandle already exists (-1 being an
exception).

9. I didn't resolve the issue of what to name the files / API, assuming
'sdma' is a little too generic for things that are now publicly visible.
If we have to change it, some suggestions are 'mpmcsdma' (long, but
precise), 'xildma', 'xsdma', or 'xdma' (also perhaps too generic).

As time permits, I'll work on refactoring the DMA engine driver to use
the sdma driver - I'll post change requests for anything else I need
rather than modifying the sdma code directly.

Regards,
------------------------------------------------------------------------
 Steven J. Magnani               "I claim this network for MARS!
 www.digidescorp.com              Earthling, return my space modulator!"

 #include <standard.disclaimer>

View attachment "sdma2.patch" of type "text/x-patch" (28131 bytes)