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

[Serge Semin]

On Wed, Oct 12, 2022 at 07:28:48PM +0200, Borislav Petkov wrote:
> 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...

It seemed to me as I should have so to make sure we are on the page of
the original discussion understanding.

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

How am I supposed to know? You should have asked him at the time of
his patches review before accepting them. He (Punnaiah Choudary
Kalluri) said they had the system with two different DDR controllers.
Since the MC idx was supposed to be provided by the controller driver
he suggested to have both devices support implemented in the same
driver. You agreed. If no interaction intended between the two devices
why did he need to combine their support in the same driver then? He
could have as well just split the drivers up and didn't bother with
all the discussions.

> 
> If you have a system with two different memory controllers 

I don't. The Synopsys EDAC driver author (Punnaiah) did judging by
what he said in your discussion.

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

Once again. What is driver-depended do you have in the EDAC core?
Does it follow the bus-device-drivers model? I failed to find any
inter-driver dependency (what could be seen for instance in the
tty/serial subsystem). But I am not the subsystem maintainer after all.
I found the possible races in the MC index registration and fixed it
in the corresponding patch. But that was the just
registration-specifics behavior which could be easily fixed in the
same way as the most of the kernel subsystem do.

You are worried about the concurrencies. Does the EDAC core have
problems if there are several DDR devices of the same type probed?
AFAICS it doesn't as long as the indexes are properly allocated by the
driver. What is the problem with registering devices of different
types? The error counters are the MC-data-specific, so are the
sysfs-nodes. The EDAC MC error handler function doesn't touch any
inter-device parts of the core. The registration procedure is
protected by the mutex and RCU. So it seems as the EDAC core developer
thought about having the devices being registered concurrently.

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

If it has never needed to, then please explain why did you let the
Synopsys EDAC driver being accepted like that then? 

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

In my case it's a single EDAC driver per-chip. There can be several
DDR-controllers installed on the same SoC, but all of them of the same
type (Synopsys DW uMCTL2 v2.61a).

> 
> If so, why?

I don't have a system with different DDR controllers detected on the
same SoC but Punnaiah Choudary Kalluri, original Synopsys EDAC driver
developer, did.

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

The main part of the letter starts exactly with the goals and then has
text with more details of what is done and why. It is enough to get a
notion regarding the patchset aim and content. Each patchlog starts
with the problem description, the suggested solution and some details
of the implementation I thought was required to add. Everything in
accordance with the kernel patches standards.

-Sergey

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