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

[Serge Semin]

On Fri, Sep 30, 2022 at 04:42:31PM +0200, Borislav Petkov wrote:
> On Fri, Sep 30, 2022 at 02:27:09AM +0300, Serge Semin wrote:
> > It was a bad idea in the first place to combine two absolutely different
> > controllers support in a single driver [1]. It caused having an additional
> > level of abstraction, which obviously have needlessly overcomplicated the
> > driver and as such caused many problems in the new main controller
> > features support implementation. The solution looks even more unreasonable
> > now seeing the justification of having both controllers support in a
> > single driver hasn't been implemented by the original code author [2].
> 

> Yeah, no, you need to give more concrete details here.
> 
> Why exactly is this a problem?

In general because it needlessly overcomplicates the driver, worsen
it scalability, maintainability and readability, makes it much harder
to add new controller features. Moreover even if you still able to add
some more features support the driver will get to be more and more messy
(as Michal correctly said in the original thread [1]). It will get to
be messy since you'll need to add more if-flag-else conditionals or
define new device-specific callbacks to select the right code path for
different controllers; you'll need to define some private data
specific to one controller and unused in another. All of that you
already have in the driver and all of that would be unneeded should the
driver author have followed the standard kernel bus-device-driver model.

Regarding this particular case. This series and two more patchsets
mentioned in the cover letter are about extending the Synopsys uMCTL2
DDRC controller functionality support in the driver. Here is just the
most significant part of the being added features:
1. DW uCMTL2 IP-core parameters auto-detection (DDR bus-width, frequency
ratio, burst-length, ECC-mode, etc).
2. Common SDRAM<->phys address translation.
3. Multi-ranked memory support.
4. ECC-scrubber and Scrubber IRQ support.
5. Individual IRQ signals support.
6. DFI alert_n IRQ support.
7. Add reference clock sources support.

None of the above can be utilized for the Zynq A05 DDR controller, but
is of the Synopsys uMCTL2 DDRC specific. Seeing the controllers are
not even of the same vendor and are absolutely incompatible,
considering all the new features, moving the Zynq A05 DDRC support
away to another driver was the best choice in order to create a
coherent and easy-to-read code, provide simple-enough patches for
review. The standard kernel bus-device-driver model is perfectly
suitable to differentiate these controllers. There is no point in
creating an additional abstraction.

> 
> Are you saying that if synopsys puts out 10 incompatible memory
> controllers, we should have 10 separate EDAC drivers?

I said "absolutely different controllers" in the patchlog. The level
of incompatibility can be different and can vary from case to case. In
this particular case there is nothing in common in Synopsys uMCTL2
DDRC and Zynq A05 DDRC. So if there are two absolutely different
devices at consideration then in general they should be handled in
different drivers as per the kernel bus-device-driver model.

Note having absolutely different devices detectable on the same
platform doesn't mean they should be handled by the same driver
otherwise the kernel would have had tons of common platform-drivers
instead of the device-specific ones. Even in current synopsys EDAC
driver state (without my patches applied) should you detach the Zynq
A05 DDRC support to another driver, the code would have got to be much
easier to read and maintain.

I one more time draw your attention to the fact that the original
reason of having both Synopsys uMCTL2 and Zynq A05 DDRC support in the
same driver has never been implemented [2]. See edac_mc_alloc()
is always called with index zero in the driver. Even despite of that I
still provide a way to solve the problem denoted in [2] in the patch
"EDAC/mc: Add MC unique index allocation procedure" in this patchset.

> 
> Hell no.
> 

> synopsys_edac.c is not a huge file and the probe logic which matches
> which synps_platform_data to load is pretty straight-forward to me.

The synps_platform_data interface is so abstractive so you could have
added any basic EDAC device support to the driver. It doesn't mean you
should even if the devices can be detected on the same platform,
especially if you expect the code to evolve further being extended with
new more complex features.

> 
> But maybe I'm missing something so please explain in detail what the
> actual problems with this are?

In this case I'd say the KISS principle applies. Simplification will
be reached by splitting the driver up only. I understand that it
couldn't have been done back then due to the original driver author
claiming the EDAC core to require the MC auto-index generation [2].
That's why I have added such functionality in the framework of this
series.

[1] https://lore.kernel.org/all/808655a9-77eb-4e3a-9781-2b059ad9517b@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
[2] https://lore.kernel.org/all/9dc2a947-d2ab-4f00-8ed3-d2499cb6fdfd@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

-Sergey

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette