| Message ID | 20210205222644.2357303-5-saravanak@google.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Make fw_devlink=on more forgiving | expand |
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!
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. > 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
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
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++;
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(-)