Hi Ionut,
On Sat, Nov 6, 2010 at 11:21 AM, Ionut Nicu <ionut.nicu@xxxxxxxxxx> wrote:
> 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.
I mean open and close the braces for the added 'else', I'm just mentioning
what the coding style says for placing braces when one of the branches
has more than one statement.
>
>> <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.
>
Agreed, this looks good.
>
Regards,
Rene
--
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
