lkml.org 
[lkml]   [2020]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
    From
    Date
    On 2020-06-23 01:47, Lee Jones wrote:
    > On Mon, 22 Jun 2020, Frank Rowand wrote:
    >
    >> On 2020-06-22 14:11, Lee Jones wrote:
    >>> On Mon, 22 Jun 2020, Frank Rowand wrote:
    >>>
    >>>> On 2020-06-22 10:10, Lee Jones wrote:
    >>>>> On Mon, 22 Jun 2020, Frank Rowand wrote:
    >>>>>
    >>>>>> On 2020-06-22 03:50, Lee Jones wrote:
    >>>>>>> On Thu, 18 Jun 2020, Frank Rowand wrote:
    >>>>>>>
    >>>>>>>> On 2020-06-15 04:26, Lee Jones wrote:
    >>>>>>>>> On Sun, 14 Jun 2020, Frank Rowand wrote:
    >>>>>>>>>
    >>>>>>>>>> Hi Lee,
    >>>>>>>>>>
    >>>>>>>>>> I'm looking at 5.8-rc1.
    >>>>>>>>>>
    >>>>>>>>>> The only use of OF_MFD_CELL() where the same compatible is specified
    >>>>>>>>>> for multiple elements of a struct mfd_cell array is for compatible
    >>>>>>>>>> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
    >>>>>>>>>>
    >>>>>>>>>> OF_MFD_CELL("ab8500-pwm",
    >>>>>>>>>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
    >>>>>>>>>> OF_MFD_CELL("ab8500-pwm",
    >>>>>>>>>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
    >>>>>>>>>> OF_MFD_CELL("ab8500-pwm",
    >>>>>>>>>> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
    >>>>>>>>
    >>>>>>>> OF_MFD_CELL("ab8500-pwm",
    >>>>>>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
    >>>>>>>>
    >>>>>>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
    >>>>>>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
    >>>>>>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
    >>>>>>>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
    >>>>>>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
    >>>>>>>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
    >>>>>>>>
    >>>>>>>>>>
    >>>>>>>>>> The only .dts or .dtsi files where I see compatible "stericsson,ab8500-pwm"
    >>>>>>>>>> are:
    >>>>>>>>>>
    >>>>>>>>>> arch/arm/boot/dts/ste-ab8500.dtsi
    >>>>>>>>>> arch/arm/boot/dts/ste-ab8505.dtsi
    >>>>>>>>>>
    >>>>>>>>>> These two .dtsi files only have a single node with this compatible.
    >>>>>>>>>> Chasing back to .dts and .dtsi files that include these two .dtsi
    >>>>>>>>>> files, I see no case where there are multiple nodes with this
    >>>>>>>>>> compatible.
    >>>>>>>>>>
    >>>>>>>>>> So it looks to me like there is no .dts in mainline that is providing
    >>>>>>>>>> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
    >>>>>>>>>> is expecting. No case that there are multiple mfd child nodes where
    >>>>>>>>>> mfd_add_device() would assign the first of n child nodes with the
    >>>>>>>>>> same compatible to multiple devices.
    >>>>>>>>>>
    >>>>>>>>>> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
    >>>>>>>>>> Am I missing something here?
    >>>>>>>>>>
    >>>>>>>>>> If I am correct, then either drivers/mfd/ab8500-core.c or
    >>>>>>>>>> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
    >>>>>>>>>
    >>>>>>>>> Your analysis is correct.
    >>>>>>>>
    >>>>>>>> OK, if I'm not overlooking anything, that is good news.
    >>>>>>>>
    >>>>>>>> Existing .dts source files only have one "ab8500-pwm" child. They already
    >>>>>>>> work correcly.
    >>>>>>>>
    >>>>>>>> Create a new compatible for the case of multiple children. In my example
    >>>>>>>> I will add "-mc" (multiple children) to the existing compatible. There
    >>>>>>>> is likely a better name, but this lets me provide an example.
    >>>>>>>>
    >>>>>>>> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
    >>>>>>>> source files with multiple children use the new compatible:
    >>>>>>>>
    >>>>>>>> OF_MFD_CELL("ab8500-pwm",
    >>>>>>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
    >>>>>>>>
    >>>>>>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
    >>>>>>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
    >>>>>>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
    >>>>>>>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
    >>>>>>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
    >>>>>>>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
    >>>>>>>>
    >>>>>>>> The "OF_MFD_CELL" entry is the existing entry, which will handle current
    >>>>>>>> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
    >>>>>>>> .dts source files.
    >>>>>>>
    >>>>>>> Sorry, but I'm not sure what the above exercise is supposed to solve.
    >>>>>>>
    >>>>>>> Could you explain it for me please?
    >>>>>>
    >>>>>> The OF_MFD_CELL() entry handles all of the existing .dts source files
    >>>>>> that only have one ab8500-pwm child nodes. So existing .dtb blobs
    >>>>>> continue to work.
    >>>>>>
    >>>>>> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
    >>>>>> files that will have up to 3 ab8500-pwm child nodes.
    >>>>>>
    >>>>>> Compatibility is maintained for existing .dtb files. A new kernel
    >>>>>> version with the changes will support new .dtb files that contain
    >>>>>> multiple ab8500-pwm child nodes.
    >>>>>
    >>>>> I can see *what* you're trying to do. I was looking for an
    >>>>> explanation of *how* you think that will work. FWIW, I don't think
    >>>>> what you're proposing will work as you envisage. I thought that
    >>>>> perhaps I was missing something, which is why I requested further
    >>>>> explanation.
    >>>>>
    >>>>>>>> And of course the patch that creates OF_MFD_CELL_REG() needs to precede
    >>>>>>>> this change.
    >>>>>>>>
    >>>>>>>> I would remove the fallback code in the existing patch that tries to
    >>>>>>>> handle an incorrect binding. Just error out if the binding is not
    >>>>>>>> used properly.
    >>>>>>>
    >>>>>>> What fallback code?
    >>>>>>
    >>>>>> Based on reading the patch description, I expected some extra code to try
    >>>>>> to handle the case where the compatible in more than one struct mfd_cell
    >>>>>> entry is "stericsson,ab8500-pwm" and there are multiple ab8500-pwm child
    >>>>>> nodes.
    >>>>>>
    >>>>>> Looking at the actual code (which I had not done before), I see that the
    >>>>>> "best effort attempt to match" is keeping a list of child nodes that
    >>>>>> have already been used (mfd_of_node_list) and avoiding re-use of such
    >>>>>> nodes. This allows an invalid .dtb (one with multple "stericsson,ab8500-pwm"
    >>>>>> child nodes) to possibly be assigned unique child nodes for multiple
    >>>>>> struct mfd_cell entries to be "stericsson,ab8500-pwm".
    >>>>>>
    >>>>>> So it is confusing for me to call that "fallback code". It really is
    >>>>>> "best effort attempt to match" for a broken .dtb code.
    >>>>>>
    >>>>>> There should be no best effort for a broken .dtb. The broken .dtb should
    >>>>>> instead result in an error.
    >>>>>
    >>>>> The problem is, how can you tell the difference between a valid and a
    >>>>> broken FDT without pre-processing - which, as I explained in the
    >>>>> commit message, I am not prepared to do. We cannot test individually
    >>>>> since all configurations (e.g. no 'reg' property are valid on an
    >>>>> individual basis.
    >>>>
    >>>> If my proposed changes are made, then there are at least 3 ways to detect
    >>>> a broken FDT or prevent the problem caused by the broken FDT.
    >>>>
    >>>>
    >>>> 1) Use the validation process that uses the bindings to validate the
    >>>> devicetree source.
    >>>
    >>> Could you provide an example please?
    >>
    >> I'm sorry I don't have a concise description and example. I have not been
    >> keeping up with the state of the art in this area.
    >>
    >> A terribly non-precise pointer to the general area is:
    >>
    >> https://elinux.org/Device_tree_future#Devicetree_Verification
    >>
    >> As a general comment, I think that validation / verification is a very
    >> valuable tool, but the schemas to support it are an ongoing project.
    >>
    >> Even after the schemas are all in place, it will still be possible for
    >> bad FDTs to be fed to the kernel, so it is not a total pancea.
    >
    > Ah, you meant schema. Yes, I know what that is, I just didn't
    > connect the two from the description above.
    >
    >>>> 2) Modify patch 1/3. The small part of the patch to modify is:
    >>>>
    >>>> +static int mfd_match_of_node_to_dev(struct platform_device *pdev,
    >>>> + struct device_node *np,
    >>>> + const struct mfd_cell *cell)
    >>>> +{
    >>>> + struct mfd_of_node_entry *of_entry;
    >>>> + const __be32 *reg;
    >>>> + u64 of_node_addr;
    >>>> +
    >>>> + /* Skip devices 'disabled' by Device Tree */
    >>>> + if (!of_device_is_available(np))
    >>>> + return -ENODEV;
    >>>> +
    >>>> + /* Skip if OF node has previously been allocated to a device */
    >>>> + list_for_each_entry(of_entry, &mfd_of_node_list, list)
    >>>>
    >>>> Change:
    >>>>
    >>>> + if (of_entry->np == np)
    >>>> + return -EAGAIN;
    >>>>
    >>>> To:
    >>>>
    >>>> + if (of_entry->np == np) {
    >>>> + if (!cell->use_of_reg)
    >>>> + return -EINVAL;
    >>>> + else
    >>>> + return -EAGAIN;
    >>>>
    >>>> There may be a better choice than EINVAL, but I am just showing the method.
    >>>>
    >>>> You may also want to refactor this section of the patch slightly
    >>>> differently to achieve the same result. It was just easiest to
    >>>> show the suggested change the way I did it.
    >>>>
    >>>> The test that returns EINVAL detects the issue that the FDT does
    >>>> not match the binding (there is more one child node with the
    >>>> "stericsson,ab8500-pwm" compatible.
    >>>
    >>> So here, instead of just failing a single device, we fail everything?
    >>> Sounds a lot like throwing the baby out with the bath water. How is
    >>> that an improvement?
    >
    > [0]

    Is "[0]" the patch series, especially patch 1/3?

    >
    >>>> 3) I'm not sure if the pre-parsing that is wanted is parsing of the
    >>>> devicetree or parsing of the struct mfd_cell array. If the mfd_cell
    >>>> array then solution 3 is not acceptable.
    >>>>
    >>>> A different change to a small part of patch 1/3. In mfd_add_devices(),
    >>>> validate parameter "cells". The validation could precede the existing
    >>>> code, or it could be folded into the existing for loop. The validation
    >>>> is checking for any other element of the cells array containing
    >>>> the same compatible value if cell->use_of_reg is not true for an element.
    >>>>
    >>>> If this validation occurs, then I think mfd_of_node_list, and all the
    >>>> associated code to deal with it is no longer needed. But I didn't
    >>>> look at this part in detail, so maybe I missed something.
    >>>>
    >>>> The validation is something like (untested):
    >>>>
    >>>> if (IS_ENABLED(CONFIG_OF)
    >>>> for (i = 0; i < n_devs; i++) {
    >>>> this_cell = cells + i;
    >>>> if (!this_cell->use_of_reg) {
    >>>> for (j = 1; j < n_devs; j++) {
    >>>> if (j != i) {
    >>>> cell = cells + j;
    >>>> if (!strcmp(this_cell->of_compatible, cell->of_compatible))
    >>>> return -EINVAL;
    >>>> }
    >>>> }
    >>>> }
    >>>> }
    >>>
    >>> I think I just threw-up a little. ;)
    >>
    >> I'm not surprised.
    >>
    >> But it actually is pretty simple code.
    >>
    >>>
    >>> Did you read the commit message?
    >>
    >> Yes, I did.
    >>
    >>>
    >>> "We could code around this with some pre-parsing semantics, but the
    >>
    >> And as I said above, it was not clear to me what was meant by pre-parsing.
    >>
    >>> added complexity required to cover each and every corner-case is not
    >>> justified. Merely patching the current failing (via this patch) is
    >>> already working with some pretty small corner-cases"
    >>>
    >>> Providing thorough pre-parsing would be highly complex and highly
    >>> error prone. The example you provide above is not only ugly, there
    >>> are numerous issues with it. Not least:
    >>>
    >>> * Only one corner-case is covered
    >>
    >> I agree with this. I also agree it is a fool's errand to try to add
    >> code to fully validate all possible devicetree source errors in
    >> driver source.
    >
    > Great. Phew!
    >
    >>> * Validation is only completed on a single mfd_cells struct
    >>
    >> On a single _array_ of struct mfd_cells. But this does appear
    >> to be a fatal flaw. I had not looked at enough callers of
    >> mfd_add_devices() to see that it is a common pattern for
    >> a single driver to call it multiple times.
    >
    > Exactly.
    >
    >>> * False positives can occur and will fail as a result
    >>
    >> ((What is an example of a false positive?)) Never mind, now that
    >> I see that the previous issue is a fatal flaw, this becomes
    >> academic.
    >
    > That's okay, I don't mind discussing.
    >
    > Ironically, the 'ab8500-pwm' is a good example of a false positive,
    > since it's fine for the DT nodes to be identical. So long as there
    > are nodes present for each instance, it doesn't matter which one is
    > allocated to which device .Forcing a 'reg' property onto them for no> good reason it not a valid solution here.

    I thought that one of the points of this patch series was to add a
    "reg" property to any mfd child that was described by the
    OF_MFD_CELL_REG() macro.

    And that was meant to fix the problem where multiple indistinguishable
    children existed. The only instance I found of that (using the
    weak search on OF_MFD_CELL()) was of compatible "stericsson,ab8500-pwm"
    in drivers/mfd/ab8500-core.c. You agreed with my email that
    reported that.

    So I thought that drivers/mfd/ab8500-core.c would be modified to
    replace the multiple instances of compatible "stericsson,ab8500-pwm"
    in OF_MFD_CELL() with OF_MFD_CELL_REG().

    This is another problem with the patch series: there is no user
    of OF_MFD_CELL_REG(). Please add one to the series.

    >
    >>> The above actually makes the solution worse, not better.
    >>>
    >>
    >> Patch 1/3 silently fails to deal with a broken devicetree.
    >> It results on one of the three ab8500-pwm child nodes in
    >> the hypothetical devicetree source tree not being added.
    >>
    >> That is not a good result either.
    >
    > No it doesn't. In the case of 'ab8500-pwm' the OF node is not set for
    > 2 of the devices and warnings are presented in the kernel log.

    OK, I was wrong about "silent". There is a warning:
    pr_warn("%s: Failed to locate of_node [id: %d]\n",

    > The
    > device will continue to probe and function as usual.

    If the device probes and functions as usual without the child of_node,
    then why does the node have any properties (for the cases of
    arch/arm/boot/dts/ste-ab8500.dtsi and arch/arm/boot/dts/ste-ab8505.dtsi
    the properties "clocks" and "clock-names").

    Digging through that leads to yet another related question, or actually
    sort of the same question. Why do the child nodes with compatible
    "stericsson,ab8500-pwm" have the properties "clocks" and "clock-names"
    since the binding Documentation/devicetree/bindings/mfd/ab8500.txt
    does not list them?

    >
    >> OK, so my solution #3 is a no go. How about my solution #2,
    >> which you did not comment on?
    >
    > I did [0]. You must have missed it. :)

    But yes or no to my solution #2 (with some slight changes to
    make it better (more gracious handling of the detected error) as
    discussed elsewhere in the email thread)?

    >
    > It also suffers with false positives.
    >

    \
     
     \ /
      Last update: 2020-06-23 19:57    [W:3.216 / U:0.320 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site