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

[Serge Semin]

On Thu, Oct 06, 2022 at 03:25:42PM +0200, Borislav Petkov wrote:
> On Thu, Oct 06, 2022 at 03:17:40PM +0300, Serge Semin wrote:
> > 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]).
> 

> Did you read that thread until the end?

Of course I did. Here is a short summary:

1. @Punnaiah described that he got a Zynq system with two different
DDR controllers. One of them was Synopsys DW uMCTL2 DDRC (ARM cortex A9
PS) and another one - Zynq A05 DDRC (Xilinx PL). All of these DDR
controllers could be probed in Linux. It was required to detect the ECC
errors for both of them.

2. @Punnaiah suggested that both of these controllers should have been
handled by two different drivers since the controllers were different.
But he was reasonably afraid that there could be a race condition for
the mc_num assignment since it was the drivers responsibility to
allocate one.

3. @Punnaiah suggested two solutions: 1) Keep two drivers in single file
and use static global variable for tracking the mc_num. 2) Let the
framework assign the mc_num to the edac driver.

4. @Punnaiah preferred the solution 2, but you said that the solution
1 would do it.

5. @Michal was against mixing up the support of two different devices
in a single driver. He reasonably assumed that the driver would get
to look messy. 

6. @Boris disagreed saying that it won't be messy and any
communication between the two memory controllers could be done
internally in that driver.

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.

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). Second there is no even a glimpse of communications
between two devices in the system. Third there are two different set
of macros and multiple platform-specific methods and if-flag-else
statements fully abstracting out the devices implementation from the
EDAC core.  All of that has made the driver overcomplicated in-vain
seeing none of the reasons of these complications have been realised.
Meanwhile extending further the Synopsys uMCTL2 DDR controller
functionality support would mean adding new macros, functions,
platform-specific flags, parameters and device-specific data. That
would have made the driver even more complicated. Dropping the
abstraction layer out and splitting up the drivers was the best choice
in the current circumstances especially seeing the driver author and
@Michal preferred that solution in the first place.

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.

> 
> > 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.
> 

> So lemme ask this again because the last time it all sounded weird and I
> don't think it got clarified fully: you cannot have more than one memory
> controller type at the same time on those systems, can you?

To be clear I don't work with the Xilinx systems. So I can only say based
on your discussion with @Punnaiah on the initial driver review.
@Punnaiah said they could have more than one memory controller type on
their systems. That's why he described the problem with the MC indexes
allocation and suggested to combine the devices support in a single
driver.

> 
> Because if you can and you want to support that, the current EDAC
> "design" allows to have only a single EDAC driver per system. So you
> can't do two drivers now.

I do understand that.

> 
> If your answer to that is
> 
> Subject: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure
> 
> then I'm sceptical but I'd need to look at that first.

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.

> 
> And reading your commit messages, you're talking a lot about what you're
> doing. But that should be visible from the patch. What I wanna read is
> *why* you're doing it.

Each patchlog has a thorough enough description of "why".

> 
> Like in this patch above, what's that "unique index allocation
> procedure" for?

Have you read the patchlog? 

> 
> edac_mc_alloc() already gets a mc_num as the MC number.
> 
> And yes, if you want to do multiple driver instances like x86 does per
> NUMA node instances, then that is done with edac_mc_alloc() which gives
> you a memory controller descriptor and then you can deal with those.
> 

See the "EDAC/mc: Add MC unique index allocation procedure" patch. It
also provides a way to assign the indexes based on the OF-aliases.

> From all the text it sounds to me you want to add a separate driver for
> that Zynq A05 thing but I might still be missing something...

I do want to detach the Zynq A05 device support from out of the
Synopsys uMCTL2 driver.

BTW have you read the cover letter? It contains a short summary of the
changes and their justification. It seems as if you started your
review straight from this patch.

-Sergey

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