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

WIP: Simple Rust driver that touches real hardware #254

Closed

Conversation

TheSven73
Copy link
Collaborator

@TheSven73 TheSven73 commented May 7, 2021

(find history and TODO at the end)

Proof-of-concept of a bcm2835-rng Rust driver. This is the hardware
random-number generator present on Raspberry Pi Zero(W), Classic,
Two, and Three. It's a convenient starting point because:

  • it's ubiquitous: a Pi Zero can be purchased for $10
  • it has QEMU Support (-M raspi2)
  • it's very simple: just 0x10 bytes of register space

The hwrng is exposed as a Rust miscdev named rust_hwrng. Reading
its devnode will produce up to 4 random bytes at a time:

pi@raspberrypi:~$ hexdump -C /dev/rust_hwrng
00000000  ef 9c 19 8a                                       |....|
00000004

Tested on a real Raspberry Pi Zero-W, and QEMU (-M raspi2).

Consider this to be a "pencil outline": most of the new Rust abstractions
I've introduced here are clunky, inelegant and incomplete - my Rust is very
poor. But I'm sure that collective wisdom can improve them. The unsafe
sections need careful review too.

Rust abstractions/infrastructure were introduced for the following kernel concepts:

  • struct platform_device / struct platform_driver
  • per-device driver data
  • struct regmap

How to run on QEMU

Download a Raspbian image. I used 2021-03-04-raspios-buster-armhf-lite.img.
It will consist of two partitions. Discover their offsets using:

$ fdisk -l 2021-03-04-raspios-buster-armhf-lite.img
Device                                    Boot  Start     End Sectors  Size Id Type
2021-03-04-raspios-buster-armhf-lite.img1        8192  532479  524288  256M  c W95 FAT32 (LBA)
2021-03-04-raspios-buster-armhf-lite.img2      532480 3645439 3112960  1.5G 83 Linux

Mount the second partition on your PC: (note how the offset is multiplied by 512)

$ mount -o loop,offset=$((512*532480)) 2021-03-04-raspios-buster-armhf-lite.img /mnt
Comment out everything in /etc/ld.so.preload - otherwise the Raspbian rootfs cannot support
a mainline kernel:
$ vi /etc/ld.so.preload # comment everything out
$ umount /mnt

Build the kernel for arm 32-bit:

$ make bcm2835_defconfig # defconfig modded so `bcm2835-rng` binds to Rust
$ make zImage dtbs modules

Start QEMU:

  # to boot mainline, make sure that /etc/ld.so.preload is commented out
  # in the Raspbian image.
qemu-system-arm \
    -M raspi2 \
    -append "rw earlyprintk loglevel=8 console=ttyAMA0,115200 dwc_otg.lpm_enable=0 root=/dev/mmcblk0p2 rootwait" \
    -cpu arm1176 \
    -dtb bcm2836-rpi-2-b.dts \
    -hda ./2021-03-04-raspios-buster-armhf-lite.img \
    -kernel zImage \
    -m 1G \
    -smp 4 \
    -nographic \
;

How to run on a Raspberry Pi Zero(W)

Follow the instructions for QEMU above. Deploy the Raspbian image to SD card.
Copy zImage and bcm2835-rpi-zero-w.dtb to Raspbian's first (boot) partition:

	zImage			-> boot partition: kernel.img
	bcm2835-rpi-zero-w.dtb	-> boot partition: bcm2708-rpi-0-w.dtb

If you'd like wifi to keep working, also copy the kernel modules you built to
Raspbian's second partition:

$ make modules_install INSTALL_MOD_PATH=<somewhere>
$ cp -rfa <somewhere> <Raspbian Partition> # should end up in /lib/modules/5.12.0-rc4+/

Signed-off-by: Sven Van Asbroeck [email protected]

v1 -> v2:

soruh:

  • eliminate NOPANIC annotation
  • add safety comment, allow Box type to be inferred

ojeda:

  • various comment improvements
  • initialize val to eliminate unsafe code
  • eliminate C types in public APIs
  • capitalize and put period at the end of comments
  • move Regmap helper functions into Regmap class (as static members)
  • limit reg_stride to u8, which helps eliminate errors
  • remove spurious log message
  • remove unused module params field
  • move driver to drivers/char/hw_random

wedsonaf:

  • don't force Box, use PointerWrapper instead
  • fork bcm2835_defconfig into bcm2835_rust_defconfig
  • provide explicit remove callback (with TODO)

TheSven73:

  • remove redundant placeholder type
  • add TODO for Regmap and void __iomem *` lifetimes
  • add TODO: use a struct hwrng instead of a miscdev
  • remove redundant #[cfg(CONFIG_REGMAP_MMIO)]
  • eliminate incorrectly named PointerWrapper

Important TODOs:

  • the of_table should ideally be variable size, created at build time, be const, using only safe Rust.
  • investigate lifetime management for Regmap.
  • Use Rust type system to make Regmap API safer #260 Use Rust type system to make Regmap API safer.
  • use a struct hwrng instead of a miscdev.
  • do drivers ever need to override remove() ?
  • choose which ptr_err_check() function to use.

Copy link
Member

@ojeda ojeda left a comment

Choose a reason for hiding this comment

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

Great work! A few comments.

rust/kernel/platform_driver.rs Outdated Show resolved Hide resolved
rust/kernel/platform_driver.rs Outdated Show resolved Hide resolved
rust/kernel/regmap.rs Outdated Show resolved Hide resolved
rust/kernel/regmap.rs Outdated Show resolved Hide resolved
rust/kernel/regmap.rs Outdated Show resolved Hide resolved
rust/kernel/regmap.rs Outdated Show resolved Hide resolved
rust/kernel/regmap.rs Outdated Show resolved Hide resolved
samples/rust/rust_platdev.rs Outdated Show resolved Hide resolved
samples/rust/rust_platdev.rs Outdated Show resolved Hide resolved
samples/rust/rust_platdev.rs Outdated Show resolved Hide resolved
Copy link

@adamrk adamrk left a comment

Choose a reason for hiding this comment

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

Very cool!

rust/kernel/platform_driver.rs Show resolved Hide resolved
@@ -104,3 +104,20 @@ impl From<AllocError> for Error {
Error::ENOMEM
}
}

pub(crate) unsafe fn ptr_err_check<T>(ptr: *mut T) -> KernelResult<*mut T> {
Copy link

Choose a reason for hiding this comment

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

It would be nice for this to be an associated function of KernelResult (e.g. KernelResult::check_ptr) but that would require a trait (as KernelResult is a type alias to Result) would that be worth it / easy to include in the prelude?

Alternatives include having this be Error::check_ptr, keeping the freestanding function and have associated functions call it or just leaving it this way.

We probably also want this to be called check_ptr_mut and have and analogous check_ptr.

(Note that I have very little experience with kernel code and thus the applicability of IS_ERR so this may be unnecessary)

Copy link
Collaborator Author

@TheSven73 TheSven73 May 10, 2021

Choose a reason for hiding this comment

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

Would you be ok with moving this discussion to @fbq's #109 ? (I will rebase to his implementation when it gets merged)

return Err(Error::EINVAL);
}
// PANIC: this will never panic: `compatible` is not longer than `buf`.
buf[..compatible.len()].copy_from_slice(compatible.as_bytes());
Copy link

Choose a reason for hiding this comment

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

This could probably be

let dest = buf.get_mut(..compatible.len()).ok_or(Error::EINVAL)?;
    
dest.copy_from_slice(compatible.to_bytes());

which removes the PANIC comment / the duplicated bounds check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice! Can you (or anyone) think of a way to perform the check at build time?

Copy link

@soruh soruh May 8, 2021

Choose a reason for hiding this comment

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

I don't think there is a way with the current function signature as the length of the &CStr is only decided at runtime from the view of the function. It may be possible to do this with a macro but I don't know if the additional complexity that would introduce is worth the one bounds check. I'll see if I can write something that's not too complex if I find the time.

EDIT: I don't think there is a good way to do this as the &CStr is propagated through quite a few functions before getting here which all would need to be macros for this to work.
(All of these functions could have a const N: usize generic and pass around an [u8; N] instead of a &CStr but the complexity that would introduce is extreme)

Copy link

Choose a reason for hiding this comment

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

I thought about this a bit more and I guess it would be possible take the [i8; 128] (possibly wrapped in a newtype) as an argument instead of a &CStr (which needs to be propagated to new_pinned etc.) and then use a macro to create that from a string literal at compile time when calling these function (instead of cstr!). That way the ergonomics impact would not be that big but I still don't know if that is worth it and if the compiler can sufficiently optimise the array copies to be faster than a single bounds check.

rust/kernel/platform_driver.rs Outdated Show resolved Hide resolved
Copy link

@wedsonaf wedsonaf left a comment

Choose a reason for hiding this comment

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

Very nice!

I haven't reviewed everything yet but sending my comments so far.

arch/arm/configs/bcm2835_defconfig Outdated Show resolved Hide resolved
@@ -104,3 +104,20 @@ impl From<AllocError> for Error {
Error::ENOMEM
}
}

pub(crate) unsafe fn ptr_err_check<T>(ptr: *mut T) -> KernelResult<*mut T> {
Copy link

Choose a reason for hiding this comment

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

@fbq has a different approach to this in #109. We should discuss which one we'd rather have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I hadn't spotted fbq's implementation yet, thank you! Both implementations seem similar, looks like a choice between relying on the kernel's IS_ERR()/PTR_ERR() versus coding this directly in Rust.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I'm OK with either implementation. One thing I wonder is whether Rust compiler could inline a extern "C" function? If it could, then I think @TheSven73 implementation is better, because we don't need to worry about kernel modifying the errno range (although it's a very rare case).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had a look at the arm32 assembly code generated for the kernel. It doesn't look like the rust_helper_is_err()/ptr_err() is being inlined:

00014024 <_RNvMs0_NtCsbDqzXfLQacH_6kernel6regmapNtB5_6Regmap27init_mmio_platform_resource>:
   14024:       e92d48f0        push    {r4, r5, r6, r7, fp, lr}
   14028:       e24dd0a8        sub     sp, sp, #168    ; 0xa8
   1402c:       e1a06000        mov     r6, r0
   14030:       e5900000        ldr     r0, [r0]
   14034:       e1a04002        mov     r4, r2
   14038:       ebfffffe        bl      0 <devm_platform_ioremap_resource>
   1403c:       e1a05000        mov     r5, r0
   14040:       ebfffffe        bl      0 <rust_helper_is_err>

I guess there's a trade-off between performance/efficiency (an extra function call) and safety/compatibility (kernel changing its definitions on us).

Is it ok if we discuss this in a separate PR, and defer the decision? We can keep #109 as the place where these trade-offs are discussed. If #109 gets merged first. I'll rebase to it. If the opposite happens, you can rebase to mine. In both cases, the main branch will contain the consensus implementation of ptr_to_result() after both PRs are merged.

Or I'm open to anything else you'd like to suggest.

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense if we have a specific PR for this change. I will make one and use my implementation, feel free to put any thought on it.

Copy link
Member

Choose a reason for hiding this comment

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

done #267

Copy link
Member

Choose a reason for hiding this comment

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

In general, I think we want to go for re-using the C functions/macros where possible. Performance work should be based on actual numbers, which we can focus on later. And, anyway, we should be able to have cross-language LTO at some point, which will most likely inline those.

rust/kernel/platform_driver.rs Outdated Show resolved Hide resolved
rust/kernel/platform_driver.rs Outdated Show resolved Hide resolved
Comment on lines 89 to 99
// TODO should create a variable size table here.
this.of_table[0] = new_of_device_id(&of_id)?;
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at how we do this for chrdev: https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/chrdev.rs#L23

We use const generics to size the registration, and then you can call register() up to N times

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks !! I did try something along those lines. I ran into the following issue: an of table with N compatible strings has N+1 entries. When I tried to use N+1, rustc told me to activate some extra features. And when I switched those on, rustc told me it could result in compiler crashes...

Copy link
Member

Choose a reason for hiding this comment

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

Some of the const_fn features are still unstable, but I think they solve this well, so I think it makes sense to attempt to try that out -- if it's geniunely non-functional, then we could try a different approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks ! I believe at least two alternatives were suggested during last Saturday's call: const_fn and procedural macros. Does it make sense to prefer one over the other, i.e. which one to try first?

@kloenk expressed interest in writing a procmac for of_table. Finn, are you still ok with that? Would you like to try out @alex's const_fn suggestion as well?

There's probably more here than meets the eye. of_table's data member allows drivers to distinguish between device "flavours" and adjust accordingly. For example:

static const struct of_device_id bcm2835_rng_of_match[] = {
	{ .compatible = "brcm,bcm2835-rng"},
	{ .compatible = "brcm,bcm-nsp-rng", .data = &nsp_rng_of_data },
	{ .compatible = "brcm,bcm5301x-rng", .data = &nsp_rng_of_data },
	{ .compatible = "brcm,bcm6368-rng"},
	{},
};

The data above indicates that the 5301 and nsp-rng variants require interrupt masking. I guess the appropriate Rust abstraction here would be an enum? Pass an optional enum variant for every table entry?

Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to prefer one over the other, i.e. which one to try first?

In general, it is best to avoid macros, in particular proc ones. We should reach for them when there is no other way to do something.

We may also want to use them if they really improve/simplify some interface for users or if they allow for a much easier/cleaner implementation. But, in general, if something can be done without them, it is usually the best way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kloenk let me know if you would like to look into a const_fn way of doing this.

Copy link
Member

Choose a reason for hiding this comment

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

I could look at it, not sure if I can make it before Thursday.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No worries at all. I'll place a TODO: in the v2 PR I'm likely to push before Thu.

}

struct RngDevice {
state: Arc<SharedState>,
Copy link
Collaborator Author

@TheSven73 TheSven73 May 10, 2021

Choose a reason for hiding this comment

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

(edited to remove incorrect stuff)

I think we can get rid of the Arc here: Regmap contains only a shareable reference to the actual kernel abstraction struct regmap, which is thread-safe to access through its helper functions. So Regmap can be Clone.

But there's a deeper issue here: the lifetime of the underlying struct regmap is tied to the lifetime of our device - or rather the struct device we received from the kernel on probe(). It's of course perfectly possible in Rust to send a Regmap all over the place, and keep it even after the device was released. Calling functions on a stale Regmap/struct regmap will result in a use-after-free !

Do we need to look for a Rust abstraction that ensures that, on driver remove, there are no more handles to kernel objects with struct device lifetimes? Such kernel objects are always created using functions that start with devm_.

Our struct regmap is such an object, because it was created using **devm_**regmap_init_mmio.

Maybe we need a follow-up Issue for this, to discuss?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about this some more. Many (if not most) drivers use devm_ lifetime management: kernel objects created using devm_xxx(struct device *dev, ...) all have lifetimes that are tied to the lifetime of the struct device. Right before the device is released/deallocated by the kernel, all associated objects are automatically cleaned up (in reverse order of creation, and by calling their release functions).

Here is how the struct regmap's lifetime is controlled in our platform driver:

int probe(struct platform_device *pdev)
{
    struct device *dev = &pdev->dev; /* platform device "is-a" device */
    void __iomem *iomem = devm_ioremap(dev, ...);
    struct regmap *regmap = devm_regmap_init_mmio(dev, iomem);
}

int remove(struct platform_device *pdev)
{
    /* no need to explicitly clean up regmap or iomem, or care about the order:
     * the kernel takes care of that right before the platform device is cleaned up
     */
}

And now the driver writer no longer has clean up the struct regmap, or care about its lifetime: it'll get cleaned up automatically.

So how can we leverage Rust to verify this at build time? Do we confer a lifetime on the struct device's Rust abstraction, which corresponds to the actual kernel device lifetime? And do we transfer that lifetime onto Rust abstractions of kernel objects which are tied to the device lifetime?

Alternatively, we can forego the devm_ kernel mechanism completely (it's optional), and manually manage our own kernel resources in Rust. Then we still need to watch out for linked lifetimes: the struct regmap depends on the iomem, for example. And some kernel objects may still depend on a struct device, even when not devm_ managed.

If the Rust abstraction of the struct regmap needs to link to its dependencies, things may get messy, because regmaps may be created from various underlying resources: iomem, spi, i2c, clocks... I hope we won't need a Regmap type for every regmap variant ?! There are a great many possible combinations.

@alex
Copy link
Member

alex commented May 11, 2021

Looks like we've got some conflicts here, likely as a result of #259.

@TheSven73
Copy link
Collaborator Author

Thanks for the heads-up! I have a rebase in-flight, which I'll push once the CI goes green in my own GitHub repo.

Proof-of-concept of a `bcm2835-rng` Rust driver. This is the hardware
random-number generator present on Raspberry Pi Zero(W), Classic,
Two, and Three. It's a convenient starting point because:
- it's ubiquitous: a Pi Zero can be purchased for $10
- it has QEMU Support (-M raspi2)
- it's very simple: just 0x10 bytes of register space

The hwrng is exposed as a Rust `miscdev` named `rust_hwrng`. Reading
its devnode will produce up to 4 random bytes at a time:
pi@raspberrypi:~$ hexdump -C /dev/rust_hwrng
00000000  ef 9c 19 8a                                       |....|
00000004

Tested on a real Raspberry Pi Zero-W, and QEMU (-M raspi2).

Consider this to be a "pencil outline": most of the new Rust abstractions
I've introduced here are clunky, inelegant and incomplete - my Rust is very
poor. But I'm sure that collective wisdom can improve them. The `unsafe`
sections need careful review too.

Rust abstractions/infrastructure were introduced for the following kernel concepts:
- `struct platform_device` / `struct platform_driver`
- per-device driver data
- `struct regmap`

How to run on QEMU
==================
Download a Raspbian image. I used `2021-03-04-raspios-buster-armhf-lite.img`.
It will consist of two partitions. Discover their offsets using:
```sh
$ fdisk -l 2021-03-04-raspios-buster-armhf-lite.img
Device                                    Boot  Start     End Sectors  Size Id Type
2021-03-04-raspios-buster-armhf-lite.img1        8192  532479  524288  256M  c W95 FAT32 (LBA)
2021-03-04-raspios-buster-armhf-lite.img2      532480 3645439 3112960  1.5G 83 Linux
```

Mount the second partition on your PC: (note how the offset is multiplied by 512)
```sh
$ mount -o loop,offset=$((512*532480)) 2021-03-04-raspios-buster-armhf-lite.img /mnt
Comment out everything in /etc/ld.so.preload - otherwise the Raspbian rootfs cannot support
a mainline kernel:
$ vi /etc/ld.so.preload # comment everything out
$ umount /mnt
```

Build the kernel for arm 32-bit:
```sh
$ make bcm2835_defconfig # defconfig modded so `bcm2835-rng` binds to Rust
$ make zImage dtbs modules
```

Start QEMU:
```sh
  # to boot mainline, make sure that /etc/ld.so.preload is commented out
  # in the Raspbian image.
