Re: [RFC PATCH] drm: panthor: add dev_coredumpv support

[Steven Price]

On 24/07/2024 14:15, Rob Herring wrote:
> On Wed, Jul 24, 2024 at 3:59 AM Steven Price <steven.price@xxxxxxx> wrote:
>>
>> Hi Boris,
>>
>> On 23/07/2024 17:06, Boris Brezillon wrote:
>>> Hi Steve,
>>>
>>> On Mon, 15 Jul 2024 10:12:16 +0100
>>> Steven Price <steven.price@xxxxxxx> wrote:
>>>
>>>> I note it also shows that the "panthor_regs.rs" would ideally be shared.
>>>> For arm64 we have been moving to generating system register descriptions
>>>> from a text source (see arch/arm64/tools/sysreg) - I'm wondering whether
>>>> something similar is needed for Panthor to generate both C and Rust
>>>> headers? Although perhaps that's overkill, sysregs are certainly
>>>> somewhat more complex.
>>>
>>> Just had a long discussion with Daniel regarding this panthor_regs.rs
>>> auto-generation, and, while I agree this is something we'd rather do if
>>> we intend to maintain the C and rust code base forever, I'm not
>>> entirely convinced this is super useful here because:
>>
>> So I think we need some more alignment on how the 'Rustification'
>> (oxidation?) of the driver is going to happen.
>>
>> My understanding was that the intention was to effectively start a
>> completely separate driver (I call it "Rustthor" here) with the view
>> that it would eventually replace (the C) Panthor. Rustthor would be
>> written by taking the C driver and incrementally converting parts to
>> Rust, but as a separate code base so that 'global' refactoring can be
>> done when necessary without risking the stability of Panthor. Then once
>> Rustthor is feature complete the Panthor driver can be dropped.
>> Obviously we'd keep the UABI the same to avoid user space having to care.
> 
> We did discuss this, but I've come to the conclusion that's the wrong
> approach. Converting is going to need to track kernel closely as there
> are lots of dependencies with the various rust abstractions needed. If
> we just copy over the C driver, that's an invitation to diverge and
> accumulate technical debt. The advice to upstreaming things is never
> go work on a fork for a couple of years and come back with a huge pile
> of code to upstream. I don't think this situation is any different. If
> there's a path to do it in small pieces, we should take it.

I'd be quite keen for the "fork" to live in the upstream kernel. My
preference is for the two drivers to sit side-by-side. I'm not sure
whether that's a common view though.

> What parts of the current driver are optional that we could leave out?
> Perhaps devfreq and any power mgt. That's not much, so I think the
> rust implementation (complete or partial) will always be feature
> complete.

Agreed, there's not much you can drop and still have a useful driver.

>> I may have got the wrong impression - and I'm certainly not saying the
>> above is how we have to do it. But I think we need to go into it with
>> open-eyes if we're proposing a creeping Rust implementation upstream of
>> the main Mali driver. That approach will make ensuring stability harder
>> and will make the bar for implementing large refactors higher (we'd need
>> significantly more review and testing for each change to ensure there
>> are no regressions).
> 
> This sounds to me like the old argument for products running ancient
> kernels. Don't change anything so it is 'stable' and doesn't regress.
> I think it's a question of when, not if we're going to upstream the
> partially converted driver. Pretty much the only reason I see to wait
> (ignoring dependencies) is not technical, but the concerns with
> markets/environments that can't/won't adopt Rust yet. That's probably
> the biggest issue with this patch. If converting the main driver first
> is a requirement (as discussed elsewhere in this thread), I think all
> the dependencies are going to take some time to upstream, so it's not
> something we have to decide anytime soon.

I think here's an important issues: what do we do about users who for
whatever reason don't have a Rust toolchain for their kernel build? Do
we really expect that the "other dependencies" are going to take so long
to upstream that everyone who wants this driver will have a Rust toolchain?

If we're adding new features (devcoredump) it's reasonable to say you
don't get the feature unless you have Rust[1]. If we're converting
existing functionality that's a different matter (it's a clear regression).

Having a separate code base for the Rust Panthor sidesteps the problem,
but does of course allow the two drivers to diverge. I don't have a good
solution.

[1] Although I have to admit for a debugging feature like devcoredump
there might well be pressure to implement this in C as well purely so
that customer issues can be debugged...

> Speaking of converting the main driver, here's what I've got so far
> doing that[1]. It's a top down conversion with the driver model and
> DRM registration in Rust. All the ioctls are rust wrappers calling
> into driver C code. It's compiling without the top commit.
> 
>>> 1. the C code base is meant to be entirely replaced by a rust driver.
>>> Of course, that's not going to happen overnight, so maybe it'd be worth
>>> having this autogen script but...
>>
>> Just to put my cards on the table. I'm not completely convinced a Rust
>> driver is necessarily an improvement, and I saw this as more of an
>> experiment - let's see what a Rust driver looks like and then we can
>> decide which is preferable. I'd like to be "proved wrong" and be shown a
>> Rust driver which is much cleaner and easier to work with, but I still
>> need convincing ;)
> 
> Unless your Rust is as good as your C, that's never going to happen.

Well I'd hope that there's some benefit to Rust as a language, and that
therefore it's easier to write cleaner code. Not least that in theory
there's no need to review for memory safety outside of unsafe code. I
expect I'll retire before my Rust experience exceeds my C experience
even if I never touch C again!

Steve

> Rob
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=rust/panthor-6.10