Re: [PATCH v4 4/8] of: property: Add fw_devlink support for optional properties
Hi Saravana,
CC iommu
On Tue, Feb 9, 2021 at 10:55 PM Saravana Kannan wrote:
> On Tue, Feb 9, 2021 at 1:33 PM Rob Herring 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 -- ge...@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
Re: [PATCH v4 4/8] of: property: Add fw_devlink support for optional properties
On Tue, Feb 9, 2021 at 1:33 PM Rob Herring 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. > 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! 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? -Saravana
Re: [PATCH v4 4/8] of: property: Add fw_devlink support for optional properties
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!
[PATCH v4 4/8] of: property: Add fw_devlink support for optional properties
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
---
drivers/of/property.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
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++;
--
2.30.0.478.g8a0d178c01-goog
