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

[Oleksandr <olekstysh@gmail.com>]

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@epam.com>
>>
>> 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@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel