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

[David Cohen]

On Wed, Feb 23, 2011 at 11:48 PM, Guzman Lugo, Fernando
<fernando.lugo at ti.com> wrote:
> 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.

Let's wait for Hiroshi's opinion and decide if I change or not the patches.

Br,

David

>
>>
>> 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
>>
>