[RFC,v3,1/8] of: Mark interconnects property supplier as optional

In order to set their correct DMA address offset, some devices rely on
the device-tree interconnects property which identifies an
interconnect node that provides a dma-ranges property that can be used
to set said offset.

Since that logic is all handled by the generic openfirmware and driver
code, the device-tree description could be enough to properly set
the offset.

However the interconnects property is currently not marked as
optional, which implies that a driver for the corresponding node
must be loaded as a requirement. When no such driver exists, this
results in an endless EPROBE_DEFER which gets propagated to the
calling driver. This ends up in the driver never loading.

Marking the interconnects property as optional makes it possible
to load the driver in that situation, since the EPROBE_DEFER return
code will no longer be propagated to the driver.

There might however be undesirable consequences with this change,
which I do not fully grasp at this point.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/of/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

+Saravana

On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote:
> In order to set their correct DMA address offset, some devices rely on
> the device-tree interconnects property which identifies an
> interconnect node that provides a dma-ranges property that can be used
> to set said offset.
> 
> Since that logic is all handled by the generic openfirmware and driver
> code, the device-tree description could be enough to properly set
> the offset.
> 
> However the interconnects property is currently not marked as
> optional, which implies that a driver for the corresponding node
> must be loaded as a requirement. When no such driver exists, this
> results in an endless EPROBE_DEFER which gets propagated to the
> calling driver. This ends up in the driver never loading.
> 
> Marking the interconnects property as optional makes it possible
> to load the driver in that situation, since the EPROBE_DEFER return
> code will no longer be propagated to the driver.
> 
> There might however be undesirable consequences with this change,
> which I do not fully grasp at this point.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 8e90071de6ed..ef7c56b510e8 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1365,7 +1365,7 @@ 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_interconnects, .optional = true,},
>  	{ .parse_prop = parse_iommus, .optional = true, },
>  	{ .parse_prop = parse_iommu_maps, .optional = true, },
>  	{ .parse_prop = parse_mboxes, },
> -- 
> 2.35.1
> 
>

On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote:
>
> +Saravana
>
> On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote:
> > In order to set their correct DMA address offset, some devices rely on
> > the device-tree interconnects property which identifies an
> > interconnect node that provides a dma-ranges property that can be used
> > to set said offset.
> >
> > Since that logic is all handled by the generic openfirmware and driver
> > code, the device-tree description could be enough to properly set
> > the offset.
> >
> > However the interconnects property is currently not marked as
> > optional, which implies that a driver for the corresponding node
> > must be loaded as a requirement. When no such driver exists, this
> > results in an endless EPROBE_DEFER which gets propagated to the
> > calling driver. This ends up in the driver never loading.
> >
> > Marking the interconnects property as optional makes it possible
> > to load the driver in that situation, since the EPROBE_DEFER return
> > code will no longer be propagated to the driver.
> >
> > There might however be undesirable consequences with this change,
> > which I do not fully grasp at this point.

Temporary NACK till I get a bit more time to take a closer look. I
really don't like the idea of making interconnects optional. IOMMUs
and DMAs were exceptions. Also, we kinda discuss similar issues in
LPC. We had some consensus on how to handle these and I noted them all
down with a lot of details -- let me go take a look at those notes
again and see if I can send a more generic patch.

Paul,

Can you point to the DTS (not DTSI) file that corresponds to this?
Also, if it's a builtin kernel, I'd recommend setting
deferred_probe_timeout=1 and that should take care of it too.

-Saravana

> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/of/property.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 8e90071de6ed..ef7c56b510e8 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1365,7 +1365,7 @@ 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_interconnects, .optional = true,},
> >       { .parse_prop = parse_iommus, .optional = true, },
> >       { .parse_prop = parse_iommu_maps, .optional = true, },
> >       { .parse_prop = parse_mboxes, },
> > --
> > 2.35.1
> >
> >

Hi,

On Mon, Mar 07, 2022 at 07:34:22PM -0800, Saravana Kannan wrote:
> On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote:
> >
> > +Saravana
> >
> > On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote:
> > > In order to set their correct DMA address offset, some devices rely on
> > > the device-tree interconnects property which identifies an
> > > interconnect node that provides a dma-ranges property that can be used
> > > to set said offset.
> > >
> > > Since that logic is all handled by the generic openfirmware and driver
> > > code, the device-tree description could be enough to properly set
> > > the offset.
> > >
> > > However the interconnects property is currently not marked as
> > > optional, which implies that a driver for the corresponding node
> > > must be loaded as a requirement. When no such driver exists, this
> > > results in an endless EPROBE_DEFER which gets propagated to the
> > > calling driver. This ends up in the driver never loading.
> > >
> > > Marking the interconnects property as optional makes it possible
> > > to load the driver in that situation, since the EPROBE_DEFER return
> > > code will no longer be propagated to the driver.
> > >
> > > There might however be undesirable consequences with this change,
> > > which I do not fully grasp at this point.
> 
> Temporary NACK till I get a bit more time to take a closer look. I
> really don't like the idea of making interconnects optional. IOMMUs
> and DMAs were exceptions. Also, we kinda discuss similar issues in
> LPC. We had some consensus on how to handle these and I noted them all
> down with a lot of details -- let me go take a look at those notes
> again and see if I can send a more generic patch.
> 
> Paul,
> 
> Can you point to the DTS (not DTSI) file that corresponds to this?
> Also, if it's a builtin kernel, I'd recommend setting
> deferred_probe_timeout=1 and that should take care of it too.

For the record, I also encountered this today on next-20220726 with this
device:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun5i.dtsi#n775

The driver won't probe without fw_devlink=off

Maxime

On Wed, Jul 27, 2022 at 5:06 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Mon, Mar 07, 2022 at 07:34:22PM -0800, Saravana Kannan wrote:
> > On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > +Saravana
> > >
> > > On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote:
> > > > In order to set their correct DMA address offset, some devices rely on
> > > > the device-tree interconnects property which identifies an
> > > > interconnect node that provides a dma-ranges property that can be used
> > > > to set said offset.
> > > >
> > > > Since that logic is all handled by the generic openfirmware and driver
> > > > code, the device-tree description could be enough to properly set
> > > > the offset.
> > > >
> > > > However the interconnects property is currently not marked as
> > > > optional, which implies that a driver for the corresponding node
> > > > must be loaded as a requirement. When no such driver exists, this
> > > > results in an endless EPROBE_DEFER which gets propagated to the
> > > > calling driver. This ends up in the driver never loading.
> > > >
> > > > Marking the interconnects property as optional makes it possible
> > > > to load the driver in that situation, since the EPROBE_DEFER return
> > > > code will no longer be propagated to the driver.
> > > >
> > > > There might however be undesirable consequences with this change,
> > > > which I do not fully grasp at this point.
> >
> > Temporary NACK till I get a bit more time to take a closer look. I
> > really don't like the idea of making interconnects optional. IOMMUs
> > and DMAs were exceptions. Also, we kinda discuss similar issues in
> > LPC. We had some consensus on how to handle these and I noted them all
> > down with a lot of details -- let me go take a look at those notes
> > again and see if I can send a more generic patch.
> >
> > Paul,
> >
> > Can you point to the DTS (not DTSI) file that corresponds to this?
> > Also, if it's a builtin kernel, I'd recommend setting
> > deferred_probe_timeout=1 and that should take care of it too.
>
> For the record, I also encountered this today on next-20220726 with this
> device:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun5i.dtsi#n775
>
> The driver won't probe without fw_devlink=off

Really? I basically ended up doing what I mentioned in my original
reply. next-20220726 should have my changes that'll make sure
fw_devlink doesn't block any probe (it'll still try to create as many
device links as possible) after 10s (default deferred probe timeout).
Can you try to find more info on why it's not probing?
<debugfs>/devices_deferred should give more details.


-Saravana

Hi,

On Wed 27 Jul 22, 09:06, Saravana Kannan wrote:
> On Wed, Jul 27, 2022 at 5:06 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > On Mon, Mar 07, 2022 at 07:34:22PM -0800, Saravana Kannan wrote:
> > > On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > +Saravana
> > > >
> > > > On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote:
> > > > > In order to set their correct DMA address offset, some devices rely on
> > > > > the device-tree interconnects property which identifies an
> > > > > interconnect node that provides a dma-ranges property that can be used
> > > > > to set said offset.
> > > > >
> > > > > Since that logic is all handled by the generic openfirmware and driver
> > > > > code, the device-tree description could be enough to properly set
> > > > > the offset.
> > > > >
> > > > > However the interconnects property is currently not marked as
> > > > > optional, which implies that a driver for the corresponding node
> > > > > must be loaded as a requirement. When no such driver exists, this
> > > > > results in an endless EPROBE_DEFER which gets propagated to the
> > > > > calling driver. This ends up in the driver never loading.
> > > > >
> > > > > Marking the interconnects property as optional makes it possible
> > > > > to load the driver in that situation, since the EPROBE_DEFER return
> > > > > code will no longer be propagated to the driver.
> > > > >
> > > > > There might however be undesirable consequences with this change,
> > > > > which I do not fully grasp at this point.
> > >
> > > Temporary NACK till I get a bit more time to take a closer look. I
> > > really don't like the idea of making interconnects optional. IOMMUs
> > > and DMAs were exceptions. Also, we kinda discuss similar issues in
> > > LPC. We had some consensus on how to handle these and I noted them all
> > > down with a lot of details -- let me go take a look at those notes
> > > again and see if I can send a more generic patch.
> > >
> > > Paul,
> > >
> > > Can you point to the DTS (not DTSI) file that corresponds to this?
> > > Also, if it's a builtin kernel, I'd recommend setting
> > > deferred_probe_timeout=1 and that should take care of it too.
> >
> > For the record, I also encountered this today on next-20220726 with this
> > device:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun5i.dtsi#n775
> >
> > The driver won't probe without fw_devlink=off
> 
> Really? I basically ended up doing what I mentioned in my original
> reply. next-20220726 should have my changes that'll make sure
> fw_devlink doesn't block any probe (it'll still try to create as many
> device links as possible) after 10s (default deferred probe timeout).
> Can you try to find more info on why it's not probing?
> <debugfs>/devices_deferred should give more details.

By the way last time I checked the initial issue that I reported appeared to be
fixed by the patch (Extend deferred probe timeout on driver registration).

Cheers,

Paul

On Wed, Jul 27, 2022 at 10:17 AM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Wed 27 Jul 22, 09:06, Saravana Kannan wrote:
> > On Wed, Jul 27, 2022 at 5:06 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Mar 07, 2022 at 07:34:22PM -0800, Saravana Kannan wrote:
> > > > On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > +Saravana
> > > > >
> > > > > On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote:
> > > > > > In order to set their correct DMA address offset, some devices rely on
> > > > > > the device-tree interconnects property which identifies an
> > > > > > interconnect node that provides a dma-ranges property that can be used
> > > > > > to set said offset.
> > > > > >
> > > > > > Since that logic is all handled by the generic openfirmware and driver
> > > > > > code, the device-tree description could be enough to properly set
> > > > > > the offset.
> > > > > >
> > > > > > However the interconnects property is currently not marked as
> > > > > > optional, which implies that a driver for the corresponding node
> > > > > > must be loaded as a requirement. When no such driver exists, this
> > > > > > results in an endless EPROBE_DEFER which gets propagated to the
> > > > > > calling driver. This ends up in the driver never loading.
> > > > > >
> > > > > > Marking the interconnects property as optional makes it possible
> > > > > > to load the driver in that situation, since the EPROBE_DEFER return
> > > > > > code will no longer be propagated to the driver.
> > > > > >
> > > > > > There might however be undesirable consequences with this change,
> > > > > > which I do not fully grasp at this point.
> > > >
> > > > Temporary NACK till I get a bit more time to take a closer look. I
> > > > really don't like the idea of making interconnects optional. IOMMUs
> > > > and DMAs were exceptions. Also, we kinda discuss similar issues in
> > > > LPC. We had some consensus on how to handle these and I noted them all
> > > > down with a lot of details -- let me go take a look at those notes
> > > > again and see if I can send a more generic patch.
> > > >
> > > > Paul,
> > > >
> > > > Can you point to the DTS (not DTSI) file that corresponds to this?
> > > > Also, if it's a builtin kernel, I'd recommend setting
> > > > deferred_probe_timeout=1 and that should take care of it too.
> > >
> > > For the record, I also encountered this today on next-20220726 with this
> > > device:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun5i.dtsi#n775
> > >
> > > The driver won't probe without fw_devlink=off
> >
> > Really? I basically ended up doing what I mentioned in my original
> > reply. next-20220726 should have my changes that'll make sure
> > fw_devlink doesn't block any probe (it'll still try to create as many
> > device links as possible) after 10s (default deferred probe timeout).
> > Can you try to find more info on why it's not probing?
> > <debugfs>/devices_deferred should give more details.
>
> By the way last time I checked the initial issue that I reported appeared to be
> fixed by the patch (Extend deferred probe timeout on driver registration).

Thanks for the confirmation.

-Saravana