Skip to content

Port kernel driver to Linux 6.8 #8005

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 26, 2024
Merged

Port kernel driver to Linux 6.8 #8005

merged 4 commits into from
Mar 26, 2024

Conversation

keryell
Copy link
Contributor

@keryell keryell commented Mar 12, 2024

To be rebased on main once #8003 has landed.
I think the code would benefit from some review by DRM experts because I have 0 expertise. @superm1 @airlied
This is to have https://github.com/amd/xdna-driver working along
amd/xdna-driver#3

@gbuildx
Copy link
Collaborator

gbuildx commented Mar 12, 2024

keryell - is not a collaborator
Can XRT admins please validate PR

@keryell keryell mentioned this pull request Mar 12, 2024
Closed
@maxzhen maxzhen requested a review from houlz0507 March 12, 2024 04:25
@maxzhen
Copy link
Collaborator

maxzhen commented Mar 12, 2024

/build

@keryell keryell force-pushed the eventfd_signal-linux-6.8 branch from 48e0c48 to e28bed8 Compare March 12, 2024 15:56
@gbuildx
Copy link
Collaborator

gbuildx commented Mar 12, 2024

keryell - is not a collaborator
Can XRT admins please validate PR

@keryell
Copy link
Contributor Author

keryell commented Mar 12, 2024 •
edited
Loading

I have signed-off the commits now.
This PR is not supposed to be squashed as its commits and their messages are independent.

houlz0507
houlz0507 reviewed Mar 12, 2024
View reviewed changes
src/runtime_src/core/pcie/driver/linux/xocl/userpf/xocl_drv.c Outdated
if (page_sz > (PAGE_SIZE << (MAX_ORDER-1))) {
DRM_WARN("Unable to allocate with page size 0x%llx", page_sz);
#endif
DRM_WARN("Unable to allocate with page size 0x%llx", page_sz);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.
Otherwise my question I put in the comment is whether this should not be just MAX_PAGE_ORDER and MAX_ORDER here instead of (MAX_PAGE_ORDER-1) and (MAX_ORDER-1).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Thanks for mention this. NR_PAGE_ORDERS should be used instead of MAX_PAGE_ORDER.

Copy link
Contributor Author

@keryell keryell Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly. I put some links to some discussion in each commit message for reference.
So, after rereading the literature and some changes like https://lwn.net/ml/linux-kernel/[email protected]/
I think the XRT code was not pessimistic about the allocation (and at least avoided the floppy-disk bughttps://lwn.net/ml/linux-mm/[email protected]/ ;-) )
So now the modern way to write it is just

if (page_sz > (PAGE_SIZE << MAX_ORDER_NR_PAGES)) {

by using definitions from https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/include/linux/mmzone.h?id=e8f897f4afef0031fe618a8e94127a0934896aba#n34

But actually this seems like the definitive answer that has been in use for quite some Linux releases, as it it is in 5.4 and 4.18 on my laptop.

@keryell keryell force-pushed the eventfd_signal-linux-6.8 branch from e28bed8 to ff1a437 Compare March 12, 2024 16:33
@keryell
Copy link
Contributor Author

keryell commented Mar 12, 2024

I do not know if removing DRM_UNLOCKED which is no longer in use since Linux 6.3 is OK, since the driver is supposed to be compiled also on some antique kernels where it might be required.
I have no idea about the big picture of this driver.

@houlz0507
Copy link
Collaborator

I do not know if removing DRM_UNLOCKED which is no longer in use since Linux 6.3 is OK, since the driver is supposed to be compiled also on some antique kernels where it might be required. I have no idea about the big picture of this driver.

It should not be unconditionally removed. Maybe just add a Macro like
#if KERNEL_VERSION(6, 8, 0) <= LINUX_VERSION_CODE
#define DRM_UNLOCKED 0
#endif

@keryell
Copy link
Contributor Author

keryell commented Mar 12, 2024

It should not be unconditionally removed. Maybe just add a Macro

Yes I can do that.

keryell added 2 commits March 12, 2024 12:51
@keryell
eventfd_signal() has only 1 parameter starting with Linux 6.8
773dbdb 
This follows kernel commit 3652117f854819a148ff0fbe4492587d3520b5e5
https://lore.kernel.org/all/[email protected]/

Signed-off-by: Ronan Keryell <[email protected]>
@keryell
Do not use obsolete strlcpy() removed in Linux 6.8 anyway
e6be47f 
Interestingly, the newer strscpy() matches exactly the
return behavior of get_vendor_firmware_dir() API.

strlcpy() has been removed in Linux 6.8 by commit
d26270061ae66b915138af7cd73ca6f8b85e6b44.

For the context:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
KSPP/linux#89

Signed-off-by: Ronan Keryell <[email protected]>
@keryell keryell force-pushed the eventfd_signal-linux-6.8 branch from ff1a437 to 002e8ba Compare March 12, 2024 20:17
@keryell
Copy link
Contributor Author

keryell commented Mar 12, 2024

This is not clear why the test on pcie-hw-tests (xilinx_u250_gen3x16_xdma_4_1_202210_1) is not working.
How to investigate this?

@keryell
Copy link
Contributor Author

keryell commented Mar 13, 2024

XOAH shows ERROR: Not enough host mem. Please check grub settings.: Cannot allocate memory on pcie-hw-tests (xilinx_u250_gen3x16_xdma_4_1_202210_1).

keryell added 2 commits March 12, 2024 18:10
@keryell
Use portable MAX_ORDER_NR_PAGES instead of confusing MAX_ORDER
af7a239 
MAX_ORDER was replaced partially with MAX_PAGE_ORDER starting with
Linux 6.8 anyway.

Cf Linux commits fd37721803c6e73619108f76ad2e12a9aa5fafaf and
5e0a760b44417f7cadd79de2204d6247109558a0

https://lwn.net/Articles/956321/
https://lwn.net/ml/linux-kernel/[email protected]/
https://lwn.net/ml/linux-kernel/[email protected]/

Signed-off-by: Ronan Keryell <[email protected]>
@keryell
Remove DRM_UNLOCKED usage that has disappeared with Linux 6.8
72b6a9b 
This was removed in Linux with commit
2798ffcc1d6a788b5769b1fbcf0750dfc06ae98a

https://lwn.net/Articles/952344/
https://patchwork.freedesktop.org/patch/msgid/[email protected]

Signed-off-by: Ronan Keryell <[email protected]>
@keryell keryell force-pushed the eventfd_signal-linux-6.8 branch from 002e8ba to 72b6a9b Compare March 13, 2024 01:10
@keryell
Copy link
Contributor Author

keryell commented Mar 13, 2024

OK, the problem was that in af7a239 (#8005) I forgot to replace the << by *, so there was a UB operation due to the overflow, probably leading 0, so the allocation was always too big...
Ahem.

@keryell
Copy link
Contributor Author

keryell commented Mar 13, 2024

This PR is not supposed to be squashed as its commits and their messages are independent.

@keryell
Copy link
Contributor Author

keryell commented Mar 26, 2024

So, what is the problem on this PR?

@stsoe stsoe removed their request for review March 26, 2024 18:16
@stsoe stsoe merged commit 37a1ee4 into Xilinx:master Mar 26, 2024
@keryell
Copy link
Contributor Author

keryell commented Mar 26, 2024

@stsoe Thanks!

keryell added a commit to keryell/xdna-driver that referenced this pull request Apr 2, 2024
@keryell
Update XRT to current version to be able to compile with Linux 6.8
a2eb77f 
Cf Xilinx/XRT#8005
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mamin506 mamin506 mamin506 approved these changes

@houlz0507 houlz0507 houlz0507 approved these changes

@maxzhen maxzhen maxzhen approved these changes

@chvamshi-xilinx chvamshi-xilinx chvamshi-xilinx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@keryell @gbuildx @maxzhen @houlz0507 @mamin506 @chvamshi-xilinx @stsoe