Re: [PATCH 05/11] of: Ratify of_dma_configure() interface

[Nicolas Saenz Julienne]

On Mon, 2019-09-30 at 16:24 -0500, Rob Herring wrote:
> On Mon, Sep 30, 2019 at 8:32 AM Nicolas Saenz Julienne
> <nsaenzjulienne@xxxxxxx> wrote:
> > On Mon, 2019-09-30 at 05:57 -0700, Christoph Hellwig wrote:
> > > On Thu, Sep 26, 2019 at 07:24:49PM -0500, Rob Herring wrote:
> > > > -int of_dma_configure(struct device *dev, struct device_node *np, bool
> > > > force_dma)
> > > > +int of_dma_configure(struct device *dev, struct device_node *parent,
> > > > bool
> > > > force_dma)
> > > 
> > > This creates a > 80 char line.
> > > 
> > > >  {
> > > >     u64 dma_addr, paddr, size = 0;
> > > >     int ret;
> > > >     bool coherent;
> > > >     unsigned long offset;
> > > >     const struct iommu_ops *iommu;
> > > > +   struct device_node *np;
> > > >     u64 mask;
> > > > 
> > > > +   np = dev->of_node;
> > > > +   if (!np)
> > > > +           np = parent;
> > > > +   if (!np)
> > > > +           return -ENODEV;
> > > 
> > > I have to say I find the older calling convention simpler to understand.
> > > If we want to enforce the invariant I'd rather do that explicitly:
> > > 
> > >       if (dev->of_node && np != dev->of_node)
> > >               return -EINVAL;
> > 
> > As is, this would break Freescale Layerscape fsl-mc bus' dma_configure():
> 
> This may break PCI too for devices that have a DT node.
> 
> > static int fsl_mc_dma_configure(struct device *dev)
> > {
> >         struct device *dma_dev = dev;
> > 
> >         while (dev_is_fsl_mc(dma_dev))
> >                 dma_dev = dma_dev->parent;
> > 
> >         return of_dma_configure(dev, dma_dev->of_node, 0);
> > }
> > 
> > But I think that with this series, given the fact that we now treat the lack
> > of
> > dma-ranges as a 1:1 mapping instead of an error, we could rewrite the
> > function
> > like this:
> 
> Now, I'm reconsidering allowing this abuse... It's better if the code
> which understands the bus structure in DT for a specific bus passes in
> the right thing. Maybe I should go back to Robin's version (below).
> OTOH, the existing assumption that 'dma-ranges' was in the immediate
> parent was an assumption on the bus structure which maybe doesn't
> always apply.
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index a45261e21144..6951450bb8f3 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -98,12 +98,15 @@ int of_dma_configure(struct device *dev, struct
> device_node *parent, bool force_
>         u64 mask;
> 
>         np = dev->of_node;
> -       if (!np)
> -               np = parent;
> +       if (np)
> +               parent = of_get_dma_parent(np);
> +       else
> +               np = of_node_get(parent);
>         if (!np)
>                 return -ENODEV;
> 
> -       ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> +       ret = of_dma_get_range(parent, &dma_addr, &paddr, &size);
> +       of_node_put(parent);
>         if (ret < 0) {
>                 /*
>                  * For legacy reasons, we have to assume some devices need

I spent some time thinking about your comments and researching. I came to the
realization that both these solutions break the usage in
drivers/gpu/drm/sun4i/sun4i_backend.c:805. In that specific case both
'dev->of_node' and 'parent' exist yet the device receiving the configuration
and 'parent' aren't related in any way.

IOW we can't just use 'dev->of_node' as a starting point to walk upwards the
tree. We always have to respect whatever DT node the bus provided, and start
there. This clashes with the current solutions, as they are based on the fact
that we can use dev->of_node when present.

My guess at this point, if we're forced to honor that behaviour, is that we
have to create a new API for the PCI use case. Something the likes of
of_dma_configure_parent().

Regards,
Nicolas

Attachment: signature.asc
Description: This is a digitally signed message part