Re: [PATCH V2 2/3] iommu: of: Ignore all errors except EPROBE_DEFER

[Sricharan R]

Hi Laurent,

On 5/18/2017 7:13 PM, Laurent Pinchart wrote:
> Hi Sricharan
> 
> On Thursday 18 May 2017 19:08:12 Sricharan R wrote:
>> On 5/18/2017 6:00 PM, Laurent Pinchart wrote:
>>> On Thursday 18 May 2017 17:26:14 Sricharan R wrote:
>>>> On 5/18/2017 4:09 PM, Laurent Pinchart wrote:
>>>>> On Thursday 18 May 2017 15:37:09 Sricharan R wrote:
>>>>>> While deferring the probe of IOMMU masters,
>>>>>> xlate and add_device callback can pass back error values
>>>>>> like -ENODEV, which means IOMMU cannot be connected
>>>>>> with that master for real reasons. So rather than
>>>>>> killing the master's probe for such errors, just
>>>>>> ignore the errors and let the master work without
>>>>>> an IOMMU.
>>>>>
>>>>> I don't think this is a good idea. Why should we allow IOMMU drivers to
>>>>> return an error if we're always going to ignore the error value ? That
>>>>> will lead to drivers implementing slightly different behaviours, which
>>>>> will be painful the day we'll need to start acting based on the error
>>>>> value.
>>>>
>>>> The of_iommu_configure interface, before this series, was returning
>>>> either correct 'iommu_ops' or NULL. Also there was no return value from
>>>> of_dma_configure which calls of_iommu_configure. This means that if we
>>>> block only -ENODEV now and let the other errors, the probe of the master
>>>> devices can be killed for reasons apart from deferring. This would be a
>>>> change in behavior introduced. All of xlate, add_device, of_pci_map_rid
>>>> and others can return values apart from -ENODEV. So was thinking that
>>>> restoring the old behavior, except for returning EPROBE_DEFER was the
>>>> better thing to do ?
>>>
>>> We went from a situation where of_iommu_configure() could return either
>>> valid operations in the case the device was to be handled by the IOMMU or
>>> NULL otherwise, to a situation where we needed a third option for probe
>>> deferral. The way we've done this, through error pointers, allows lots of
>>> other errors to be returned as well from the of_xlate and add_device
>>> operations.
>>
>> right, this was difference in the behavior now.
>>
>>> There is currently no use for returning error codes other than
>>> -EPROBE_DEFFER from of_iommu_configure(), so your proposal is to block
>>> errors returned from the of_xlate and add_device operations inside
>>> of_iommu_configure(). My point is that, by doing so, we allow IOMMU
>>> drivers to return random error codes that are then ignored. I believe
>>> this can cause problems in the future when we will need to extend the API
>>> and standardize more error codes, as by then IOMMU drivers will return
>>> random errors (they actually do so already to some extent).
>>>
>>> For of_xlate I agree with you to some extent. v4.11 just checked whether
>>> of_xlate succeeded or not, and just didn't use the IOMMU in case it
>>> failed. The exact error value was ignored, and drivers already return
>>> random errors. Going back to the v4.11 behaviour is what we need to do in
>>> the short-term, even if I believe we should standardize the error values
>>> returned from of_xlate after v4.12.
>>>
>>> For add_device, however, the situation is a bit different. The add_device
>>> operation is called from the IOMMU bus notifier, and the -ENODEV error is
>>> ignored by add_iommu_group(). Any other error will cause bus_set_iommu()
>>> to fail, which makes IOMMU probing fail for the drivers that check the
>>> return value of bus_set_iommu() (some of them don't :-/).
>>>
>>> Fixing all this properly requires standardizing the error codes, and going
>>> through the existing IOMMU drivers to comply with the standardized
>>> behaviour.
>>
>> I understand your concern on standardizing the error codes from xlate,
>> add_device, others and handling them properly. As you said there are quite
>> some errors returned from them today. Also another thing is standardizing
>> the behavior of of_iommu_configure itself. So that API serves to connect a
>> device to its correct iommu_ops. When that's not possible, what should be
>> the output and how should that be handled by the caller. The current
>> behavior is to either 1) connect to correct ops or 2) wait for it or 3)
>> progress further with plain/default dma_ops. Anyways as you said
>> standardizing the iommu api ops, would make the of_iommu_configure handling
>> more specific. Having said that i think similar fix needs to be done for
>> acpi's iort_iommu_configure as well.
> 
> I'm less knowledgeable about ACPI but I think you're right. Would you like to 
> tackle this for v4.13 ? :-)

Will add the fix for ACPI now in this series. ok, Will really see if i can
address the standardizing part for 4.13.

> 
>>> While this shouldn't be very difficult, it's likely not material for a
>>> v4.12- rc fix. We will thus likely need to merge this patch (or something
>>> very similar to it), but I'd really like to see this fixed properly for
>>> v4.13.
>>
>> When you say "merge this patch (or something similar)", is that about
>> documenting the error values for of_xlate and add_device that you showed
>> down below  (or) about the patch in discussion ?
> 
> I meant the patch we're discussing, "[PATCH V2 2/3] iommu: of: Ignore all 
> errors except EPROBE_DEFER". Sorry for the confusion. One more comment about 
> this below.

ok.

> 
>>>>> At the very least, if you want to give a specific meaning to -ENODEV,
>>>>> you should check for that value specifically and not ignore all errors
>>>>> other than -EPROBE_DEFER. You also need to document the meaning of the
>>>>> error value. This can be done in the documentation of the of_xlate
>>>>> operation in include/linux/iommu.h:
>>>>>
>>>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>>>> index 2cb54adc4a33..6ba553e7384a 100644
>>>>> --- a/include/linux/iommu.h
>>>>> +++ b/include/linux/iommu.h
>>>>> @@ -181,7 +181,6 @@ struct iommu_resv_region {
>>>>>   * @domain_window_disable: Disable a particular window for a domain
>>>>>   * @domain_set_windows: Set the number of windows for a domain
>>>>>   * @domain_get_windows: Return the number of windows for a domain
>>>>> - * @of_xlate: add OF master IDs to iommu grouping
>>>>>   * @pgsize_bitmap: bitmap of all possible supported page sizes
>>>>>   */
>>>>>  struct iommu_ops {
>>>>> @@ -224,6 +223,11 @@ struct iommu_ops {
>>>>>  	/* Get the number of windows per domain */
>>>>>  	u32 (*domain_get_windows)(struct iommu_domain *domain);
>>>>>
>>>>> +	/**
>>>>> +	 * @of_xlate:
>>>>> +	 *
>>>>> +	 * Add OF master IDs to iommu grouping.
>>>>> +	 */
>>>>>  	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
>>>>>  	
>>>>>  	unsigned long pgsize_bitmap;
>>>>>
>>>>> And add documentation for the error codes there.
>>>>>
>>>>> If you want to ignore some errors returned from the add_device operation
>>>>> you should document it similarly, and in particular document which error
>>>>> check(s) need to be performed by of_xlate and which are the
>>>>> responsibility of add_device.
>>>>>
>>>>>> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with
>>>>>> deferred probing or error")
>>>>>> Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>>>>>> Tested-by: Magnus Damn <magnus.damn@xxxxxxxxx>
>>>>>> Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> [V2] Corrected spelling/case in commit log
>>>>>>
>>>>>>  drivers/iommu/of_iommu.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>>>>>> index e6e9bec..f0d22c0 100644
>>>>>> --- a/drivers/iommu/of_iommu.c
>>>>>> +++ b/drivers/iommu/of_iommu.c
>>>>>> @@ -237,6 +237,12 @@ const struct iommu_ops *of_iommu_configure(struct
>>>>>> device *dev,
>>>>>> 			ops = ERR_PTR(err);
>>>>>>  	}
>>>>>>
>>>>>> +	/* Ignore all other errors apart from EPROBE_DEFER */
>>>>>> +	if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
>>>>>> +		dev_info(dev, "Adding to IOMMU failed: %ld\n", 
> PTR_ERR(ops));
> 
> I would turn this into a dev_dbg(), as at least the -ENODEV error returned 
> from add_device has a defined meaning (see the comment in add_iommu_group()) 
> and is in my opinion not a condition that should be reported in the kernel log 
> by default.
> 

ok, will turn this in to a dev_dbg.

Regards,
 Sricharan


>>>>>> +		ops = NULL;
>>>>>> +	}
>>>>>> +
>>>>>>  	return ops;
>>>>>>  }
> 

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation