| Message ID | 20200611191002.2256570-1-lee.jones@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes | expand |
+ Frank (me) On 2020-06-11 14:10, Lee Jones wrote: > 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 <lee.jones@linaro.org> > --- > > 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; > >
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"),
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.
Moving forward, your proposed OF_MFD_CELL_REG() method seems a good
approach (I have not completely read the actual code in the patch yet
though).
On 2020-06-11 14:10, Lee Jones wrote:
> 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
> */
The following mfd_of_test_cell[] looks like a bug to me. For a given
compatible value, either reg is required or is not allowed.
One could add validation code to mfd_add_devices(), which would scan the
parameter "cells", checking for multiple array elements with the same
compatible value where one of the elements does not have .use_of_reg
set. This seems to be a pain to program and not needed, because the
validation process, using the binding definition, would detect the
problem in the .dts source file, because either the required reg
property would be missing or the not to be specified reg property exists.
Whether reg is required or not allowed is based on the compatible value.
> 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
So this should problem should not occur.
> justified. Merely patching the current failing (via this patch) is
> already working with some pretty small corner-cases. Other issues
If there are existing corner cases, I did not search the source tree
sufficiently. What are the current corner cases?
-Frank
> should be patched in the parent drivers which can be achieved simply
> by implementing OF_MFD_CELL_REG().
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>
> 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;
>
>
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"), > > 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. Although it's not "broken", it just works when it really shouldn't. I will be fixing the 'ab8500-pwm' case in due course. > Moving forward, your proposed OF_MFD_CELL_REG() method seems a good > approach (I have not completely read the actual code in the patch yet > though). Thanks. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
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. 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. -Frank > > Although it's not "broken", it just works when it really shouldn't. > > I will be fixing the 'ab8500-pwm' case in due course. > >> Moving forward, your proposed OF_MFD_CELL_REG() method seems a good >> approach (I have not completely read the actual code in the patch yet >> though). > > Thanks. >
On Thu, 11 Jun 2020, Lee Jones wrote: > 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. Any more reviews/comments before I apply this? -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
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? > 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? > > Although it's not "broken", it just works when it really shouldn't. > > > > I will be fixing the 'ab8500-pwm' case in due course. > > > >> Moving forward, your proposed OF_MFD_CELL_REG() method seems a good > >> approach (I have not completely read the actual code in the patch yet > >> though). > > > > Thanks. > > > -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
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. > >> 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. -Frank > >>> Although it's not "broken", it just works when it really shouldn't. >>> >>> I will be fixing the 'ab8500-pwm' case in due course. >>> >>>> Moving forward, your proposed OF_MFD_CELL_REG() method seems a good >>>> approach (I have not completely read the actual code in the patch yet >>>> though). >>> >>> Thanks. >>> >> >
On 2020-06-22 09:32, 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. > >> >>> 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". struct mfd_cell entries that each have the same compatible value "stericsson,ab8500-pwm". Some day I'll learn how to speak my native language. :-) -Frank > > 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. > > -Frank > >> >>>> Although it's not "broken", it just works when it really shouldn't. >>>> >>>> I will be fixing the 'ab8500-pwm' case in due course. >>>> >>>>> Moving forward, your proposed OF_MFD_CELL_REG() method seems a good >>>>> approach (I have not completely read the actual code in the patch yet >>>>> though). >>>> >>>> Thanks. >>>> >>> >> >
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. The best we can do is "best effort", to try and match each cell with its requested OF node. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
On 2020-06-22 03:09, Lee Jones wrote: > On Thu, 11 Jun 2020, Lee Jones wrote: > >> 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. > > Any more reviews/comments before I apply this? > Yes, outstanding issues, so please do not apply. Shortly after you sent this email, you sent a reply to one of my earlier emails in this thread. I have replied to that email, so we still have an ongoing conversation where we are trying to resolve my understanding of the problem and whether the solution is appropriate. -Frank
On Mon, 22 Jun 2020, Frank Rowand wrote: > On 2020-06-22 03:09, Lee Jones wrote: > > On Thu, 11 Jun 2020, Lee Jones wrote: > > > >> 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. > > > > Any more reviews/comments before I apply this? > > > > Yes, outstanding issues, so please do not apply. > > Shortly after you sent this email, you sent a reply to one of my > earlier emails in this thread. I have replied to that email, > so we still have an ongoing conversation where we are trying > to resolve my understanding of the problem and whether the > solution is appropriate. Happy to discuss whatever issues you're having. Looks like the ball is in your court. :) -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
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. >
On 2020-06-22 13:01, 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. > > > 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 ^^^^^^^^^^^^^^ that is not wanted > 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. >> >
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? > 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? > 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. ;) Did you read the commit message? "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" 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 * Validation is only completed on a single mfd_cells struct * False positives can occur and will fail as a result The above actually makes the solution worse, not better. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
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. > >> 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? > >> 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
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 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 > . >
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 >> . >> > > . >
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
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. >
Suggestion #2 > >>>> 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. My reply to suggestion #2 > >>> 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? No, this is my reply to your suggestion #2. The [0] is referenced further down. [...] > >>> * 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. The OF_MFD_CELL_REG() macro didn't exist until this patch-set. There are currently no users. > 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. No, I agreed with you that there is a current problem with "stericsson,ab8500-pwm", as identified by Michael. I didn't actually know about this issue until *after* drafting this patch-set. To be clear the "stericsson,ab8500-pwm" scenario is not the reason for this set's existence. Also, please forget about the OF_MFD_* macros, they are totally agnostic to this effort. The only relevance they have here is the addition of 1 extra macro which *could* be used to provide the 'reg' property where appropriate. > 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(). That is not my vision. There is no need for "stericsson,ab8500-pwm" to have 'reg' properties as far as I see it. > This is another problem with the patch series: there is no user > of OF_MFD_CELL_REG(). Please add one to the series. That's not a problem with this patch-set, it's a problem with your understanding of this patch-set. :) As far as I know, there aren't any current users who would benefit from this work. Instead, it is designed to provide future submitters with another tool to help them link their child devices to the correct OF nodes. That's not to say that current users can't and won't benefit from this. Just that they are not the target audience. > >>> 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"). Because DT is meant to describe the hardware, not the implementation. DT does not know, or care that in our case most operations that happen on the platform are passed back via an API to a central controlling location. Or that in reality, the OF node in this situation is superfluous. Can we please stop talking about the AB8500. It doesn't have anything to do with this series besides the fact that if it (this set) had existed *before* 'ab8500-pwm' was OF enabled, it wouldn't now be wonky. > 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? If you want to talk about the AB8500, please start a new thread. > >> 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)? Please see "[0]" above! AFAICT your solution #2 involves bombing out *all* devices if there is a duplicate compatible with no 'reg' property value. This is a) over-kill and b) not an error, as I mentioned: > > 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
On 2020-06-11 14:10, Lee Jones wrote: > 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 <lee.jones@linaro.org> > --- > > 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 > + */ Instead of the above comment, suggest something like instead (I have not properly line wrapped, to make it easier to see the difference): > + /* > + * Address as defined in Device Tree mfd child node "reg" property. Used in combination with '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; > >
On 2020-06-23 14:59, Lee Jones wrote: > Suggestion #2 > >>>>>> 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. > > My reply to suggestion #2 > >>>>> 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? I could have sworn that I had replied with a solution to this issue. So I searched and searched and searched my emails in the thread. And checked my drafts email folder. Then finally realized I had made a stupid mistake. I did reply about this, but I had put my "-Frank" signature at the end of a comment much higher in the email. So of course I expect you stopped reading at that point and never saw my answer. My apologies!!! The email in question is: https://lore.kernel.org/linux-devicetree/eae9cc00-e67a-cb6a-37c2-f2235782ed77@gmail.com/ and what I wrote at this point in that email is: 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(). Thus a modification to my suggestion #2 to make it _not_ fail everything. I didn't really flesh out all that "more gracefully handle" means. Among other things, it could include a pr_warn() that provides a fairly specific possible cause of the problem (eg the corner case mentioned near the end of the patch 1/3 header that shows mixing OF_MFD_CELL() and OF_MFD_CELL_REG() for the same compatible value. It may be tricky coming up with good phrasing in a pr_warn() that describes the generic issue instead of the specific example. Again, sorry. >>> >>> [0] >> >> Is "[0]" the patch series, especially patch 1/3? > > No, this is my reply to your suggestion #2. > > The [0] is referenced further down. > > [...] > >>>>> * 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. >> Start of my comment that I wrote with "too many shortcuts" (see below): >> 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. > > The OF_MFD_CELL_REG() macro didn't exist until this patch-set. Maybe the way I wrote that took too many shortcuts. Let me re-phrase. I thought that one of the points of this patch set was to add the of_reg and use_of_reg fields to struct mfd_cell. The expected use of the of_reg and use_of_reg fields was to allow the presence of a "reg" property in a devicetree mfd child node to be used to match specific child nodes to specific elements of the mfd_add_devices() cell array parameter, with the match occurring when the array elements are processed (currently in mfd_match_of_node_to_dev(), which is called by mfd_add_device()). The key point being the matching specific devicetree mfd child nodes to specific cell array members. The OF_MFD_CELL_REG() is simply a helper macro related to the above. > > There are currently no users. Yes. And as I pointed out elsewhere, I would expect a user of new functionality to be added as part of a patch series that adds the new functionality. Or at least a mention of a specific plan to use the functionality. I had been assuming that the intended user was the one use case that I had identified, and that you let me continue to assume was the one existing use case. > >> 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. > > No, I agreed with you that there is a current problem with > "stericsson,ab8500-pwm", as identified by Michael. I didn't actually > know about this issue until *after* drafting this patch-set. To be > clear the "stericsson,ab8500-pwm" scenario is not the reason for this > set's existence. So now I know that drivers/mfd/ab8500-core.c is totally unrelated to this patch series, and not the intended user of the new functionality. > > Also, please forget about the OF_MFD_* macros, they are totally > agnostic to this effort. The only relevance they have here is the > addition of 1 extra macro which *could* be used to provide the 'reg' > property where appropriate. My point was that my search for the data that comprised the "cell" parameter passed to mfd_add_devices() was inadequate, because I was searching on OF_MFD_CELL() instead of mfd_add_devices. I was admitting that part of my ignorance was because of this poor search. I was searching for where the problem case actually occurred in the kernel. Maybe you did not realize that I have been thinking that the only place where the problem case occurred was the single case I found with this insufficient search method. In some or many or all (I don't know, I'm not going to go back and search for all of them) you can probably replace mention of the OF_MFD_* with either my search for input data to mfd_add_devices() _or_ a concise reference to the new of_reg and use_of_reg fields of struct mfd_cell and the use of the new fields. Where is the problem that the patch set was intended to fix? > >> 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(). > > That is not my vision. There is no need for "stericsson,ab8500-pwm" > to have 'reg' properties as far as I see it. In that case the binding document for the mfd child node with compatible "stericsson,ab8500-pwm" should be updated to state that if there are multiple such child nodes with the same parent then they must contain exactly the same set of properties and values. Maybe not your problem, I have no idea who is responsible for that update. However, > >> This is another problem with the patch series: there is no user >> of OF_MFD_CELL_REG(). Please add one to the series. > > That's not a problem with this patch-set, it's a problem with your > understanding of this patch-set. :) I have already responded above about whether there should be a user of OF_MFD_CELL_REG() in the patch set. > > As far as I know, there aren't any current users who would benefit > from this work. Sigh. From the original patch 1/3 header: "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." This implies that there is a current instance where multiple devices are "allocated a pointer to the *first* node". If the patch header had instead said something like: adding the ability for an mfd device to have multiple children with the same value of "compatible" property then my whole approach to trying to analyze and understand the patch series would have been entirely different. One of my early replies described my attempt to find the code that was encountering the problem that the patch series claimed to fix. One of my concerns was handling potential compatibility issues with existing FDTs. And my understanding of your response to my analysis and investigation was that I had indeed found a problem case in existing code. But now you tell me that the driver and mfd child node compatible value that I identified are not at all a problem. > Instead, it is designed to provide future submitters > with another tool to help them link their child devices to the correct > OF nodes. And that is what I was looking for above in this reply, looking for a user of the new functionality in the patch series, where I stated: "Or at least a mention of a specific plan to use the functionality." > That's not to say that current users can't and won't > benefit from this. Just that they are not the target audience. > >>>>> 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"). > > Because DT is meant to describe the hardware, not the implementation. > > DT does not know, or care that in our case most operations that happen > on the platform are passed back via an API to a central controlling > location. Or that in reality, the OF node in this situation is > superfluous. > > Can we please stop talking about the AB8500. It doesn't have anything > to do with this series besides the fact that if it (this set) had > existed *before* 'ab8500-pwm' was OF enabled, it wouldn't now be > wonky. OK. I now understand that you don't expect the new functionality of the of_reg and use_of_reg fields of struct mfd_cell to be used by in relation to "ab8500-pwm" and drivers/mfd/ab8500-core.c. I will drop them from the discussion. > >> 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? > > If you want to talk about the AB8500, please start a new thread. > >>>> 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)? > > Please see "[0]" above! > > AFAICT your solution #2 involves bombing out *all* devices if there is > a duplicate compatible with no 'reg' property value. This is a) > over-kill and b) not an error, as I mentioned: As I mentioned above, I set you up to have this misunderstanding by a mistake in one of my earlier emails. So now that I have pointed out what I meant here by "more gracious handling of the detected error", what do you think of my amended solution #2? > >>> It also suffers with false positives. > Sorry for the very long response, but it seemed we were operating under some different understandings and I hope I have clarified some things. -Frank
On 2020-06-11 14:10, Lee Jones wrote: > 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. As you mentioned elsewhere in this thread, this series "fixes" the problem related to the "stericsson,ab8500-pwm" compatible. I know, I said I would drop discussion of that compatible, but bear with me for a second. :-) The "problem" is that the devices for multiple mfd child nodes with the same compatible value of "stericsson,ab8500-pwm" will all have a pointer to the first child node. At the moment the same child of_node being used by more than one device does not cause any incorrect behavior. Just in case the driver for "stericsson,ab8500-pwm" is modified in a way that the child of_node needs to be distinct for each device, and that changes gets back ported, it would be useful to have Fixes: tags for this patch series. So, at your discretion (and I'll let you worry about the correct Fixes: tag format), this series fixes: bad76991d7847b7877ae797cc79745d82ffd9120 mfd: Register ab8500 devices using the newly DT:ed MFD API c94bb233a9fee3314dc5d9c7de9fa702e91283f2 mfd: Make MFD core code Device Tree and IRQ domain aware -Frank > > 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 <lee.jones@linaro.org> > --- > > 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; > >
On Tue, 23 Jun 2020, Frank Rowand wrote: > On 2020-06-11 14:10, Lee Jones wrote: > > 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. > > As you mentioned elsewhere in this thread, this series "fixes" the > problem related to the "stericsson,ab8500-pwm" compatible. > > I know, I said I would drop discussion of that compatible, but bear > with me for a second. :-) > > The "problem" is that the devices for multiple mfd child nodes with > the same compatible value of "stericsson,ab8500-pwm" will all have > a pointer to the first child node. At the moment the same child > of_node being used by more than one device does not cause any > incorrect behavior. > > Just in case the driver for "stericsson,ab8500-pwm" is modified > in a way that the child of_node needs to be distinct for each > device, and that changes gets back ported, it would be useful > to have Fixes: tags for this patch series. > > So, at your discretion (and I'll let you worry about the correct > Fixes: tag format), this series fixes: > > bad76991d7847b7877ae797cc79745d82ffd9120 mfd: Register ab8500 devices using the newly DT:ed MFD API This patch isn't actually broken. The issue is the DTB, which [0] addresses. [0] https://lkml.kernel.org/lkml/20200622083432.1491715-1-lee.jones@linaro.org/ > c94bb233a9fee3314dc5d9c7de9fa702e91283f2 mfd: Make MFD core code Device Tree and IRQ domain aware It sounds reasonable to list this one, thanks. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
On Tue, 23 Jun 2020, Frank Rowand wrote: > On 2020-06-11 14:10, Lee Jones wrote: > > 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 <lee.jones@linaro.org> > > --- > > > > 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/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 > > + */ > > Instead of the above comment, suggest something like instead (I have not properly > line wrapped, to make it easier to see the difference): > > > + /* > > + * Address as defined in Device Tree mfd child node "reg" property. Used in combination with 'of_compatible' > > + * (above) when matching OF nodes with devices that have identical > > + * compatible strings > > + */ I'll split the difference and make it more clear, thanks. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
On Tue, 23 Jun 2020, Frank Rowand wrote: > On 2020-06-23 14:59, Lee Jones wrote: > > Suggestion #2 > > > >>>>>> 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. > > > > My reply to suggestion #2 > > > >>>>> 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? > > I could have sworn that I had replied with a solution to this issue. So I > searched and searched and searched my emails in the thread. And checked my > drafts email folder. Then finally realized I had made a stupid mistake. > > I did reply about this, but I had put my "-Frank" signature at the end > of a comment much higher in the email. So of course I expect you stopped > reading at that point and never saw my answer. My apologies!!! > > The email in question is: > > https://lore.kernel.org/linux-devicetree/eae9cc00-e67a-cb6a-37c2-f2235782ed77@gmail.com/ > > and what I wrote at this point in that email is: > > 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(). > > Thus a modification to my suggestion #2 to make it _not_ fail > everything. > > I didn't really flesh out all that "more gracefully handle" means. > Among other things, it could include a pr_warn() that provides > a fairly specific possible cause of the problem (eg the corner > case mentioned near the end of the patch 1/3 header that shows > mixing OF_MFD_CELL() and OF_MFD_CELL_REG() for the same compatible > value. It may be tricky coming up with good phrasing in a pr_warn() > that describes the generic issue instead of the specific example. The current solution already provides a warning if we fail to match a device with its requested OF node. It's also semantically incorrect to error out just because a node with the same compatible string is already taken, since there maybe another one coming up (which will be found on the next iteration post return of -EAGAIN). Providing a more accurate warning describing *why* a node wasn't found it also non-trivial, for the same reasons as it's hard to do during a pre-validation routine. > >>> [0] > >> > >> Is "[0]" the patch series, especially patch 1/3? > > > > No, this is my reply to your suggestion #2. > > > > The [0] is referenced further down. > > > > [...] > > > >>>>> * 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. > >> > > Start of my comment that I wrote with "too many shortcuts" (see below): > > >> 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. > > > > The OF_MFD_CELL_REG() macro didn't exist until this patch-set. > > > Maybe the way I wrote that took too many shortcuts. Let me re-phrase. > > I thought that one of the points of this patch set was to add the > of_reg and use_of_reg fields to struct mfd_cell. The expected use > of the of_reg and use_of_reg fields was to allow the presence of a > "reg" property in a devicetree mfd child node to be used to match > specific child nodes to specific elements of the mfd_add_devices() > cell array parameter, with the match occurring when the array elements > are processed (currently in mfd_match_of_node_to_dev(), which is > called by mfd_add_device()). > > The key point being the matching specific devicetree mfd child nodes > to specific cell array members. > > The OF_MFD_CELL_REG() is simply a helper macro related to the above. Correct so far. > > There are currently no users. > > Yes. And as I pointed out elsewhere, I would expect a user of new > functionality to be added as part of a patch series that adds the > new functionality. Or at least a mention of a specific plan to > use the functionality. There have been 2 such use-cases in recent months. The most recent is Michael's (CC'ed) use-case. Moreover, this patch-set actually fixes something that has been known to be broken (actually not broken per-say, just 'less featureful') for a number of years. Since the original support was authored, only 2 potential issues have arisen. The first time we deemed it a problem too complex to fix (I can't, for the life of me find that thread, but I'm pretty sure it involved Robin from Arm [which is why he is CC'ed]). Michael's use-case is the second. This time I thought I'd have a stab at fixing (or at least bettering) it. > I had been assuming that the intended user was the one use case that > I had identified, and that you let me continue to assume was the one > existing use case. Sorry if you feel misled, but that is not the case. > >> 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. > > > > No, I agreed with you that there is a current problem with > > "stericsson,ab8500-pwm", as identified by Michael. I didn't actually > > know about this issue until *after* drafting this patch-set. To be > > clear the "stericsson,ab8500-pwm" scenario is not the reason for this > > set's existence. > > So now I know that drivers/mfd/ab8500-core.c is totally unrelated to > this patch series, and not the intended user of the new functionality. > > > > > Also, please forget about the OF_MFD_* macros, they are totally > > agnostic to this effort. The only relevance they have here is the > > addition of 1 extra macro which *could* be used to provide the 'reg' > > property where appropriate. > > My point was that my search for the data that comprised the "cell" > parameter passed to mfd_add_devices() was inadequate, because I > was searching on OF_MFD_CELL() instead of mfd_add_devices. I was > admitting that part of my ignorance was because of this poor search. > > I was searching for where the problem case actually occurred in the > kernel. Maybe you did not realize that I have been thinking that > the only place where the problem case occurred was the single case > I found with this insufficient search method. > > In some or many or all (I don't know, I'm not going to go back > and search for all of them) you can probably replace mention > of the OF_MFD_* with either my search for input data to > mfd_add_devices() _or_ a concise reference to the new of_reg > and use_of_reg fields of struct mfd_cell and the use of the > new fields. > > Where is the problem that the patch set was intended to fix? Firstly, the problem is present, as described in the commit message. It is *not correct to re-match an OF node with multiple devices. Even if there wasn't a current *real* user, this patch is still the right thing to do, as it makes the situation *sooo* much better. However, there is a prospective user also [1]. [1] https://lore.kernel.org/linux-arm-kernel/20200423174543.17161-1-michael@walle.cc/ > >> 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(). > > > > That is not my vision. There is no need for "stericsson,ab8500-pwm" > > to have 'reg' properties as far as I see it. > > In that case the binding document for the mfd child node with > compatible "stericsson,ab8500-pwm" should be updated to state > that if there are multiple such child nodes with the same parent > then they must contain exactly the same set of properties and > values. > > Maybe not your problem, I have no idea who is responsible for > that update. This is the case for all OF nodes, not just 'ab8500-pwm'. Fell free to submit a patch. > However, > > >> This is another problem with the patch series: there is no user > >> of OF_MFD_CELL_REG(). Please add one to the series. > > > > That's not a problem with this patch-set, it's a problem with your > > understanding of this patch-set. :) > > I have already responded above about whether there should be a user > of OF_MFD_CELL_REG() in the patch set. No need, as it's indented for *future* users. That said, once applied, I will have a look around for some more potential current issues/users. My plan it so also start converting users to other OF_MFD_* macros. > > As far as I know, there aren't any current users who would benefit > > from this work. > > Sigh. From the original patch 1/3 header: > > "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." > > This implies that there is a current instance where multiple devices > are "allocated a pointer to the *first* node". > > If the patch header had instead said something like: > > adding the ability for an mfd device to have multiple children > with the same value of "compatible" property > > then my whole approach to trying to analyze and understand the > patch series would have been entirely different. One of my > early replies described my attempt to find the code that was > encountering the problem that the patch series claimed to fix. > One of my concerns was handling potential compatibility issues > with existing FDTs. > > And my understanding of your response to my analysis and investigation > was that I had indeed found a problem case in existing code. But now > you tell me that the driver and mfd child node compatible value that > I identified are not at all a problem. Again, I'm sorry that you took that path, but my replies have only conveyed the facts as I see them. The snippet that you quote above was and is still an accurate and precise description of the issue with the current matching code. Maybe now that you have identified your issue, we can move on. > > Instead, it is designed to provide future submitters > > with another tool to help them link their child devices to the correct > > OF nodes. > > And that is what I was looking for above in this reply, looking for > a user of the new functionality in the patch series, where I stated: > > "Or at least a mention of a specific plan to > use the functionality." One more time; this patch-set addresses an present in-kernel issue. The current code does it's best to match device with OF node, but there are corner-cases where the matching semantics are not sufficient. Even if there weren't any prospective users (but there are), improving the current matching logic is something that *must* be seen as a positive step in the right direction. If this code was already present when 'ab8500-pwm' was OF enabled, it would have identified the potential issue which you (and Michael) correctly identified with missing OF nodes. > > That's not to say that current users can't and won't > > benefit from this. Just that they are not the target audience. > > > >>>>> 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"). > > > > Because DT is meant to describe the hardware, not the implementation. > > > > DT does not know, or care that in our case most operations that happen > > on the platform are passed back via an API to a central controlling > > location. Or that in reality, the OF node in this situation is > > superfluous. > > > > Can we please stop talking about the AB8500. It doesn't have anything > > to do with this series besides the fact that if it (this set) had > > existed *before* 'ab8500-pwm' was OF enabled, it wouldn't now be > > wonky. > > OK. I now understand that you don't expect the new functionality of > the of_reg and use_of_reg fields of struct mfd_cell to be used by > in relation to "ab8500-pwm" and drivers/mfd/ab8500-core.c. I will > drop them from the discussion. \o/ > >> 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? > > > > If you want to talk about the AB8500, please start a new thread. > > > >>>> 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)? > > > > Please see "[0]" above! > > > > AFAICT your solution #2 involves bombing out *all* devices if there is > > a duplicate compatible with no 'reg' property value. This is a) > > over-kill and b) not an error, as I mentioned: > > As I mentioned above, I set you up to have this misunderstanding by > a mistake in one of my earlier emails. So now that I have pointed > out what I meant here by "more gracious handling of the detected > error", what do you think of my amended solution #2? Explained above, but the LT;DR is that it's not correct. > >>> It also suffers with false positives. > > > > Sorry for the very long response, but it seemed we were operating > under some different understandings and I hope I have clarified some > things. Likewise. :) -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Hi, Am 2020-06-24 08:41, schrieb Lee Jones: > On Tue, 23 Jun 2020, Frank Rowand wrote: > >> On 2020-06-11 14:10, Lee Jones wrote: >> > 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. >> >> As you mentioned elsewhere in this thread, this series "fixes" the >> problem related to the "stericsson,ab8500-pwm" compatible. >> >> I know, I said I would drop discussion of that compatible, but bear >> with me for a second. :-) >> >> The "problem" is that the devices for multiple mfd child nodes with >> the same compatible value of "stericsson,ab8500-pwm" will all have >> a pointer to the first child node. At the moment the same child >> of_node being used by more than one device does not cause any >> incorrect behavior. >> >> Just in case the driver for "stericsson,ab8500-pwm" is modified >> in a way that the child of_node needs to be distinct for each >> device, and that changes gets back ported, it would be useful >> to have Fixes: tags for this patch series. >> >> So, at your discretion (and I'll let you worry about the correct >> Fixes: tag format), this series fixes: >> >> bad76991d7847b7877ae797cc79745d82ffd9120 mfd: Register ab8500 devices >> using the newly DT:ed MFD API > > This patch isn't actually broken. > > The issue is the DTB, which [0] addresses. > > [0] > https://lkml.kernel.org/lkml/20200622083432.1491715-1-lee.jones@linaro.org/ Now, I'm confused; because this patch doesn't use the reg property but a different node name. I'd actually prefer this for any MFD driver which has multiple nodes of the same compatible string. See my reasoning here [1]. But until now, no one has responded. Thus, I'd rather see a OF_MFD_CELL_NAME() which matches the node name instead of the OF_MFD_CELL_REG() macro. This would also circumvent the fact that the unit-address has one number space. Eg. it is not possible to have: mfd { compatible = "mfd,compatible"; gpio@0 { reg = <0>; }; gpio@1 { reg = <1>; }; pwm@0 { reg = <0>; }; }; Although Rob mentioned to maybe relax that, but I sill fail to see the advantage to have an arbitrary reg property instead of a unique node name. [1] https://lore.kernel.org/linux-devicetree/0709f20bc61afb6656bc57312eb69f56@walle.cc/ -- -michael
On Wed, 24 Jun 2020, Michael Walle wrote: > Hi, > > Am 2020-06-24 08:41, schrieb Lee Jones: > > On Tue, 23 Jun 2020, Frank Rowand wrote: > > > > > On 2020-06-11 14:10, Lee Jones wrote: > > > > 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. > > > > > > As you mentioned elsewhere in this thread, this series "fixes" the > > > problem related to the "stericsson,ab8500-pwm" compatible. > > > > > > I know, I said I would drop discussion of that compatible, but bear > > > with me for a second. :-) > > > > > > The "problem" is that the devices for multiple mfd child nodes with > > > the same compatible value of "stericsson,ab8500-pwm" will all have > > > a pointer to the first child node. At the moment the same child > > > of_node being used by more than one device does not cause any > > > incorrect behavior. > > > > > > Just in case the driver for "stericsson,ab8500-pwm" is modified > > > in a way that the child of_node needs to be distinct for each > > > device, and that changes gets back ported, it would be useful > > > to have Fixes: tags for this patch series. > > > > > > So, at your discretion (and I'll let you worry about the correct > > > Fixes: tag format), this series fixes: > > > > > > bad76991d7847b7877ae797cc79745d82ffd9120 mfd: Register ab8500 > > > devices using the newly DT:ed MFD API > > > > This patch isn't actually broken. > > > > The issue is the DTB, which [0] addresses. > > > > [0] > > https://lkml.kernel.org/lkml/20200622083432.1491715-1-lee.jones@linaro.org/ > > Now, I'm confused; because this patch doesn't use the reg property > but a different node name. The fix mentioned above is orthogonal to this set. The *only* reason for the differing node names there is to circumvent the following DTC warnings: arch/arm/boot/dts/ste-ab8500.dtsi:210.16-214.7: ERROR (duplicate_node_names): /soc/prcmu@80157000/ab8500/ab8500-pwm: Duplicate node name arch/arm/boot/dts/ste-ab8500.dtsi:216.16-220.7: ERROR (duplicate_node_names): /soc/prcmu@80157000/ab8500/ab8500-pwm: Duplicate node name arch/arm/boot/dts/ste-ab8500.dtsi:216.16-220.7: ERROR (duplicate_node_names): /soc/prcmu@80157000/ab8500/ab8500-pwm: Duplicate node name > I'd actually prefer this for any MFD > driver which has multiple nodes of the same compatible string. See > my reasoning here [1]. But until now, no one has responded. Thus, > I'd rather see a OF_MFD_CELL_NAME() which matches the node name > instead of the OF_MFD_CELL_REG() macro. > > This would also circumvent the fact that the unit-address has one > number space. Eg. it is not possible to have: > > mfd { > compatible = "mfd,compatible"; > > gpio@0 { > reg = <0>; > }; > gpio@1 { > reg = <1>; > }; > pwm@0 { > reg = <0>; > }; > }; > > Although Rob mentioned to maybe relax that, but I sill fail to see > the advantage to have an arbitrary reg property instead of a unique > node name. I don't have a strong opinion either way. We can *also* add node name matching if Rob deems it fit. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Am 2020-06-24 10:23, schrieb Lee Jones: > On Wed, 24 Jun 2020, Michael Walle wrote: [..] >> Although Rob mentioned to maybe relax that, but I sill fail to see >> the advantage to have an arbitrary reg property instead of a unique >> node name. > > I don't have a strong opinion either way. > > We can *also* add node name matching if Rob deems it fit. Where do you see a use of the reg property? You already expressed that you see exposing the internal offset as a hack: "Placing "internal offsets" into the 'reg' property is a hack." [1] So what are you putting into reg instead? Rob suggested "anything" documented in the hardware manual. But isn't this just also something we make up and especially for the MFD driver. Thus IMHO it doesn't qualify as a unit-address, which - as far as I understand it - is unique on the parent bus. To repeat my argument, its not a defined thing like an I2C address. [1] https://lore.kernel.org/linux-devicetree/20200609185231.GO4106@dell/ -michael
On Wed, 24 Jun 2020, Michael Walle wrote: > Am 2020-06-24 10:23, schrieb Lee Jones: > > On Wed, 24 Jun 2020, Michael Walle wrote: > > [..] > > > > Although Rob mentioned to maybe relax that, but I sill fail to see > > > the advantage to have an arbitrary reg property instead of a unique > > > node name. > > > > I don't have a strong opinion either way. > > > > We can *also* add node name matching if Rob deems it fit. > > Where do you see a use of the reg property? The vast proportion of devices do and will have 'reg' properties. > You already expressed > that you see exposing the internal offset as a hack: > > "Placing "internal offsets" into the 'reg' property is a hack." [1] > > So what are you putting into reg instead? Rob suggested "anything" > documented in the hardware manual. But isn't this just also something > we make up and especially for the MFD driver. Thus IMHO it doesn't > qualify as a unit-address, which - as far as I understand it - is > unique on the parent bus. To repeat my argument, its not a defined > thing like an I2C address. So our argument in the past (although this is going back the best part of 10 years) has always been that; if devices are different, there is almost always a documented (in the H/W manual/datasheet) way to differentiate/address them. Whether that's a physical address, an offset, a bank ID, an I2C/SPI address or whatever. As to not being able to use that address/ID due to the DT rules surrounding address space as per the example in your previous email, well that's a rule specific to DT and makes little sense in some real world cases (such as, dare I say it, the AB8500). You'll have to take the aforementioned point and the point about using node names instead of 'reg' properties up with Rob and the other interested DT folk. Again, I'm happy to extend that functionality if it becomes acceptable practice. > [1] https://lore.kernel.org/linux-devicetree/20200609185231.GO4106@dell/ > > -michael -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
On 2020-06-24 02:46, Lee Jones wrote: > On Tue, 23 Jun 2020, Frank Rowand wrote: > >> On 2020-06-23 14:59, Lee Jones wrote: < big snip > Thanks for the replies in the above portion. >>>> 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)? >>> >>> Please see "[0]" above! >>> >>> AFAICT your solution #2 involves bombing out *all* devices if there is >>> a duplicate compatible with no 'reg' property value. This is a) >>> over-kill and b) not an error, as I mentioned: >> >> As I mentioned above, I set you up to have this misunderstanding by >> a mistake in one of my earlier emails. So now that I have pointed >> out what I meant here by "more gracious handling of the detected >> error", what do you think of my amended solution #2? > > Explained above, but the LT;DR is that it's not correct. I don't agree with you, I think my solution is better. Even if I prefer my solution, I find your solution to be good enough. So I am dropping my specific objection to returning -EAGAIN from mfd_match_of_node_to_dev() when the node has previously been allocated to a device. > >>>>> It also suffers with false positives. >>> >> >> Sorry for the very long response, but it seemed we were operating >> under some different understandings and I hope I have clarified some >> things. > > Likewise. :) >
On Wed, 24 Jun 2020, Frank Rowand wrote: > On 2020-06-24 02:46, Lee Jones wrote: > > On Tue, 23 Jun 2020, Frank Rowand wrote: > > > >> On 2020-06-23 14:59, Lee Jones wrote: > > < big snip > > > Thanks for the replies in the above portion. NP. > >>>> 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)? > >>> > >>> Please see "[0]" above! > >>> > >>> AFAICT your solution #2 involves bombing out *all* devices if there is > >>> a duplicate compatible with no 'reg' property value. This is a) > >>> over-kill and b) not an error, as I mentioned: > >> > >> As I mentioned above, I set you up to have this misunderstanding by > >> a mistake in one of my earlier emails. So now that I have pointed > >> out what I meant here by "more gracious handling of the detected > >> error", what do you think of my amended solution #2? > > > > Explained above, but the LT;DR is that it's not correct. > > I don't agree with you, I think my solution is better. Even if I > prefer my solution, I find your solution to be good enough. I still don't see how it could work, but please feel free to submit a subsequent patch and we can discuss it on its own merits. > So I am dropping my specific objection to returning -EAGAIN from > mfd_match_of_node_to_dev() when the node has previously been > allocated to a device. Great. Thanks for taking an interest. Does this mean I can apply your Reviewed-by? -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
On 2020-06-24 11:14, Lee Jones wrote: > On Wed, 24 Jun 2020, Frank Rowand wrote: > >> On 2020-06-24 02:46, Lee Jones wrote: >>> On Tue, 23 Jun 2020, Frank Rowand wrote: >>> >>>> On 2020-06-23 14:59, Lee Jones wrote: >> >> < big snip > >> >> Thanks for the replies in the above portion. > > NP. > >>>>>> 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)? >>>>> >>>>> Please see "[0]" above! >>>>> >>>>> AFAICT your solution #2 involves bombing out *all* devices if there is >>>>> a duplicate compatible with no 'reg' property value. This is a) >>>>> over-kill and b) not an error, as I mentioned: >>>> >>>> As I mentioned above, I set you up to have this misunderstanding by >>>> a mistake in one of my earlier emails. So now that I have pointed >>>> out what I meant here by "more gracious handling of the detected >>>> error", what do you think of my amended solution #2? >>> >>> Explained above, but the LT;DR is that it's not correct. >> >> I don't agree with you, I think my solution is better. Even if I >> prefer my solution, I find your solution to be good enough. > > I still don't see how it could work, but please feel free to submit a > subsequent patch and we can discuss it on its own merits. > >> So I am dropping my specific objection to returning -EAGAIN from >> mfd_match_of_node_to_dev() when the node has previously been >> allocated to a device. > > Great. Thanks for taking an interest. > > Does this mean I can apply your Reviewed-by? > No, please do not. I don't want to give the patch that strong of an endorsement.
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;
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 <lee.jones@linaro.org> --- 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(-) -- 2.25.1