| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200624064538.GD954398@dell>
Date: Wed, 24 Jun 2020 07:45:38 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Frank Rowand <frowand.list@...il.com>
Cc: andy.shevchenko@...il.com, michael@...le.cc, robh+dt@...nel.org,
broonie@...nel.org, devicetree@...r.kernel.org,
linus.walleij@...aro.org, linux@...ck-us.net,
andriy.shevchenko@...ux.intel.com, robin.murphy@....com,
gregkh@...uxfoundation.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match
devices with the correct of_nodes
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@...aaaaaaaaaaaaa {
> > compatible = "mfd,of-test-child";
> > reg = <0xaaaaaaaa 0xaaaaaaaa 0 0x11>,
> > <0xbbbbbbbb 0xbbbbbbbb 0 0x22>;
> > };
> >
> > child@...ccccc {
> > compatible = "mfd,of-test-child";
> > reg = <0x00000000 0xcccccccc 0 0x33>;
> > };
> >
> > child@...ddddd00000000 {
> > 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@...aaaaaaaaaaaaa
> > [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@...aaaaaaaaaaaaa
> > [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@...aaaaaaaaaaaaa
> >
> > 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@...aaaaaaaaaaaaa
> > [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@...ccccc
> > [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@...ddddd00000000
> >
> > 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@...ddddd00000000
> > [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@...aaaaaaaaaaaaa
> > [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@...ccccc
> >
> > 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@...aaaaaaaaaaaaa
> > [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@...ccccc
> >
> > 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@...aro.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
Powered by blists - more mailing lists