diff mbox series

[v4,4/8] of: property: Add fw_devlink support for optional properties

Message ID 20210205222644.2357303-5-saravanak@google.com
State Accepted
Commit a9dd8f3c2cf3413d9302ef12e88181cf001c3ed8
Headers show
Series Make fw_devlink=on more forgiving | expand

Commit Message

Saravana Kannan Feb. 5, 2021, 10:26 p.m. UTC
Not all DT bindings are mandatory bindings. Add support for optional DT
bindings and mark iommus, iommu-map, dmas as optional DT bindings.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Rob Herring (Arm) Feb. 9, 2021, 9:33 p.m. UTC | #1
On Fri, Feb 05, 2021 at 02:26:40PM -0800, Saravana Kannan wrote:
> Not all DT bindings are mandatory bindings. Add support for optional DT

> bindings and mark iommus, iommu-map, dmas as optional DT bindings.


I don't think we can say these are optional or not. It's got to be a 
driver decision somehow.

For example, if IOMMU is optional, what happens with this sequence:

driver probes without IOMMU
driver calls dma_map_?()
IOMMU driver probes
h/w accesses DMA buffer --> BOOM!
Geert Uytterhoeven Feb. 10, 2021, 8:25 a.m. UTC | #2
Hi Saravana,

CC iommu

On Tue, Feb 9, 2021 at 10:55 PM Saravana Kannan <saravanak@google.com> wrote:
> On Tue, Feb 9, 2021 at 1:33 PM Rob Herring <robh@kernel.org> wrote:

> > On Fri, Feb 05, 2021 at 02:26:40PM -0800, Saravana Kannan wrote:

> > > Not all DT bindings are mandatory bindings. Add support for optional DT

> > > bindings and mark iommus, iommu-map, dmas as optional DT bindings.

> >

> > I don't think we can say these are optional or not. It's got to be a

> > driver decision somehow.

>

> Right, so maybe the word "optional" isn't a good name for it. I can

> change that if you want.

>

> The point being, fw_devlink can't block the probe of this driver based

> on iommu property. We let the driver decide if it wants to

> -EPROBE_DEFER or not or however it wants to handle this.


The driver cannot make that decision, cfr. below.

> > For example, if IOMMU is optional, what happens with this sequence:

> >

> > driver probes without IOMMU

> > driver calls dma_map_?()

> > IOMMU driver probes

> > h/w accesses DMA buffer --> BOOM!


Does it really behave that way? Or does it continue without IOMMU?

> Right. But how is this really related to fw_devlink? AFAICT, this is

> an issue even today. If the driver needs the IOMMU, then it needs to

> make sure the IOMMU has probed? What am I missing?


Individual I/O (IOMMU slave) drivers are completely unaware of the
presence or absence of an IOMMU; they just use the DMA API, which is the
same regardless of an IOMMU being used or not.
While for GPIO/IRQ/CLK/DMA/... have request/get_{gpio,irq,clk,dma,...}
APIs for a driver to get a reference, which can return -EPROBE_DEFER, no
such thing exists for IOMMUs.  This is handled by the IOMMU core
instead.

Using the IOMMU or not is more like a system policy decision.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 53d163c8d39b..962109082df1 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1235,6 +1235,7 @@  static struct device_node *parse_##fname(struct device_node *np,	     \
 struct supplier_bindings {
 	struct device_node *(*parse_prop)(struct device_node *np,
 					  const char *prop_name, int index);
+	bool optional;
 };
 
 DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
@@ -1308,12 +1309,12 @@  static struct device_node *parse_interrupts(struct device_node *np,
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
-	{ .parse_prop = parse_iommus, },
-	{ .parse_prop = parse_iommu_maps, },
+	{ .parse_prop = parse_iommus, .optional = true, },
+	{ .parse_prop = parse_iommu_maps, .optional = true, },
 	{ .parse_prop = parse_mboxes, },
 	{ .parse_prop = parse_io_channels, },
 	{ .parse_prop = parse_interrupt_parent, },
-	{ .parse_prop = parse_dmas, },
+	{ .parse_prop = parse_dmas, .optional = true, },
 	{ .parse_prop = parse_power_domains, },
 	{ .parse_prop = parse_hwlocks, },
 	{ .parse_prop = parse_extcon, },
@@ -1368,6 +1369,11 @@  static int of_link_property(struct device_node *con_np, const char *prop_name)
 
 	/* Do not stop at first failed link, link all available suppliers. */
 	while (!matched && s->parse_prop) {
+		if (s->optional && !fw_devlink_is_strict()) {
+			s++;
+			continue;
+		}
+
 		while ((phandle = s->parse_prop(con_np, prop_name, i))) {
 			matched = true;
 			i++;