Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XDMA: Add lseek() to all interfaces and add sanity checks for addresses #299

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Prandr
Copy link

@Prandr Prandr commented Nov 8, 2024

XDMA driver doesn't implement lseek for AXI Lite interface, which is not only completely illogical, because it is supposed to set the AXI addresses, but potentially problematic, because since version 6.0 Linux kernel no longer checks for null f_ops.

This pull request adds an implementation of lseek to all interface types. Additionally, SEEK_END properly sets address from the end of the address space. This was done by adding suitable entry to thexdma_dev structure.

This allowed to consolidate and add sanity checks for addresses that:

  • Are negative. Despite already present check in lseek, it is still possible to try to pass it with pwrite\pread.
  • Lie beyond (mapped BAR) address space. Current upstream version allows access to arbitrary addresses, which can wreak havoc in the kernel log.
  • are not correctly aligned. The checks are now consolidated in a common function

Please have a look, because I am not a Linux kernel driver expert, and also test for memory-mapped DMA and AXI Bridge (so called DMA Bypass)

Unify and consolidate in a function checks for addresses that are:
	-Negative
	-Exceed BAR space size
	-Not correctly aligned
@Prandr Prandr mentioned this pull request Nov 8, 2024
@hinxx
Copy link

hinxx commented Nov 9, 2024

I'm not sure having seek is useful in the case of user BAR. As pointed out in the #96 (comment) one should use mmap instead of read/write/seek. For example, in our designs the user BAR is 16MB of CSR (32-bit registers) space and we are accessing them via mmap. This is the fastest and most straightforward way to work with the registers, AFAIK, because read/write/seek introduce more syscalls and potential sleeping points due to scheduling.

which is not only completely illogical, because it is supposed to set the AXI addresses

I'm not sure what you mean with this. I never had any problems working with the user BAR so far.

FWIW, Xilinx (today AMD) has not been maintaining this repo for a long time now hence any contributions are not being merged.

[AMD managed to get XDMA DMA engine in upstream Linux kernel GIT couple of months ago and that seems to be the path forward for them. Sadly, DMA engine is not usable as an end product; one needs to create a device specific driver around the DMA engine. That is why this driver is still useful - it allows working with XDMA from userspace. OTOH, this is also its limitation ie. one can not use it to expose a v4l device from kernel or similar. We still find it useful for our application, though.]

@Prandr
Copy link
Author

Prandr commented Nov 11, 2024

I'm not sure having seek is useful in the case of user BAR. As pointed out in the #96 (comment) one should use mmap instead of read/write/seek. For example, in our designs the user BAR is 16MB of CSR (32-bit registers) space and we are accessing them via mmap. This is the fastest and most straightforward way to work with the registers, AFAIK, because read/write/seek introduce more syscalls and potential sleeping points due to scheduling.

This is not true. As I explain in the reworked answer in the linked thread, already upstream Xilinx DMA implements read/write functions, that enable access with POSIX preadand pwrite. However, if one wants to use simple writeand read, lseekis necessary to set the AXI-Lite address. This is particularly useful when accessing from other languages, like Java, which call standard read/write.

In any case there are no side effects of this pull request. All other access methods remain available.

FWIW, Xilinx (today AMD) has not been maintaining this repo for a long time now hence any contributions are not being merged.

I know that. This pull request is primarily to give other community members a chance to look at and check implementation. And perhaps someone even finds it useful, unlike you

[AMD managed to get XDMA DMA engine in upstream Linux kernel GIT couple of months ago and that seems to be the path forward for them. Sadly, DMA engine is not usable as an end product; one needs to create a device specific driver around the DMA engine. That is why this driver is still useful - it allows working with XDMA from userspace. OTOH, this is also its limitation ie. one can not use it to expose a v4l device from kernel or similar. We still find it useful for our application, though.]

This is good to know. Do you mean https://github.com/torvalds/linux/blob/master/drivers/dma/xilinx/xdma.c
Looks like a cut down version that does not define any file operations, not even ioctl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants