[PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling

[Guzman Lugo, Fernando]

On Wed, Feb 23, 2011 at 2:56 PM, Sakari Ailus
<sakari.ailus at maxwell.research.nokia.com> wrote:
> David Cohen wrote:
>> On Wed, Feb 23, 2011 at 3:39 PM, Guzman Lugo, Fernando
>> <fernando.lugo at ti.com> wrote:
>>> On Wed, Feb 23, 2011 at 3:45 AM, David Cohen <dacohen at gmail.com> wrote:
>>>> On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
>>>> <fernando.lugo at ti.com> wrote:
>>>>> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen at gmail.com> wrote:
>>>>>> Add support to register an isr for IOMMU fault situations and adapt it
>>>>>> to allow such (*isr)() to be used as fault callback. Drivers using IOMMU
>>>>>> module might want to be informed when errors happen in order to debug it
>>>>>> or react.
>>>>>>
>>>>>> Signed-off-by: David Cohen <dacohen at gmail.com>
>>>>>> ---
>>>>>>  arch/arm/mach-omap2/iommu2.c            |   17 +++++++++-
>>>>>>  arch/arm/plat-omap/include/plat/iommu.h |   14 ++++++++-
>>>>>>  arch/arm/plat-omap/iommu.c              |   52 ++++++++++++++++++++++---------
>>>>>>  3 files changed, 65 insertions(+), 18 deletions(-)
>>>>>>
>>>>> ....
>>>>>
>>>>>> @@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
>>>>>>  }
>>>>>>  EXPORT_SYMBOL_GPL(iommu_put);
>>>>>>
>>>>>> +int iommu_set_isr(const char *name,
>>>>>> +                 int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
>>>>>> +                            void *priv),
>>>>>> +                 void *isr_priv)
>>>>>> +{
>>>>>> +       struct device *dev;
>>>>>> +       struct iommu *obj;
>>>>>> +
>>>>>
>>>>> if the driver support multiple user for the same iommu why can only
>>>>> one callback be registered? should it support register multiple
>>>>> callback function (one per user)?
>>>>
>>>> Can you define a scenario for that?
>>>> On OMAP3 ISP the multiple users are the multiple ISP submodule, but I
>>>> don't think it's necessary all submodule to have a specific callback.
>>>> ISP core layer should handle.
>>>
>>> Hi,
>>>
>>> In OMAP4 the cortex M3 is a double core processor and as each core is
>>> running they own version of the RTOS we threat them independently. So
>>> our driver which controls the remote processor sees two processor but
>>> both use the same iommu hw. When a iommu fault happens, at this
>>> moment, it is consider as a faltal error and it is no managed to
>>> recover and continue, instead a restart of the processor is needed, if
>>> the fault happens in core0 we need to reset core1 too and vice versa.
>>> if the iommu would support several user callbacks, we can register the
>>> callback which resets core0 and also the callback which resets core1
>>> and treat them as totally independent processors. Also we have an
>>> error event notifier driver, which is only in charge of notifying
>>> error events to userspace, so we would have multiple callbacks we
>>> could do this
>>
>> I understood your point. In this case, I may not disagree about having
>> more than one callback per obj, although it doesn't seem a nice
>> scenario.
>> We can have a list of callbacks and call the entire list when a fault
>> happens. But it's necessary to pay attention it will happen in atomic
>> context and users should not abuse and register many callbacks. The
>> callback should *NOT* print useless messages and must verify the error
>> code to not execute useless steps.
>> In this context, callback and ISR cannot share a same pointer anymore.
>
> I think this is outside of the scope of the patch but...

yes, the same behaviour was before the patches, but as the patches are
changing the isr, I think it is a good time to modify, not in patch 2,
but in a new patch to be added to the serie between patch 1 and 2, so
that we dont need to change ISR part again after this set of patches.

>
> To efficiently debug iommu faults (with a driver using iommu page
> walking), besides the actual fault address the list of existing mappings
> and the information which driver created them and for which purpose is
> useful.
>
> The list of mappings is already available in the iommu structure. It'd
> be nice if there was a function a driver could call to print them.

>
> I can only think of ugly ways to implement the other.
>
> Just my 5 cents (as we have no 2 cent coins here).
>
> Regards,
>
> --
> Sakari Ailus
> sakari.ailus at maxwell.research.nokia.com
>