io32_memcpy

[SebastianBoe]

Implement memcpy32, a version of memcpy that only performs 32 bit
word-wise copying. (useful for MMIO).

Based on Pete Dietl's implementation and the mcan implementation.

Also make use of it from mcan.

I was not able to find a way to build mcan.

But I have done some simple testing locally.

A second attempt at #41273

[martinjaeger]

I was not able to find a way to build mcan.

It's the backend for a few other CAN-FD drivers, but you can't build it directly. Instead you can e.g. build any CAN sample or test for nucleo_g474re board.

[galak]

Are you doing this specifically to have a memcpy32 for IO purposes?

If so, should rename to memcpy_io32 or something like that.

[SebastianBoe]

@galak : I will rename as such, do you have any other feedback?

I'd prefer to wait until all feedback is given in case there is some feedback that completely blocks the PR.

[alexanderwachter]

Looks good to me

[andyross]

Nitpickery: Can we name this something less generic? Maybe sys_wordcopy32() or something? Calling it "memcpy" and putting it in util.h makes it sound like something regular apps are expected to use. And while, sure, this can be implemented in pure C it exists only to honor MMIO requirements of hardware with behavior way outside the C standard.

Also, note that this is sort of a can of worms. What if the hardware has ordering requirements? Does the copy work identically forward and backwards? Hardware can have prefetch units and write combining that messes this up even if the software is doing everything right.

I guess my worry is this: this meets the requirements of that one piece of MCAN hardware (and on the particular SOC integration!). How many other devices are actually going to productively use it? My guess "not enough to make this a standard-looking memcpy".

[alexanderwachter]

@andyross m_can is actually a driver backend used on many SoC.
But this is just a remark.

[henrikbrixandersen]

@SebastianBoe What use-case do you have for this outside of the M_CAN driver? I tend to agree with @andyross.

[SebastianBoe]

@henrikbrixandersen : My use-case was to copy data from nRF->UICR->OTP MMIO, that had this limitation, to RAM. So in my case I eventuallly found that it's awkward that the function requires the buffer in RAM to be word-aligned.

I'm not sure about what to do about that. Maybe it's trivial to enforce word-aligned RAM on the buffer being written to.

Maybe the MCAN use-case also actually only needs word-aligned enforcement on the read, and not the write.

I intend to investigate and address comments.

But I want to wait a bit for feedback so I don't have to revisit this many times.

[SebastianBoe]

"Can we name this something less generic? Maybe sys_wordcopy32() or something?"
-- @andyross

I'd like to keep the term memcpy and 32, but we can add io, or sys, or both.

memcpy32 can be familiar to other developers:
https://chromium.googlesource.com/chromiumos/platform/punybench/+/refs/heads/stabilize-gnawty-5841.84.B/cpu.m/memcpy.c

https://lwn.net/Articles/165519/

and clearly communicates that it's a variant of memcpy. wordcopy is not as clear.

[alexanderwachter]

@SebastianBoe the reason why we need to copy in 32bit chunks is that the lower two address lines are not present in the m_can. If you use the byte wise copy, all four bytes of the 32bits end up in the first byte.

[SebastianBoe]

All feedback has been addressed.

@andyross , @galak

[pdgendt]

Please update the commit subjects to indicate the actual changes

• util: memcpy32
• drivers: mcan

Aren't useful

[cfriedt]

probably can remove this line

[carlocaione]

do we really need a Kconfig option for this?

[carlescufi]

Agreed, @SebastianBoe is this really needed?

[SebastianBoe]

Yes, there is no pre-existing source file that the function belongs in.

And we don't want to unconditionally invoke gcc for something that is rarely used as it slows down the build and creates noise in the build.

[andyross]

I still have a vague sense that this is too specific an API to be useful, but the name is no longer ambiguous and I don't see why it shouldn't be merged.

[cfriedt]

@alexanderwachter - do you have any input here?

[alexanderwachter]

I have two comments still open. Bit no blockers.

[stephanosio]

What would be a reason not to introduce io32_memset here if we are doing this?

[SebastianBoe]

Out-of-scope for this PR.

A PR that adds this would be welcome.

[stephanosio]

There needs to be a test for this.

[SebastianBoe]

"There needs to be a test for this."
I disagree.

It is trivial enough to be proof-read to be correct. And it is system-tested as it is used by mcan.

[stephanosio]

I disagree.

It is trivial enough to be proof-read to be correct. And it is system-tested as it is used by mcan.

While the function itself is simple enough, who is to say it will not break on some architectures and toolchains because of some ABI-specific details and/or compiler optimisations?

It is very important that we sanity-check/validate basic functions like these to ensure that they behave as intended.

Also, this is technically an API. Every API function ought to have a corresponding test.

[cfriedt]

"There needs to be a test for this." I disagree.

It is trivial enough to be proof-read to be correct. And it is system-tested as it is used by mcan.

I was just bitten by this. Code was "trivial enough to be proof-read to be correct" but it actually introduced a subtle bug that could have been easily avoided by including a test. It was before I became POSIX maintainer, but I still was on the approve list.

Pay your future self and the future selves of maintainers please 😁

[SebastianBoe]

@stephanosio : Unfortunately I am unable to prioritize spending time adding a test for this. Please, in future reviews, try to leave all comments in one pass to avoid multiple review iterations.