Page MenuHomeFreeBSD

linuxkpi v5.0 updates
AbandonedPublic

Authored by johalun on Mar 12 2019, 7:39 PM.

Details

Summary

First I was considering splitting it up in smaller chunks for the reviews but that would be at least 7 different reviews. I put it all here for starters. If needed I can create smaller reviews before committing. I intend to commit in smaller chunks.

Included changes (roughly the chunks I intend to commit in):
cdev: Make modules unload without crash or memory leak. Works for drm.ko so far but graphic drivers still leak memory and doesn't clean up properly at unload.
timers: Update del_timer() according to upstream changes.
[committed] list: Add a couple of list functions.
[committed] sysinfo: Add linux sysinfo struct and helper method to populate it. NOTE: not sure what to put in si->totalhigh here...
tasklet: Use counter for enable/disable to match upstream. This requires moving the state out of the queue member.
[committed] pci: drm_pci functions has moved to kernel internals.
other: Various other minor updates.

This patch includes code and suggested improvements from https://reviews.freebsd.org/D18041 and will make D18041 obsolete.

Test Plan

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I'll have a look at these patches tomorrow. Most of them look trivial.

Hi,

Did you build LINT with these changes and nothing breaks?

--HPS

Did you build LINT with these changes and nothing breaks?

No. What is the correct procedure to do this?

make -C sys/amd64/conf LINT
make KERNCONF=LINT -jX buildkernel

sys/compat/linuxkpi/common/include/linux/cdev.h
147 ↗(On Diff #54998)

This patch is not correct. I get a double free during module unload "mlx4ib" and "mlx5ib" with this patch. I suspect there is an FD still hanging around that you need to close. Can you give some more background on this one?

Hi Johan,

Can you rebase your patchset on top of r345109?

--HPS

You need to explain why nanouptime() is replaced by nanotime().

You need to explain why nanouptime() is replaced by nanotime().

I think this had me confused https://github.com/freebsd/freebsd/blame/master/sys/compat/linuxkpi/common/include/linux/ktime.h#L232 (ktime_get_raw and ktime_get_raw_ns differ)
It probably should be uptime for all that's not *_real*.
As for ktime_get_raw_ts(), I will remove that since not used anywhere.

johalun edited the summary of this revision. (Show Details)
  • Rebase with upstream r345127.
  • Clean up ktime.h
  • Patch two drivers that are affected by change to access_ok()

Left to do:

  • cdev related: Investigate why mlx crash on unload with double free - perhaps fixing drm kldunload requires changes to drm/linuxkpi_gplv2 instead of lkpi_common?
  • pci: Make the code use native KPI instead of LinuxKPI - probably best in a separate review since it's already committed.
sys/compat/linuxkpi/common/include/linux/mm.h
95 ↗(On Diff #55055)

This should go in linux/mm_types.h

sys/compat/linuxkpi/common/include/linux/preempt.h
38

I don't think td_critnest is quite right here, but I can't figure out anything obviously better. I think you might be able to do something with td_priority >= PI_SOFT or some such, but I don't know.

johalun added inline comments.
sys/compat/linuxkpi/common/include/linux/preempt.h
38

Yeah, I'm not sure at all about this one so any feedback is welcome...

Revert cdev patch that caused double free for mlx* driver.
Added: Don't call cdev_init() when cdev_alloc() is called. This fixes memory leaks that occur when unloading drm.

Add linux_pci_unregister_drm_driver()

sys/compat/linuxkpi/common/include/asm/uaccess.h
55 ↗(On Diff #55166)

Can you give an example where user_access_begin() is already defined?

sys/compat/linuxkpi/common/include/linux/device.h
64 ↗(On Diff #55166)

You will need to bump the __FreeBSD_version when changing this structure.

sys/compat/linuxkpi/common/include/linux/dma-mapping.h
179 ↗(On Diff #55166)

Commit style changes separately.

sys/compat/linuxkpi/common/include/linux/interrupt.h
192

Spelling:

impl->implementation :
Our implementation is different. Avoid same name as Linux.

sys/compat/linuxkpi/common/include/linux/uaccess.h
63

Can you given an example where access_ok is already defined?

sys/compat/linuxkpi/common/src/linux_tasklet.c
72
struct tasklet_struct *ts;
struct tasklet_struct *last;
77
last = TAILQ_LAST(&tw->head);
98
if (ts == last)
    break;
98

Your assumption that TAILQ_FIRST() will always go into the TAILQ_INSERT_TAIL() case does not hold. Please update the code like suggested.

153

atomic_set(&ts->_state, TASKLET_ST_IDLE);

221

atomic already has a barrier. I think this one can be removed.

237

Ditto.

johalun added inline comments.
sys/compat/linuxkpi/common/include/asm/uaccess.h
55 ↗(On Diff #55166)

In drm 4.16 code. linuxkpi_gplv2 headers are included before base.

https://github.com/FreeBSDDesktop/kms-drm/blob/drm-v4.16-fbsd13.0/linuxkpi/gplv2/include/linux/compiler.h#L41

What we actually could do is putting back the LINUXKPI_VERSION macro and define all versions here... This solution was also recommended by Warner.

#define LINUXKPI_VERSION XXX   <- users declare this
#if LINUXKPI_VERSION < 5000 (or however many zeros we want)
#define user_access_begin()
#else 
#define user_access_begin(ptr, len) access_ok(ptr, len)
#endif
sys/compat/linuxkpi/common/include/linux/device.h
64 ↗(On Diff #55166)

Yes we intend to bump the version anyways.

sys/compat/linuxkpi/common/include/linux/dma-mapping.h
179 ↗(On Diff #55166)

Ok. Will do when committing.

sys/compat/linuxkpi/common/include/linux/interrupt.h
192

Ok. I shortened it to avoid exceeding 80 chars.

sys/compat/linuxkpi/common/include/linux/uaccess.h
63

See user_access_begin above

sys/compat/linuxkpi/common/src/linux_tasklet.c
98

Oh yes, I see now what you mean.

johalun marked 4 inline comments as done.

Implement recommended tasklet changes.

Use LINUXKPI_VERSION macro to check what KPI version the client expects, as per discussion. Any code that uses LinuxKPI will define this. We will support at most 2 versions concurrently and it is recommended that that any code that depends on LINUXKPI_VERSION < 50000 is updated to most recent version.

The version is defined as XXXYY, where XXX is Linux version (for example 4.16 = 416, 5.0 = 500) and YY is for our minor version.

sys/compat/linuxkpi/common/include/linux/random.h
38 ↗(On Diff #56626)

Please use function macro.

Hi, Can you commit the register chardev fixes first and rebase?

This revision was not accepted when it landed; it landed in state Needs Review.Apr 25 2019, 9:54 PM
This revision was automatically updated to reflect the committed changes.

Rebase after committing r347470

Check LINUXKPI_VERSION before declaring IS_ALIGNED (declared in gplv2 code for < 5.0) macro to avoid build error with older out of tree code.
Use function macro for get_random_u32 as suggested by reviewer.

Everything committed!