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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140216163639.GB28616@kroah.com>
Date:	Sun, 16 Feb 2014 08:36:39 -0800
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Jassi Brar <jassisinghbrar@...il.com>
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

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