Hi Ionut,
On Sun, Nov 7, 2010 at 7:39 AM, Ionut Nicu <ionut.nicu@xxxxxxxxxx> wrote:
> Hi Rene,
>
> On Fri, 2010-11-05 at 16:41 -0600, Sapiens, Rene wrote:
>> Hi Ionut,
>>
>> On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu <ionut.nicu@xxxxxxxxx> wrote:
>> > Convert the pmgr module of the tidspbridge driver
>> > to use struct list_head instead of struct lst_list.
>>
>> <snip>
>>
>> > + * Memory is coalesced back to the appropriate heap when a buffer is
>>
>> What is being fixed here?
>>
>
> It was a typo (s/coelesced/coalesced/).
Ok, thanks
>
>> > * freed.
>> > *
>> > * Notes:
>>
>> <snip>
>>
>> > @@ -833,67 +768,44 @@ static void add_to_free_list(struct cmm_allocator *allocator,
>> > DBC_REQUIRE(allocator != NULL);
>> > dw_this_pa = pnode->dw_pa;
>> > dw_next_pa = NEXT_PA(pnode);
>>
>> i think it would be good to return with error if !allocator or !pnode
>> and remove the resulting duplicated DBC_REQUIRE.
>>
>
> Yeah I think pnode should be checked for null. Can allocator ever be
> null?
It seems that it can actually be NULL, by a possible bug that i just
saw (will send the patch), but taking a deeper look it seems that if
allocator is NULL this function would never be called.
>
>> > - mnode_obj = (struct cmm_mnode *)lst_first(allocator->free_list_head);
>> > - while (mnode_obj) {
>> > + list_for_each_entry(mnode_obj, &allocator->free_list, link) {
>> > if (dw_this_pa == NEXT_PA(mnode_obj)) {
>>
>> <snip>
>>
>> > @@ -748,18 +736,16 @@ bool dev_init(void)
>> > */
>> > int dev_notify_clients(struct dev_object *hdev_obj, u32 ret)
>> > {
>> > - int status = 0;
>> > -
>> > struct dev_object *dev_obj = hdev_obj;
>> > - void *proc_obj;
>> > + struct list_head *curr;
>>
>> can we add a check for !dev_obj and !dev_obj->proc_list just to be
>> sure that we get always the correct pointer?
>>
>
> proc_list isn't a pointer. Can dev_obj ever be null?
I suppose that adding an extra check in all functions for the received
parameter would be redundant when those parameters come only from
our driver and were previously checked, also performance would be
affected; so relying on the having of the correct parameters in the path
that calls this function it seems that dev_notify_clients() is not called
with dev_obj with a NULL value.
>
>> <snip>
>>
>> > @@ -947,15 +933,17 @@ int dev_insert_proc_object(struct dev_object *hdev_obj,
>> > DBC_REQUIRE(refs > 0);
>> > DBC_REQUIRE(dev_obj);
>> > DBC_REQUIRE(proc_obj != 0);
>> > - DBC_REQUIRE(dev_obj->proc_list != NULL);
>> > DBC_REQUIRE(already_attached != NULL);
>>
>> can we check for !hdev_obj, !already_attached even if we have the
>> DBC_REQUIRE?, maybe we can actually remove the DBC_REQUIRE that could
>> be redundant after applying this.
>>
>
> Same question here.
The same comment.
>
>> > - if (!LST_IS_EMPTY(dev_obj->proc_list))
>> > + if (!list_empty(&dev_obj->proc_list))
>> > *already_attached = true;
>>
>> <snip>
>>
>> > @@ -986,15 +974,12 @@ int dev_remove_proc_object(struct dev_object *hdev_obj, u32 proc_obj)
>> >
>> > DBC_REQUIRE(dev_obj);
>> > DBC_REQUIRE(proc_obj != 0);
>> > - DBC_REQUIRE(dev_obj->proc_list != NULL);
>> > - DBC_REQUIRE(!LST_IS_EMPTY(dev_obj->proc_list));
>> > + DBC_REQUIRE(!list_empty(&dev_obj->proc_list));
>> >
>>
>> The same comment as above.
>>
>
> 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
