Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Feb 16, 2014 at 12:06:44PM +0530, Jassi Brar wrote:
> >> +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.

That's fine, then just "pre-declare" the structure you are going to be
using/calling it in the public header files:
	struct foo;
and then use that, not a void *, which is horrible.  We have a typesafe
language, use it :)

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

Then call it something like "client_data", as that's more like what it
is, right?

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

See above for how to fix that.

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

Then make it a structure, not a typedef please.

thanks,

greg k-h