Messages in this thread | | | Date | Sat, 29 Mar 2014 09:24:45 +0530 | Subject | Re: [PATCHv4,2/5] mailbox: Introduce framework for mailbox | From | Jassi Brar <> |
| |
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
| |