Re: [Xen-devel] [PATCH V3 2/8] iommu/arm: Add ability to handle deferred probing request

[Oleksandr <olekstysh@xxxxxxxxx>]

On 09.09.19 15:24, Julien Grall wrote:
> Hi Oleksandr,
Hi, Julien


> The code looks code, few comments below.
>
> On 8/20/19 7:09 PM, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > This patch adds minimal required support to General IOMMU framework
> > to be able to handle a case when IOMMU driver requesting deferred
> > probing for a device.
> >
> > In order not to pull Linux's error code (-EPROBE_DEFER) to Xen
> > we have chosen -EAGAIN to be used for indicating that device
> > probing is deferred.
> >
> > This is needed for the upcoming IPMMU driver which may request
> > deferred probing depending on what device will be probed the first
> > (there is some dependency between these devices, Root device must be
> > registered before Cache devices. If not the case, driver will deny
> > further Cache device probes until Root device is registered).
> > As we can't guarantee a fixed pre-defined order for the device nodes
> > in DT, we need to be ready for the situation where devices being
> > probed in "any" order.
> >
> > While here, order the headers alphabetically.
> It is common to clean code you modify in the same patch if they are 
> not complex, but this is not the case here... Indeed, the headers are 
> not touched. So I would prefer this to be in a separate patch unless 
> it breaks the compilation without it.
Well, will make this change in a separate patch.


> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > CC: Julien Grall <julien.grall@xxxxxxx>
> >
> > ---
> > Changes V2 -> V3:
> >      - removed deferred_probe field from struct dt_device_node,
> >        re-used domain_list instead
> >      - documented domain_list usage
> >      - added ASSERT to check that np->domain_list is empty
> >        before re-using it
> >      - put deferred_probe_list to init section
> >      - used more strict logic regarding processing devices in
> >        the deferred list
> >      - added more comments to code
> >      - put headers in alphabetical order
> > ---
> >   xen/drivers/passthrough/arm/iommu.c | 59 
> > ++++++++++++++++++++++++++++++++++---
> >   xen/include/asm-arm/device.h        |  6 +++-
> >   xen/include/xen/device_tree.h       |  7 +++++
> >   3 files changed, 67 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/iommu.c 
> > b/xen/drivers/passthrough/arm/iommu.c
> > index f219de9..72a30e0 100644
> > --- a/xen/drivers/passthrough/arm/iommu.c
> > +++ b/xen/drivers/passthrough/arm/iommu.c
> > @@ -15,11 +15,20 @@
> >    * GNU General Public License for more details.
> >    */
> >   -#include <xen/lib.h>
> > -#include <xen/iommu.h>
> >   #include <xen/device_tree.h>
> > +#include <xen/iommu.h>
> > +#include <xen/lib.h>
> > +
> >   #include <asm/device.h>
> >   +/*
> > + * Deferred probe list is used to keep track of devices for which 
> > driver
> > + * requested deferred probing (returned -EAGAIN).
> > + *
> > + * We re-use device's domain_list to link the device in the deferred 
> > list.
> > + */
> > +static __initdata LIST_HEAD(deferred_probe_list);
> > +
> >   static const struct iommu_ops *iommu_ops;
> >     const struct iommu_ops *iommu_get_ops(void)
> > @@ -42,7 +51,7 @@ void __init iommu_set_ops(const struct iommu_ops *ops)
> >     int __init iommu_hardware_setup(void)
> >   {
> > -    struct dt_device_node *np;
> > +    struct dt_device_node *np, *tmp;
> >       int rc;
> >       unsigned int num_iommus = 0;
> >   @@ -51,6 +60,17 @@ int __init iommu_hardware_setup(void)
> >           rc = device_init(np, DEVICE_IOMMU, NULL);
> >           if ( !rc )
> >               num_iommus++;
> > +        else if ( rc == -EAGAIN )
> > +        {
> > +            /* We expect nobody uses domain_list at such early 
> > stage. */
> AFAICT, this comment is only an English version of the next line. It 
> would be best if you explain why domain_list is re-used here.
Will do.


> > + ASSERT(list_empty(&np->domain_list));
> > +
> > +            /*
> > +             * Driver requested deferred probing, so add this device to
> > +             * the deferred list for further processing.
> > +             */
> > +            list_add(&np->domain_list, &deferred_probe_list);
> > +        }
> >           /*
> >            * Ignore the following error codes:
> >            *   - EBADF: Indicate the current not is not an IOMMU
> > @@ -61,7 +81,38 @@ int __init iommu_hardware_setup(void)
> >               return rc;
> >       }
> >   -    return ( num_iommus > 0 ) ? 0 : -ENODEV;
> > +    /* Return immediately if there are no initialized devices. */
> > +    if ( !num_iommus )
> > +        return ( list_empty(&deferred_probe_list) ) ? -ENODEV : 
> > -EAGAIN;
> NIT: Do you need the outer ()?
No, I don't.


--
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel