Hi Rene,
On Fri, 2010-11-05 at 15:07 -0600, Sapiens, Rene wrote:
> Hi Ionut,
>
> On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu <ionut.nicu@xxxxxxxxx> wrote:
> > Convert the core module of the tidspbridge driver
> > to use struct list_head instead of struct lst_list.
> >
>
> <snip>
>
> > if (!status) {
> > /* Get a free chirp: */
> > - chnl_packet_obj =
> > - (struct chnl_irp *)lst_get_head(pchnl->free_packets_list);
> > - if (chnl_packet_obj == NULL)
> > + if (!list_empty(&pchnl->free_packets_list)) {
> > + chnl_packet_obj = list_first_entry(
> > + &pchnl->free_packets_list,
> > + struct chnl_irp, link);
> > + list_del(&chnl_packet_obj->link);
> > + } else
> > status = -EIO;
>
> What do you think if we close the braces, since the first conditional
> has more than one statement?
>
Can you clarify? I don't think I understand which brace we need to close
here.
> <snip>
>
> > @@ -286,18 +286,16 @@ int bridge_chnl_cancel_io(struct chnl_object *chnl_obj)
> > }
> > }
> > /* Move all IOR's to IOC queue: */
> > - while (!LST_IS_EMPTY(pchnl->pio_requests)) {
> > - chnl_packet_obj =
> > - (struct chnl_irp *)lst_get_head(pchnl->pio_requests);
> > - if (chnl_packet_obj) {
> > - chnl_packet_obj->byte_size = 0;
> > - chnl_packet_obj->status |= CHNL_IOCSTATCANCEL;
> > - lst_put_tail(pchnl->pio_completions,
> > - (struct list_head *)chnl_packet_obj);
> > - pchnl->cio_cs++;
> > - pchnl->cio_reqs--;
> > - DBC_ASSERT(pchnl->cio_reqs >= 0);
> > - }
> > + while (!list_empty(&pchnl->pio_requests)) {
> > + chnl_packet_obj = list_first_entry(&pchnl->pio_requests,
> > + struct chnl_irp, link);
> > + list_del(&chnl_packet_obj->link);
> > + chnl_packet_obj->byte_size = 0;
> > + chnl_packet_obj->status |= CHNL_IOCSTATCANCEL;
> > + list_add_tail(&chnl_packet_obj->link, &pchnl->pio_completions);
> > + pchnl->cio_cs++;
> > + pchnl->cio_reqs--;
> > + DBC_ASSERT(pchnl->cio_reqs >= 0);
>
> Why don't we use list_for_each_entry_safe() instead?
>
Agreed, it will look better.
> > }
> > func_cont:
> > spin_unlock_bh(&chnl_mgr_obj->chnl_mgr_lock);
>
> <snip>
>
> > @@ -818,9 +804,19 @@ int bridge_chnl_open(struct chnl_object **chnl,
> > /* Protect queues from io_dpc: */
> > pchnl->dw_state = CHNL_STATECANCEL;
> > /* Allocate initial IOR and IOC queues: */
> > - pchnl->free_packets_list = create_chirp_list(pattrs->uio_reqs);
> > - pchnl->pio_requests = create_chirp_list(0);
> > - pchnl->pio_completions = create_chirp_list(0);
> > + status = create_chirp_list(&pchnl->free_packets_list,
> > + pattrs->uio_reqs);
> > + if (status)
> > + goto func_end;
> > +
> > + status = create_chirp_list(&pchnl->pio_requests, 0);
> > + if (status)
> > + goto func_end;
> > +
> > + status = create_chirp_list(&pchnl->pio_completions, 0);
> > + if (status)
> > + goto func_end;
> > +
>
> With these goto you are not freeing the memory allocated for pchnl, please free
> it at func_end.
>
Thanks for catching this. Freeing it at func_end is not a very good
idea, because it's also freed above.
What do you think if I replace the last two calls for
create_chirp_list() with just INIT_LIST_HEAD()? This way we'll have only
one error check where we'll also kfree(pchnl) and we'll have 2 less
un-necessary function calls / error checks.
Regards,
Ionut.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