qemu-system-arm \
    -M raspi2 \
    -append "rw earlyprintk loglevel=8 console=ttyAMA0,115200 dwc_otg.lpm_enable=0 root=/dev/mmcblk0p2 rootwait" \
    -cpu arm1176 \
    -dtb bcm2836-rpi-2-b.dts \
    -hda ./2021-03-04-raspios-buster-armhf-lite.img \
    -kernel zImage \
    -m 1G \
    -smp 4 \
    -nographic \
;
```

How to run on a Raspberry Pi Zero(W)
====================================
Follow the instructions for QEMU above. Deploy the Raspbian image to SD card.
Copy zImage and bcm2835-rpi-zero-w.dtb to Raspbian's first (boot) partition:
```
	zImage			-> boot partition: kernel.img
	bcm2835-rpi-zero-w.dtb	-> boot partition: bcm2708-rpi-0-w.dtb
```
If you'd like wifi to keep working, also copy the kernel modules you built to
Raspbian's second partition:
```sh
$ make modules_install INSTALL_MOD_PATH=<somewhere>
$ cp -rfa <somewhere> <Raspbian Partition> # should end up in /lib/modules/5.12.0-rc4+/
```

Signed-off-by: Sven Van Asbroeck <[email protected]>
Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

I only gave this a brief review. Right now this PR is pretty large, are there any good ways for us to spit this up into pieces we could review and merge individually?

Does it make sense to do the probe support without regmap, for example?

.copy_from_slice(compatible.as_bytes());
Ok(bindings::of_device_id {
// SAFETY: re-interpretation from [u8] to [i8] of same length is always safe.
compatible: unsafe { transmute::<[u8; 128], [i8; 128]>(buf) },
Copy link
Member

Choose a reason for hiding this comment

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

Is i8 right here? Isn't char signed on some platforms and unsigned on others? I think this should be c_char.

Comment on lines +28 to +33
let f = || {
let drv_data = T::probe(&mut PlatformDevice::new(pdev))?;
let drv_data = drv_data.into_pointer() as *mut c_types::c_void;
Ok(drv_data) as Result<_>
};
let ptr = match f() {
Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit odd stylistically to use a function for this, I think there's a macro in file_operations that might be more what you want here?

@TheSven73
Copy link
Collaborator Author

Right now this PR is pretty large, are there any good ways for us to spit this up into pieces we could review and merge individually?

@alex would it make sense for you (or other reviewers) if I split this into two PRs?

  1. Introduce platform_device + probe + DrvData + hwrng driver. but hwrng driver does not touch h/w and reads only zeroes.
  2. Introduce regmap + iomem + touching actual hwrng h/w.

@alex
Copy link
Member

alex commented May 12, 2021 via email

@TheSven73
Copy link
Collaborator Author

I'll proceed with that. But... it was mentioned during the meeting that we'll need separate commits for the Rust core and any client drivers, right?

So once both PRs are reviewed and merged, an admin will have to squash the master branch (containing two PRs consisting of two commits each) into separate core and client driver commits.

@alex or @ojeda are you ok with doing this?

@alex
Copy link
Member

alex commented May 12, 2021 via email

@ojeda
Copy link
Member

ojeda commented May 12, 2021

it was mentioned during the meeting that we'll need separate commits for the Rust core and any client drivers, right?

Ideally, yes.

So once both PRs are reviewed and merged, an admin will have to squash the master branch (containing two PRs consisting of two commits each) into separate core and client driver commits.

This is something I do when I apply the commits to rust-next. I never modify rust.

For commits, the best is that you split them as much as possible (without breaking the build and within reason). If it is possible, I would do something like platform_device, then probe, then DrvData, then regmap, then iomem, then finally the actual hwrng driver. Such split should already take care of the "core / drivers" split too.

As for PRs, due to how the GitHub UI works, I would go for small ones too -- even one PR per commit.

@TheSven73
Copy link
Collaborator Author

That's a lot of commits and PRs :)

You are suggesting 6 commits and 6 PRs? Each PR introduces a new commit, and contains the commits from previous PRs, correct? How would we manage the dependencies? When a PR changes, subsequent PRs must be manually pushed/updated? Sounds like plenty of rope to hang ourselves. Except if GitHub has features to make this easy?

@ojeda
Copy link
Member

ojeda commented May 12, 2021

The policy for commits is the same as for the kernel. For PRs, the reason I mention sending small ones is that it allows for less iterations and more focused discussion in the GitHub UI, so they should go "in" way faster (I would simply wait for the previous one to get merged before sending the next one). But please feel free to send it however you think it makes the most sense.

@TheSven73
Copy link
Collaborator Author

Sounds good, I'll go with that.

@TheSven73
Copy link
Collaborator Author

Moved to #270

ojeda pushed a commit that referenced this pull request Jun 11, 2024
… rules

rx_create no longer allocates a modify_hdr instance that needs to be
cleaned up. The mlx5_modify_header_dealloc call will lead to a NULL pointer
dereference. A leak in the rules also previously occurred since there are
now two rules populated related to status.

  BUG: kernel NULL pointer dereference, address: 0000000000000000
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  PGD 109907067 P4D 109907067 PUD 116890067 PMD 0
  Oops: 0000 [#1] SMP
  CPU: 1 PID: 484 Comm: ip Not tainted 6.9.0-rc2-rrameshbabu+ #254
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS Arch Linux 1.16.3-1-1 04/01/2014
  RIP: 0010:mlx5_modify_header_dealloc+0xd/0x70
  <snip>
  Call Trace:
   <TASK>
   ? show_regs+0x60/0x70
   ? __die+0x24/0x70
   ? page_fault_oops+0x15f/0x430
   ? free_to_partial_list.constprop.0+0x79/0x150
   ? do_user_addr_fault+0x2c9/0x5c0
   ? exc_page_fault+0x63/0x110
   ? asm_exc_page_fault+0x27/0x30
   ? mlx5_modify_header_dealloc+0xd/0x70
   rx_create+0x374/0x590
   rx_add_rule+0x3ad/0x500
   ? rx_add_rule+0x3ad/0x500
   ? mlx5_cmd_exec+0x2c/0x40
   ? mlx5_create_ipsec_obj+0xd6/0x200
   mlx5e_accel_ipsec_fs_add_rule+0x31/0xf0
   mlx5e_xfrm_add_state+0x426/0xc00
  <snip>

Fixes: 94af50c ("net/mlx5e: Unify esw and normal IPsec status table creation/destruction")
Signed-off-by: Rahul Rameshbabu <[email protected]>
Signed-off-by: Tariq Toukan <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants