[PATCH v3 5/5] OMAP: mailbox: add notification support for multiple readers

[Kanigeri, Hari]

Felipe,

>
>> I think handling of per-message callback should be handled at one
>> level above mailbox, like in IPC modules such as dspbridge,
>> syslink..etc.
>
> then you'll have duplication of functionality :-)
>
yes in mailbox :). One solution doesn't fit all and this should be
handled at IPC driver.

>> Mailbox module should be free of any protocols and should take care of
>
> it's not a protocol that I'm proposing. It's just one way to give
> mailbox users a more async API.
>
>> just writing to the mailbox fifo and delivering the message to the
>> interested listener(s).
>
> exactly. Only to the interested listener. With your patch, you'll be
> delivering the message to all listeners and listeners have to check if
> that particular message belongs to them.
>
>>> What will happen is that every user will have to check if every message
>>> belongs to them based on the notifications.
>>>
>>> Imagine a hypothetical situation where you have 100 users each with 100
>>> messages. You will have 100 * 100 (10.000)
>>> "does-this-message-belongs-to-me" checks.
>>>
>>
>> Ideally one shouldn't design their IPC this way. They should have a
>> IPC driver and the knowledge of notification to multiple users should
>> be routed from there.
>
> only if the message is interesting for N listeners. But if each
> listeners will have its own message towards the other side of the link,
> then mailbox should only give the result to one listener.
>
>> What my patch is addressing is ensure that the users of mailbox don't
>> mess with the mailbox's internal pointer, and also provide the
>> flexibility of multiple listeners. Designers of IPC should make their
>> own judgment whether to use the flexibility option based on their use
>> cases.
> but with your patch, you are delivering the same message to all
> listeners. You give the oportunity even for some attacks. I could fiddle
> with other listeners' message just by attaching a notifier_block and
> checking what's the content of every message.

IMHO, an kernel API have a scope to be misused if the Users misuse it,
and I think the users of the Kernel APIs should know what they are
doing before using an API.

>
>>> Rather than doing it this way, I would, as the easiest path, add a
>>> "callback" parameter to omap_mbox_request_send() and then, internally,
>>> allocate a e.g. struct omap_mbox_request and add that to a list of
>>> pending
>>> messages. Something like:
>>>
>>> struct omap_mbox_request {
>>>        struct omap_mbox        *mbox;
>>>        int                     (*complete)(void *context);
>>>        void                    *context;
>>>        void                    *buf;
>>>        unsigned                len;
>>>        struct list_head        list;
>>> };
>>>
>>> [...]
>>>
>>> int omap_mbox_request_send(struct omap_mbox *mbox, mbox_msg_t msg, int
>>>                (*complete)(void *context), void *context, gfp_t flags)
>>> {
>>>        struct omap_mbox_queue  *mq = mbox->txq;
>>>        struct omap_mbox_request        *req;
>>>        int                     ret = 0;
>>>        int                     len;
>>>
>>>        req = kzalloc(sizeof(*req), flags);
>>>        if (!req) {
>>>                [...]
>>>        }
>>>
>>>        memcpy(req->buf, &msg, sizeof(msg));
>>>        req->len = sizeof(msg));
>>>        req->mbox = msg;
>>>        req->complete = complete;
>>>        req->context = context;
>>>        INIT_LIST_HEAD(&req->list);
>>>
>>>        list_add_tail(&req->list, &mq->req_list);
>>>
>>>        /* kick the tasklet here */
>>>
>>>        return 0;
>>> }
>>>
>>> then on your tasklet, you simply iterate over the list and start sending
>>> one by one and calling callback when it completes. You would be giving

How do you know that a response is received for a particular sender ?
By reading mailbox payload or by reading some shared memory ? I think
this itself would constitute building up protocol in mailbox driver.

>>> your users a more asynchronous API and you wouldn't need this notifier
>>> which, IMO, isn't a good solution at all.
>>>
>>
>> Your ideas are good. But this type of intelligence is already inbuilt
>> in IPC drivers such as dspbridge and syslink. And I think mailbox
>> driver should be free of protocols.
>
> Like I said before, this is not a protocol. And if both dspbridge and
> syslink have the same kind of thing, don't you think it's a duplication?
> Don't you, also, think common features should be done at framework
> levels ? Since we don't have an IPC framework, we consider the mailbox
> driver a framework for using the OMAP mailbox, then the API I proposed
> is just one way to address the problem you described. There's nothing
> like a protocol here.

I fully agree with you about common framework, and the proposed
solution for the common IPC framework is Syslink. This was discussed
during the recent LPC meeting to come up with a common IPC framework
that is currently missing in Kernel.
I can share with you the details if you are interested.

>
> In any case, you are the one taking care of those drivers and it's your
> call how to handle multiple users, but keep in mind you'll be allowing
> anyone to see all messages that are going through mailbox just by
> attaching a notifier block :-) If that's ok for you, if you don't see
> any troubles with that, go for it. If you also don't see any troubles
> with having hundreds of users each with hundreds of messages and you
> notifying all of them about all messages as a problem, go for it :-p

As I said if some one does that then it is a misuse on their part :).
The messages should be routed through IPC driver. And let's not forget
the main motivation of this patch as well.


Thank you,
Best regards,
Hari Kanigeri