Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support

[Borislav Petkov]

On Sat, Oct 08, 2022 at 03:42:24AM +0300, Serge Semin wrote:
> Of course I did. Here is a short summary:

You didn't have to do a short summary but sure, if you prefer...

> 7. So you all decided to keep both controllers support in a single file,
> but would get back to a discussion of having separate drivers for them
> later.

Yes, pretty much.

> But after all these years of the Synopsys EDAC driver living in kernel
> we can conclude that combining the two different devices support in a
> single driver was pointless. First by the driver design nobody ever
> used the driver to handle both of them at the same time (MC index is
> fixed to zero).

So how was this supposed to work on his system?

If you have a system with two different memory controllers and you want
to have two different EDAC drivers for each, then the whole EDAC core
code needs to be audited wrt concurrency and synchronizing access to
its facilities because I don't think it has ever supported more than a
single EDAC driver per system.

And it has never needed to, at least not on x86 land. Which is
currently changing because of CXL, because of accelerators needing
RAS, GPUs needing RAS and so on and so on. So eventually we'd have to
either put the new RAS functionality in the existing chipset-specific
driver or have to go the multiple EDAC drivers route. But that's only
tangential...

So first I'd like to hear what your use case is: single EDAC driver for
your particular Baical-T1 device or you need to support multiple EDAC
drivers.

If so, why?

> Moreover in order to cover a still possible case of having both
> Synopsys uMCTL2 DDRC and Zynq A05 DDRC used on the same system in
> future I have implemented the solution 2.

See above.

> Em, if there is something else which makes the EDAC drivers to be
> impossible to co-exist on the same system, then it greatly violates
> the bus-device-driver model.

If by that you mean the aspect of a driver associating with a device and
performing operations with it then why do you assume that EDAC drivers
have to adhere to that model?

> Have you read the patchlog?

Lemme reply to it directly.

> BTW have you read the cover letter? It contains a short summary of the
> changes and their justification.

Yes, I have read it and it contains a lot of unnecessary detail which
should be in the respective patches themselves. And I still don't know
exactly what *you* are trying to do, as I said above.

A cover letter should contain a short executive summary explaining only
the goal of the patchset and then you can go into details if you prefer.
A reviewer should not have to dig into patch management details to know
what this patchset is trying to do.

A possible structure could be:

Problem is A.

It happens because of B.

Fix it by doing C.

(Potentially do D).

Btw, when you're writing your commit messages, please use passive voice
in your commit message: no "we" or "I", etc, and describe your changes
in imperative mood.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette