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
