Re: [PATCH v3 09/13] device core: Introduce multiple dma pfn offsets

[Nicolas Saenz Julienne <nsaenzjulienne@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 9715 bytes --]

Hi Jim,

On Thu, 2020-06-04 at 10:35 -0400, Jim Quinlan wrote:

[...]

> > > --- a/arch/sh/kernel/dma-coherent.c
> > > +++ b/arch/sh/kernel/dma-coherent.c
> > > @@ -14,6 +14,8 @@ void *arch_dma_alloc(struct device *dev, size_t size,
> > > dma_addr_t *dma_handle,
> > >  {
> > >       void *ret, *ret_nocache;
> > >       int order = get_order(size);
> > > +     unsigned long pfn;
> > > +     phys_addr_t phys;
> > > 
> > >       gfp |= __GFP_ZERO;
> > > 
> > > @@ -34,11 +36,14 @@ void *arch_dma_alloc(struct device *dev, size_t size,
> > > dma_addr_t *dma_handle,
> > >               return NULL;
> > >       }
> > > 
> > > -     split_page(pfn_to_page(virt_to_phys(ret) >> PAGE_SHIFT), order);
> > > +     phys = virt_to_phys(ret);
> > > +     pfn =  phys >> PAGE_SHIFT;
> > 
> > nit: not sure it really pays off to have a pfn variable here.
> Did it for readability; the compiler's optimization should take care
> of any extra variables.  But I can switch if you insist.

No need.

[...]

> > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > index 055eb0b8e396..2d66d415b6c3 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > @@ -898,7 +898,10 @@ static int sun6i_csi_probe(struct platform_device
> > > *pdev)
> > > 
> > >       sdev->dev = &pdev->dev;
> > >       /* The DMA bus has the memory mapped at 0 */
> > > -     sdev->dev->dma_pfn_offset = PHYS_OFFSET >> PAGE_SHIFT;
> > > +     ret = attach_uniform_dma_pfn_offset(sdev->dev,
> > > +                                         PHYS_OFFSET >> PAGE_SHIFT);
> > > +     if (ret)
> > > +             return ret;
> > > 
> > >       ret = sun6i_csi_resource_request(sdev, pdev);
> > >       if (ret)
> > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > index 96d8cfb14a60..c89333b0a5fb 100644
> > > --- a/drivers/of/address.c
> > > +++ b/drivers/of/address.c
> > > @@ -918,6 +918,70 @@ void __iomem *of_io_request_and_map(struct
> > > device_node
> > > *np, int index,
> > >  }
> > >  EXPORT_SYMBOL(of_io_request_and_map);
> > > 
> > > +static int attach_dma_pfn_offset_map(struct device *dev,
> > > +                                  struct device_node *node, int
> > > num_ranges)
> > 
> > As with the previous review, please take this comment with a grain of salt.
> > 
> > I think there should be a clear split between what belongs to OF and what
> > belongs to the core device infrastructure.
> > 
> > OF's job should be to parse DT and provide a list/array of ranges, whereas
> > the
> > core device infrastructure should provide an API to assign a list of
> > ranges/offset to a device.
> > 
> > As a concrete example, you're forcing devices like the sta2x11 to build with
> > OF
> > support, which, being an Intel device, it's pretty odd. But I'm also
> > thinking
> > of how will all this fit once an ACPI device wants to use it.
> To fix this I only have to move attach_uniform_dma_pfn_offset() from
> of/address.c to say include/linux/dma-mapping.h.  It has no
> dependencies on OF.  Do you agree?

Yes that seems nicer. In case you didn't had it in mind already, I'd change the
function name to match the naming scheme they use there.

On the other hand, I'd also move the non OF parts of the non unifom dma_offset
version of the function there.

> > Expanding on this idea, once you have a split between the OF's and device
> > core
> > roles, it transpires that of_dma_get_range()'s job should only be to provide
> > the ranges in a device understandable structure and of_dma_configre()'s to
> > actually assign the device's parameters. This would obsolete patch #7.
> 
> I think you mean patch #8.

Yes, my bad.

> I agree with you.  The reason I needed a "struct device *"  in the call is
> because I wanted to make sure the memory that is alloc'd belongs to the
> device that needs it.  If I do a regular kzalloc(), this memory  will become
> a leak once someone starts unbinding/binding their device.  Also, in  all
> uses of of_dma_rtange() -- there is only one --  a dev is required as one
> can't attach an offset map to NULL.
> 
> I do see that there are a number of functions in drivers/of/*.c that
> take 'struct device *dev' as an argument so there is precedent for
> something like this.  Regardless, I need an owner to the memory I
> alloc().

I understand the need for dev to be around, devm_*() is key. But also it's
important to keep the functions on purpose. And if of_dma_get_range() starts
setting ranges it calls, for the very least, for a function rename. Although
I'd rather split the parsing and setting of ranges as mentioned earlier. That
said, I get that's a more drastic move.

Talking about drastic moves. How about getting rid of the concept of
dma_pfn_offset for drivers altogether. Let them provide dma_pfn_offset_regions
(even when there is only one). I feel it's conceptually nicer, as you'd be
dealing only in one currency, so to speak, and you'd centralize the bus DMA
ranges setter function which is always easier to maintain.

I'd go as far as not creating a special case for uniform offsets. Let just set
cpu_end and dma_end to -1 so we always get a match. It's slightly more compute
heavy, but I don't think it's worth the optimization.

Just my two cents :)

> > > +{
> > > +     struct of_range_parser parser;
> > > +     struct of_range range;
> > > +     struct dma_pfn_offset_region *r;
> > > +
> > > +     r = devm_kcalloc(dev, num_ranges + 1,
> > > +                      sizeof(struct dma_pfn_offset_region), GFP_KERNEL);
> > > +     if (!r)
> > > +             return -ENOMEM;
> > > +     dev->dma_pfn_offset_map = r;
> > > +     of_dma_range_parser_init(&parser, node);
> > > +
> > > +     /*
> > > +      * Record all info for DMA ranges array.  We could
> > > +      * just use the of_range struct, but if we did that it
> > > +      * would require more calculations for phys_to_dma and
> > > +      * dma_to_phys conversions.
> > > +      */
> > > +     for_each_of_range(&parser, &range) {
> > > +             r->cpu_start = range.cpu_addr;
> > > +             r->cpu_end = r->cpu_start + range.size - 1;
> > > +             r->dma_start = range.bus_addr;
> > > +             r->dma_end = r->dma_start + range.size - 1;
> > > +             r->pfn_offset = PFN_DOWN(range.cpu_addr)
> > > +                     - PFN_DOWN(range.bus_addr);
> > > +             r++;
> > > +     }
> > > +     return 0;
> > > +}
> > > +
> > > +
> > > +
> > > +/**
> > > + * attach_dma_pfn_offset - Assign scalar offset for all addresses.
> > > + * @dev:     device pointer; only needed for a corner case.
> > 
> > That's a huge corner :P
> Good point; I'm not really sure what percent of Linux configurations
> require a dma_pfn_offset.  I'll drop the "corner".
> 
> > > + * @dma_pfn_offset:  offset to apply when converting from phys addr
> > > + *                   to dma addr and vice versa.
> > > + *
> > > + * It returns -ENOMEM if out of memory, otherwise 0.
> > > + */
> > > +int attach_uniform_dma_pfn_offset(struct device *dev, unsigned long
> > > pfn_offset)
> > 
> > As I say above, does this really belong to of/address.c?
> No it does not.  Will fix.
> 
> > > +{
> > > +     struct dma_pfn_offset_region *r;
> > > +
> > > +     if (!dev)
> > > +             return -ENODEV;
> > > +
> > > +     if (!pfn_offset)
> > > +             return 0;
> > > +
> > > +     r = devm_kcalloc(dev, 1, sizeof(struct dma_pfn_offset_region),
> > > +                      GFP_KERNEL);
> > > +     if (!r)
> > > +             return -ENOMEM;
> > 
> > I think you're missing this:
> > 
> >         dev->dma_pfn_offset_map = r;
> > 
> That's a showstopper!  DanC also pointed it out but I still didn't see
> it.  Thanks!
> 
> > > +
> > > +     r->uniform_offset = true;
> > > +     r->pfn_offset = pfn_offset;
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(attach_uniform_dma_pfn_offset);
> > > +
> > >  /**
> > >   * of_dma_get_range - Get DMA range info
> > >   * @dev:     device pointer; only needed for a corner case.
> > > @@ -933,7 +997,7 @@ EXPORT_SYMBOL(of_io_request_and_map);
> > >   *   CPU addr (phys_addr_t)  : pna cells
> > >   *   size                    : nsize cells
> > >   *
> > > - * It returns -ENODEV if "dma-ranges" property was not found
> > > + * It returns -ENODEV if !dev or "dma-ranges" property was not found
> > >   * for this device in DT.
> > >   */
> > >  int of_dma_get_range(struct device *dev, struct device_node *np, u64
> > > *dma_addr,
> > > @@ -946,7 +1010,13 @@ int of_dma_get_range(struct device *dev, struct
> > > device_node *np, u64 *dma_addr,
> > >       bool found_dma_ranges = false;
> > >       struct of_range_parser parser;
> > >       struct of_range range;
> > > +     phys_addr_t cpu_start = ~(phys_addr_t)0;
> > >       u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
> > > +     bool dma_multi_pfn_offset = false;
> > > +     int num_ranges = 0;
> > > +
> > > +     if (!dev)
> > > +             return -ENODEV;
> > 
> > Shouldn't this be part of patch #7?
> Do you mean #8?  Do you mean the test for !dev?   It is not required
> for #8 so I thought I'd keep these two changes separate.  I could
> squash them.

#8, of course.

It's more of a subjective matter, but to me it fits better #8's description and
keeps this one more focused. That said, it's just a comment, do as you please.

Regads,
Nicolas


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel