Page MenuHomeFreeBSD

Add Makefile to create if_cgem.ko kernel module
ClosedPublic

Authored by bsd_dino.sk on Mar 28 2022, 5:44 AM.

Details

Summary

For development, building a driver as kernel module is both convenient and time saver (no need for reboot on some change, testing it requires just kldunload and kldload, a matter of seconds). For some special cases, it may be even desirable to postpone initializing the network interface after some action is done (loading a FPGA bitstream may be required for Zynq/ZynqMP based hardware as an example).
Building is limited to ARM, ARM64 and RISCV architectures (for Zynq, ZynqMP, PolarFire Soc based boards, and HiFive based boards are known to use CGEM at the moment).

Test Plan

Checked locally on Zybo Z7/Cora Z7 boards (arm architecture), also on PolarFire SoC based RCHD-PF board from Conclusive Engineering (riscv architecture), module loads and works.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Additional note: Makefile originally provided by Thomas Skibo, I'd like to attribute it properly.

mhorne added reviewers: skibo, mhorne.
mhorne added a subscriber: mhorne.

Minor notes, but this looks good!

In the future, please upload diffs with full context -- git show -U99999.

sys/modules/Makefile
814

I would place this block elsewhere in the file, but admittedly the file is a bit of a mess. I can do this before commit.

sys/modules/cgem/Makefile
6 ↗(On Diff #104266)

It is slightly preferred style to place any .c sources on a separate line. Might as well list the headers alphabetically as well.

Minor notes, but this looks good!

In the future, please upload diffs with full context -- git show -U99999.

Thanks for hint, I'll try to remember for the next time.

sys/modules/Makefile
814

Probably this place was chosen because similar .if blocks are already there, so this one is just 'one more like this' item. I have no idea if it could be done some other way, maybe better, but it works. If something else is proposed instead, I'll test it.

sys/modules/cgem/Makefile
6 ↗(On Diff #104266)

Absolutely no objection. Technically, it works either way, so if this is preferred, I am OK with it.

I tested this yesterday on a Zybo and it autoloaded just fine.

sys/modules/cgem/Makefile
1 ↗(On Diff #104266)

I think these tags aren't suppose to go into any new files.

Hi, the patch does not apply correctly to the main branch. Can you rebase and re-upload the diff with context?

One final suggestion: the new subfolder should be named sys/modules/if_cgem, to match the module name.

Hi, the patch does not apply correctly to the main branch. Can you rebase and re-upload the diff with context?

It isn't obvious why the patch fails but it looks like the patch has spaces (lines 84-90) whereas the Makefile has tabs. Someone must've done a whitespace clean-up.

Hi, the patch does not apply correctly to the main branch. Can you rebase and re-upload the diff with context?

One final suggestion: the new subfolder should be named sys/modules/if_cgem, to match the module name.

I'll re-create the patch tomorrow. spaces/tabs mismatch could be the cause.

This diff was done with -U99999 and module directory was renamed to sys/modules/if_cgem. Hopefully there is no space/tabs issue, now it should apply correctly.

This diff was done with -U99999 and module directory was renamed to sys/modules/if_cgem. Hopefully there is no space/tabs issue, now it should apply correctly.

Thanks, it does apply. I will commit shortly.

This revision is now accepted and ready to land.Apr 2 2022, 6:17 PM
This revision was automatically updated to reflect the committed changes.