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

[Guzman Lugo, Fernando]

On Wed, Feb 23, 2011 at 1:54 PM, David Cohen <dacohen at gmail.com> 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

check kernel/notifier.c module and how it is manage in mailbox, so you
can implement something similar.

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

Sure, users needs to consider the callback as a ISR and if they need
to do something heavy then schedule a task to do that.

> In this context, callback and ISR cannot share a same pointer anymore.
>
>>
>> iommu <---- register fault callback for error notify driver
>>
>> instead of
>>
>> iommu <--- register fault callback for remote processor driver
>> <----register fault event for error notify driver.
>>
>> with that, we remove one dependency of the errornotify driver.
>
> I don't know very well the errornotify driver, but to bypass it seems
> be a good approach for me.

that driver is not upstream yet, the intention is to have a driver
which can handle many errors, at this moments it is not handle the
errors, but notifying them to userspace. it is thought to be a generic
error handler with the posibility of notify many errors, among them
the iommu fault, so if the error handler driver will notify iommy
fault errors it makes more sense to register with iommu module instead
of doing with other driver which uses iommu module, and that can be
done having multiple callbacks.

>
>>
>> Moreover, the iommu code support serveral users of the same hw iommu,
>> and it does not make sense for me, that you can register only one
>> callback, or if other user register its callback the previous one will
>> be overwritten.
>
> It's quite complex and dangerous this situation, as one driver can
> crash another one.

Yes that can happen, and if you don't have multiple callbacks the
other driver wont even notice it crashed and will try to work
normally. That is not good.

> But I don't think drivers have much choice.
>
> Hiroshi, do you have any different opinion for this subject? I can
> send a v4 version for this patch giving support for multiple callbacks
> per obj.
>
> Br,
>
> David
>
>>
>> Regards,
>> Fernando.
>>
>>>
>>> Br,
>>>
>>> David
>>>
>>>>
>>>> Regards,
>>>> Fernando.
>>>>
>>>
>>
>