On Wed, Oct 12, 2022 at 07:29:22PM +0200, Borislav Petkov wrote:

On Fri, Sep 30, 2022 at 02:27:08AM +0300, Serge Semin wrote:

quoted

In case of the unique index allocation it's not that optimal to always
rely on the low-level device drivers (platform drivers), because they get
to start to implement either the same design pattern (for instance global
static MC counter) or may end-up with having non-unique index eventually
at runtime. Needless to say that having a generic unique index
allocation/tracking procedure will make code more readable and safer.

I guess this is trying to say that the current memory controller index
thing doesn't work. But why doesn't it work?

From what have you got this? I said that the current MC indexing
approach wasn't that optimal (always relying on the low-level driver
to allocate the index) because it caused having the same IDx
allocation pattern re-implemented in the drivers. It can be avoided by
the provided patch. The unified approach makes code indeed more
readable in the platform drivers and safer since they didn't have to
bother with more coding. See for instance the drivers with the
static variable-based IDs allocation. It doesn't seem like these
drivers bother with the detected DDR devices order. If so then the
automatic IDs allocation is perfect for them. Note the static variable
increment isn't atomic. Thus the ID allocation algorithm there is prone
to races should the devices probe is run concurrently.

It works just fine with the x86 drivers - there the memory controller
number is the same as the node number where it is located so that works
just fine.

If that scheme cannot work on other systems, then I need to see an
explanation why it cannot work first.

quoted

The suggested implementation is based on the kernel IDA infrastructure
exposed by the lib/idr.c driver with API described in linux/idr.h header
file. It's used to create an ID resource descriptor "mc_idr", which then
is utilized either to track the custom MC idx specified by EDAC LLDDs or
to allocate the next-free MC idx.

This is talking about the "what" and not the "why".

quoted

A new special MC index is introduced here. It's defined by the
EDAC_AUTO_MC_NUM macro with a value specifically chosen as the least
probable value used for the real MC index. In case if the EDAC_AUTO_MC_NUM
index is specified by the EDAC LLDD, the MC index will be either retrieved
from the MC device OF-node alias index ("mc[:number:]") or automatically
generated as the next-free MC index found by the ID allocation procedure.

This is also talking about the "what" and not the "why".

At best, what you're doing should be visible from the patch itself.

Here's a longer explanation of how a commit message should look like:

https://kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Have you read it yourself? Here is a short excerpt from there:
"Once the problem is established, describe what you are actually doing
about it in technical detail.  It's important to describe the change
in plain English for the reviewer to verify that the code is behaving
as you intend it to."

So the "problem" is described in the first paragraph and the technical
details in the later paragraphs.

-Sergey

Thx.

-- 
Regards/Gruss,
    Boris.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel