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

[Frank Rowand]

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.


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.


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



> 
> The best we can do is "best effort", to try and match each cell with
> its requested OF node.
>