Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes

[Lee Jones <lee.jones@linaro.org>]

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]

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

> > 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.  The
device will continue to probe and function as usual.

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

It also suffers with false positives.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog