Currently, when a child platform device (sometimes referred to as a
sub-device) is registered via the Multi-Functional Device (MFD) API,
the framework attempts to match the newly registered platform device
with its associated Device Tree (OF) node. Until now, the device has
been allocated the first node found with an identical OF compatible
string. Unfortunately, if there are, say for example '3' devices
which are to be handled by the same driver and therefore have the same
compatible string, each of them will be allocated a pointer to the
*first* node.
An example Device Tree entry might look like this:
mfd_of_test {
compatible = "mfd,of-test-parent";
#address-cells = <0x02>;
#size-cells = <0x02>;
child@aaaaaaaaaaaaaaaa {
compatible = "mfd,of-test-child";
reg = <0xaaaaaaaa 0xaaaaaaaa 0 0x11>,
<0xbbbbbbbb 0xbbbbbbbb 0 0x22>;
};
child@cccccccc {
compatible = "mfd,of-test-child";
reg = <0x00000000 0xcccccccc 0 0x33>;
};
child@dddddddd00000000 {
compatible = "mfd,of-test-child";
reg = <0xdddddddd 0x00000000 0 0x44>;
};
};
When used with example sub-device registration like this:
static const struct mfd_cell mfd_of_test_cell[] = {
OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 0, "mfd,of-test-child"),
OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 1, "mfd,of-test-child"),
OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 2, "mfd,of-test-child")
};
... the current implementation will result in all devices being allocated
the first OF node found containing a matching compatible string:
[0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform device: 0
[0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node: child@aaaaaaaaaaaaaaaa
[0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
[0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node: child@aaaaaaaaaaaaaaaa
[0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
[0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node: child@aaaaaaaaaaaaaaaa
After this patch each device will be allocated a unique OF node:
[0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform device: 0
[0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node: child@aaaaaaaaaaaaaaaa
[0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
[0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node: child@cccccccc
[0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
[0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node: child@dddddddd00000000
Which is fine if all OF nodes are identical. However if we wish to
apply an attribute to particular device, we really need to ensure the
correct OF node will be associated with the device containing the
correct address. We accomplish this by matching the device's address
expressed in DT with one provided during sub-device registration.
Like this:
static const struct mfd_cell mfd_of_test_cell[] = {
OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 1, "mfd,of-test-child", 0xdddddddd00000000),
OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2, "mfd,of-test-child", 0xaaaaaaaaaaaaaaaa),
OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3, "mfd,of-test-child", 0x00000000cccccccc)
};
This will ensure a specific device (designated here using the
platform_ids; 1, 2 and 3) is matched with a particular OF node:
[0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform device: 0
[0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node: child@dddddddd00000000
[0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
[0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node: child@aaaaaaaaaaaaaaaa
[0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
[0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node: child@cccccccc
This implementation is still not infallible, hence the mention of
"best effort" in the commit subject. Since we have not *insisted* on
the existence of 'reg' properties (in some scenarios they just do not
make sense) and no device currently uses the new 'of_reg' attribute,
we have to make an on-the-fly judgement call whether to associate the
OF node anyway. Which we do in cases where parent drivers haven't
specified a particular OF node to match to. So there is a *slight*
possibility of the following result (note: the implementation here is
convoluted, but it shows you one means by which this process can
still break):
/*
* First entry will match to the first OF node with matching compatible
* Second will fail, since the first took its OF node and is no longer available
* Third will succeed
*/
static const struct mfd_cell mfd_of_test_cell[] = {
OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 1, "mfd,of-test-child"),
OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2, "mfd,of-test-child", 0xaaaaaaaaaaaaaaaa),
OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3, "mfd,of-test-child", 0x00000000cccccccc)
};
The result:
[0.753869] mfd-of-test-parent mfd_of_test: Registering 3 devices
[0.756597] mfd-of-test-child: Failed to locate of_node [id: 2]
[0.759999] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
[0.760314] mfd-of-test-child mfd-of-test-child.1: Using OF node: child@aaaaaaaaaaaaaaaa
[0.760908] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
[0.761183] mfd-of-test-child mfd-of-test-child.2: No OF node associated with this device
[0.761621] mfd-of-test-child mfd-of-test-child.3: Probing platform device: 3
[0.761899] mfd-of-test-child mfd-of-test-child.3: Using OF node: child@cccccccc
We could code around this with some pre-parsing semantics, but the
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. Other issues
should be patched in the parent drivers which can be achieved simply
by implementing OF_MFD_CELL_REG().
Signed-off-by: Lee Jones <[email protected]>
---
Changelog:
v1 => v2:
* Simply return -EAGAIN if node is already in use
* Allow for valid of_reg=0 by introducing use_of_reg boolean flag
* Split helpers out into separate patch
drivers/mfd/mfd-core.c | 99 +++++++++++++++++++++++++++++++++++-----
include/linux/mfd/core.h | 10 ++++
2 files changed, 97 insertions(+), 12 deletions(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index e831e733b38cf..120803717b828 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -10,6 +10,7 @@
#include <linux/kernel.h>
#include <linux/platform_device.h>
#include <linux/acpi.h>
+#include <linux/list.h>
#include <linux/property.h>
#include <linux/mfd/core.h>
#include <linux/pm_runtime.h>
@@ -17,8 +18,17 @@
#include <linux/module.h>
#include <linux/irqdomain.h>
#include <linux/of.h>
+#include <linux/of_address.h>
#include <linux/regulator/consumer.h>
+static LIST_HEAD(mfd_of_node_list);
+
+struct mfd_of_node_entry {
+ struct list_head list;
+ struct device *dev;
+ struct device_node *np;
+};
+
static struct device_type mfd_dev_type = {
.name = "mfd_device",
};
@@ -107,6 +117,54 @@ static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
}
#endif
+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)
+ if (of_entry->np == np)
+ return -EAGAIN;
+
+ if (!cell->use_of_reg)
+ /* No of_reg defined - allocate first free compatible match */
+ goto allocate_of_node;
+
+ /* We only care about each node's first defined address */
+ reg = of_get_address(np, 0, NULL, NULL);
+ if (!reg)
+ /* OF node does not contatin a 'reg' property to match to */
+ return -EAGAIN;
+
+ of_node_addr = of_read_number(reg, of_n_addr_cells(np));
+
+ if (cell->of_reg != of_node_addr)
+ /* No match */
+ return -EAGAIN;
+
+allocate_of_node:
+ of_entry = kzalloc(sizeof(*of_entry), GFP_KERNEL);
+ if (!of_entry)
+ return -ENOMEM;
+
+ of_entry->dev = &pdev->dev;
+ of_entry->np = np;
+ list_add_tail(&of_entry->list, &mfd_of_node_list);
+
+ pdev->dev.of_node = np;
+ pdev->dev.fwnode = &np->fwnode;
+
+ return 0;
+}
+
static int mfd_add_device(struct device *parent, int id,
const struct mfd_cell *cell,
struct resource *mem_base,
@@ -115,6 +173,7 @@ static int mfd_add_device(struct device *parent, int id,
struct resource *res;
struct platform_device *pdev;
struct device_node *np = NULL;
+ struct mfd_of_node_entry *of_entry, *tmp;
int ret = -ENOMEM;
int platform_id;
int r;
@@ -149,19 +208,22 @@ static int mfd_add_device(struct device *parent, int id,
if (ret < 0)
goto fail_res;
- if (parent->of_node && cell->of_compatible) {
+ if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell->of_compatible) {
for_each_child_of_node(parent->of_node, np) {
if (of_device_is_compatible(np, cell->of_compatible)) {
- if (!of_device_is_available(np)) {
- /* Ignore disabled devices error free */
- ret = 0;
+ ret = mfd_match_of_node_to_dev(pdev, np, cell);
+ if (ret == -EAGAIN)
+ continue;
+ if (ret)
goto fail_alias;
- }
- pdev->dev.of_node = np;
- pdev->dev.fwnode = &np->fwnode;
+
break;
}
}
+
+ if (!pdev->dev.of_node)
+ pr_warn("%s: Failed to locate of_node [id: %d]\n",
+ cell->name, platform_id);
}
mfd_acpi_add_device(cell, pdev);
@@ -170,13 +232,13 @@ static int mfd_add_device(struct device *parent, int id,
ret = platform_device_add_data(pdev,
cell->platform_data, cell->pdata_size);
if (ret)
- goto fail_alias;
+ goto fail_of_entry;
}
if (cell->properties) {
ret = platform_device_add_properties(pdev, cell->properties);
if (ret)
- goto fail_alias;
+ goto fail_of_entry;
}
for (r = 0; r < cell->num_resources; r++) {
@@ -213,18 +275,18 @@ static int mfd_add_device(struct device *parent, int id,
if (has_acpi_companion(&pdev->dev)) {
ret = acpi_check_resource_conflict(&res[r]);
if (ret)
- goto fail_alias;
+ goto fail_of_entry;
}
}
}
ret = platform_device_add_resources(pdev, res, cell->num_resources);
if (ret)
- goto fail_alias;
+ goto fail_of_entry;
ret = platform_device_add(pdev);
if (ret)
- goto fail_alias;
+ goto fail_of_entry;
if (cell->pm_runtime_no_callbacks)
pm_runtime_no_callbacks(&pdev->dev);
@@ -233,6 +295,12 @@ static int mfd_add_device(struct device *parent, int id,
return 0;
+fail_of_entry:
+ list_for_each_entry_safe(of_entry, tmp, &mfd_of_node_list, list)
+ if (of_entry->dev == &pdev->dev) {
+ list_del(&of_entry->list);
+ kfree(of_entry);
+ }
fail_alias:
regulator_bulk_unregister_supply_alias(&pdev->dev,
cell->parent_supplies,
@@ -287,6 +355,7 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
{
struct platform_device *pdev;
const struct mfd_cell *cell;
+ struct mfd_of_node_entry *of_entry, *tmp;
if (dev->type != &mfd_dev_type)
return 0;
@@ -297,6 +366,12 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
cell->num_parent_supplies);
+ list_for_each_entry_safe(of_entry, tmp, &mfd_of_node_list, list)
+ if (of_entry->dev == dev) {
+ list_del(&of_entry->list);
+ kfree(of_entry);
+ }
+
kfree(cell);
platform_device_unregister(pdev);
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index d01d1299e49dc..a148b907bb7f1 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -78,6 +78,16 @@ struct mfd_cell {
*/
const char *of_compatible;
+ /*
+ * Address as defined in Device Tree. Used to compement 'of_compatible'
+ * (above) when matching OF nodes with devices that have identical
+ * compatible strings
+ */
+ const u64 of_reg;
+
+ /* Set to 'true' to use 'of_reg' (above) - allows for of_reg=0 */
+ bool use_of_reg;
+
/* Matches ACPI */
const struct mfd_cell_acpi_match *acpi_match;
--
2.25.1
On 2020-06-22 20:17, Frank Rowand wrote:
> On 2020-06-22 17:23, 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 "".
>>>>>>
>>>>>> 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
>
> I haven't had time to search for an excellent resource yet, but following
> the above URL, the is a Linaro Connect slide set from Grant Likely that
> provides a somewhat high level conceptual view. You can skim through
> the slides pretty fast:
>
> https://elinux.org/images/6/67/Hkg18-120-devicetreeschema-grantlikely-180404144834.pdf
A better presentation for this purpose is Rob's:
https://elinux.org/images/6/6b/LPC2018_json-schema_for_Devicetree.pdf
-Frank
>
> A very high level overview is that the bindings documents in the Linux
> kernel source tree at Documentation/devicetree/bindings/ are being
> converted to a YAML format that can be processed by the verification
> tools. The verification tools can use the bindings to check whether
> a devicetree source follows the definition of the bindings for each
> of the nodes.
>
> A random example of a binding that has been converted to YAML is
>
> Documentation/devicetree/bindings/rng/st,stm32-rng.yaml
>
> In the specific case that this patch series addresses, the
> Documentation/devicetree/bindings/mfd/ab8500.txt binding has not been
> converted to YAML yet. If it was YAML, it should specify the properties
> for a child node with compatible value "stericsson,ab8500-pwm" is not
> allowed to have a "reg" property. A new compatible should be
> added for child nodes that are required to have a "reg" property.
> In my suggestion above I chose "ab8500-pwm-mc" for this new compatible.
> That is probably a terrible name, Rob would probably have a better
> suggestion.
>
> Given such a YAML binding, the verification tool would report an error
> for any child node with compatible "stericsson,ab8500-pwm" and containing
> a "reg" property. It would also report an error for any child node
> with compatible "ab8500-pwm-mc" that was missing the required "reg"
> property.
>
> -Frank
>
>>
>> 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.
>>
>>>
>>>> 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?
>
> You can modify more extensively than my simple example, changing
> mfd_add_device() to more gracefully handle an EINVAL returned by
> mfd_match_of_node_to_dev().
>
>
>>>
>>>> 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.
>>
>>> * 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.
>>
>>> * 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.
>>
>>>
>>> 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.
>>
>> OK, so my solution #3 is a no go. How about my solution #2,
>> which you did not comment on?
>>
>> -Frank
>> .
>>
>
> .
>