Date:	Sun, 16 Feb 2014 12:06:44 +0530
From:	Jassi Brar <jassisinghbrar@...il.com>
To:	Greg KH <gregkh@...uxfoundation.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Suman Anna <s-anna@...com>, Tony Lindgren <tony@...mide.com>,
	"Omar Ramirez Luna (omar.ramirez@...itl.com)" 
	<omar.ramirez@...itl.com>, Loic Pallardy <loic.pallardy@...com>,
	LeyFoon Tan <lftan.linux@...il.com>,
	Craig McGeachie <slapdau@...oo.com.au>,
	courtney.cavin@...ymobile.com,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Jassi Brar <jaswinder.singh@...aro.org>
Subject: Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[merging replies in one post]

On Sun, Feb 16, 2014 at 12:45 AM, Greg KH <gregkh@...uxfoundation.org> wrote:
> On Sat, Feb 15, 2014 at 11:55:27PM +0530, Jassi Brar wrote:
>> +/*
>> + * Call for IPC controller drivers to register a controller, adding
>> + * its channels/mailboxes to the global pool.
>> + */
>> +int ipc_links_register(struct ipc_controller *ipc)
>> +{
>> +     int i, num_links, txdone;
>> +     struct ipc_chan *chan;
>> +     struct ipc_con *con;
>> +
>> +     /* Sanity check */
>> +     if (!ipc || !ipc->ops)
>> +             return -EINVAL;
>> +
>> +     for (i = 0; ipc->links[i]; i++)
>> +             ;
>> +     if (!i)
>> +             return -EINVAL;
>
> So you have to have links?  You should document this in the function
> definition.  Actually, why no kerneldoc for the public functions?
>
Yes a controller registers a bunch of links/mailboxes that can be used
by client drivers to send/recv messages. I'll add kerneldoc for the
api as well.

>> +     num_links = i;
>> +
>> +     mutex_lock(&con_mutex);
>> +     /* Check if already populated */
>> +     list_for_each_entry(con, &ipc_cons, node)
>> +             if (!strcmp(ipc->controller_name, con->name)) {
>> +                     mutex_unlock(&con_mutex);
>> +                     return -EINVAL;
>> +             }
>> +     mutex_unlock(&con_mutex);
>
> Why drop the lock here?  Shouldn't you grab it for the whole function,
> as this could race if two callers want to register the same name.
>
Yes, thanks.

>> +     con = kzalloc(sizeof(*con) + sizeof(*chan) * num_links, GFP_KERNEL);
>
> Are you ok with structures on unaligned boundries?  That might really
> slow down some processors if your pointers are unaligned...
>
OK, I'll align allocation.

>> +     if (!con)
>> +             return -ENOMEM;
>> +
>> +     INIT_LIST_HEAD(&con->channels);
>> +     snprintf(con->name, 16, "%s", ipc->controller_name);
>
> Magic name size :(
>
Yeah I know. I tried to keep the implementation simple.

>> +
>> +     if (ipc->txdone_irq)
>> +             txdone = TXDONE_BY_IRQ;
>> +     else if (ipc->txdone_poll)
>> +             txdone = TXDONE_BY_POLL;
>> +     else /* It has to be ACK then */
>> +             txdone = TXDONE_BY_ACK;
>> +
>> +     if (txdone == TXDONE_BY_POLL) {
>> +             con->period = ipc->txpoll_period;
>> +             con->poll.function = &poll_txdone;
>> +             con->poll.data = (unsigned long)con;
>> +             init_timer(&con->poll);
>> +     }
>> +
>> +     chan = (void *)con + sizeof(*con);
>> +     for (i = 0; i < num_links; i++) {
>> +             chan[i].con = con;
>> +             chan[i].assigned = false;
>> +             chan[i].link_ops = ipc->ops;
>> +             chan[i].link = ipc->links[i];
>> +             chan[i].txdone_method = txdone;
>> +             chan[i].link->api_priv = &chan[i];
>> +             spin_lock_init(&chan[i].lock);
>> +             BLOCKING_INIT_NOTIFIER_HEAD(&chan[i].avail);
>> +             list_add_tail(&chan[i].node, &con->channels);
>> +             snprintf(chan[i].name, 16, "%s", ipc->links[i]->link_name);
>
> Magic name size :(
>
"Controller:Link" specify the 32byte identity of a link.
I haven't yet opened the can of worms that is generic naming/identity
scheme like DMAEngine. Because the local controller and the remote f/w
together represents an entity that the local client driver deals
with.... so many common client drivers.are unlikely.

>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(ipc_links_register);
>> +
>> +void ipc_links_unregister(struct ipc_controller *ipc)
>> +{
>> +     struct ipc_con *t, *con = NULL;
>> +     struct ipc_chan *chan;
>> +
>> +     mutex_lock(&con_mutex);
>> +
>> +     list_for_each_entry(t, &ipc_cons, node)
>> +             if (!strcmp(ipc->controller_name, t->name)) {
>> +                     con = t;
>> +                     break;
>> +             }
>> +
>> +     if (con)
>> +             list_del(&con->node);
>> +
>> +     mutex_unlock(&con_mutex);
>> +
>> +     if (!con)
>> +             return;
>> +
>> +     list_for_each_entry(chan, &con->channels, node)
>> +             ipc_free_channel((void *)chan);
>
> Why does this function take a void *?  Shouldn't it take a "real"
> structure pointer?
>
ipc_request/free_channel() is the api for client drivers. I have tried
to make the return channel opaque object to the clients and yet be
able to reuse the object within the api implementation. For that
reason we have mailbox_client.h and mailbox_controller.h so no side
can abuse what's on the other side. Only the api(mailbox.c) includes
both the headers.

>> +
>> +     del_timer_sync(&con->poll);
>> +
>> +     kfree(con);
>> +}
>> +EXPORT_SYMBOL(ipc_links_unregister);
>
>> +struct ipc_client {
>> +     char *chan_name;
>> +     void *cl_id;
>
> Why a void *?  Can't you have a "real" type here?
>
That is for client driver to specify how the controller driver is to
behave .... the api simply passes it on to the underlying controller
driver. We couldn't have defined some global real type because the
same controller behaves differently if the remote f/w changes.

>> +     void (*rxcb)(void *cl_id, void *mssg);
>> +     void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
>> +     bool tx_block;
>> +     unsigned long tx_tout;
>> +     bool knows_txdone;
>> +     void *link_data;
>> +};
>> +
>> +/**
>> + * The Client specifies its requirements and capabilities while asking for
>> + * a channel/mailbox by name. It can't be called from atomic context.
>> + * The channel is exclusively allocated and can't be used by another
>> + * client before the owner calls ipc_free_channel.
>> + */
>> +void *ipc_request_channel(struct ipc_client *cl);
>
> Can't you return a real type, and use it everywhere?  That's much
> "safer" and nicer.  This isn't other operating systems that have void *
> everywhere and handles, we have real types in Linux :)
>
As I said, we don't want the client driver to interpret the channel it
has been assigned. For clients a channel assigned is just an opaque
token that can't be used anyway other than request/release the channel
from the api.

> Did you forget a Kconfig file here?  How can this code be built?
Yup, sorry. The patch somehow got that change dropped.

>> +typedef unsigned request_token_t;
>
> Ick.  Why add a new typedef?  And if you do need this,
> drop the "_t" on the end please.
> Why not just rely on an unsigned int?  Or long?
> Do you really need a new type?
>
We can live without the typedef, it is only to impress that the cookie
returned is not to be used just like some unsigned... but an opaque object.

>> +EXPORT_SYMBOL(ipc_link_received_data);
>
> EXPORT_SYMBOL_GPL() on all of these perhaps?
>
Yes of course :)

Thanks,
Jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists