lkml.org 
[lkml]   [2014]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCHv4,2/5] mailbox: Introduce framework for mailbox
    From
    On Sat, Mar 29, 2014 at 3:38 AM, Markus Mayer <markus.mayer@linaro.org> wrote:

    .....

    >> +int mbox_send_message(struct mbox_chan *chan, void *mssg)
    >> +{
    >> + int t;
    >> +
    >> + if (!chan || !chan->cl)
    >> + return -EINVAL;
    >> +
    >> + t = _add_to_rbuf(chan, mssg);
    >> + if (t < 0) {
    >> + pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
    >> + return t;
    >> + }
    >> +
    >> + _msg_submit(chan);
    >> +
    >> + if (chan->txdone_method == TXDONE_BY_POLL)
    >> + poll_txdone((unsigned long)chan->con);
    >
    > Wouldn't it be cleaner to use
    > poll_txdone((unsigned long)&chan->con);
    > ?
    >
    Here's how we use it ...

    static void poll_txdone(unsigned long data)
    {
    struct mbox_con *con = (struct mbox_con *)data;
    .....
    }

    To me, unnecessarily passing a pointer to a pointer seems unclean.


    >> +int mbox_controller_register(struct mbox_controller *mbox)
    >> +{
    >> + int i, num_links, txdone;
    >> + struct mbox_chan *chan;
    >> + struct mbox_con *con;
    >> +
    >> + /* Sanity check */
    >> + if (!mbox || !mbox->ops)
    >> + return -EINVAL;
    >> +
    >> + for (i = 0; mbox->links[i]; i++)
    >> + ;
    >> + if (!i)
    >> + return -EINVAL;
    >> + num_links = i;
    >> +
    >> + mutex_lock(&con_mutex);
    >> + /* Check if already populated */
    >> + list_for_each_entry(con, &mbox_cons, node)
    >> + if (!strcmp(mbox->controller_name, con->name)) {
    >> + mutex_unlock(&con_mutex);
    >> + return -EINVAL;
    >> + }
    >> +
    >> + con = kzalloc(sizeof(struct mbox_con), GFP_KERNEL);
    >> + if (!con)
    >> + return -ENOMEM;
    >
    > The mutex is not freed here.
    >
    >> +
    >> + chan = kzalloc(sizeof(struct mbox_chan) * num_links, GFP_KERNEL);
    >> + if (!chan) {
    >> + kfree(con);
    >> + return -ENOMEM;
    >
    > Again, the mutex is not freed.
    >
    > You could move both allocations above the mutex. Then you won't have to
    > worry about it.
    >
    Yes thanks. I overlooked. Will fix it.

    Regards,
    -Jassi


    \
     
     \ /
      Last update: 2014-03-29 05:41    [W:3.250 / U:0.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site