lkml.org 
[lkml]   [2013]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/4] staging: ozwpan: Increment port number for new device.
On 05/08/13 21:23, Dan Carpenter wrote:
> On Mon, Aug 05, 2013 at 07:43:16PM +0100, Rupesh Gujare wrote:
>> On 05/08/13 18:53, Dan Carpenter wrote:
>>> On Mon, Aug 05, 2013 at 06:40:13PM +0100, Rupesh Gujare wrote:
>>>> This patch fixes crash issue when there is quick cycle of
>>>> de-enumeration & enumeration due to loss of wireless link.
>>>>
>>>> It is found that sometimes new device (or coming back device)
>>>> returns very fast, even before USB core read out hub status,
>>>> resulting in allocation of same port, which results in unstable
>>>> system & crash.
>>>>
>>>> Above issue is resolved by making sure that we always assign
>>>> new port to new device, making sure that USB core reads correct
>>>> hub status.
>>>>
>>> This feels like papering over the problem. Surely the real fix
>>> would be to improve the reference counting.
>>>
>>> This patch is probably effective but it makes the code more subtle
>>> and it shows that we don't really understand what we are doing with
>>> regards to reference counting.
>>>
>>>
>> Probably this is easier way to fix issue, since we don't have
>> reference count for ports & we rely on flags to check port status.
>> Any suggestions are appreciated.
> To be honest, I wish someone would just go through and make this
> look more like kernel style. It's very ugly to look at. Even a
> very cursory patch series would make a big difference:
>
> [patch 1/6] Add a blank line between declaractions and code.
> [patch 2/6] Add a blank line between functions
> [patch 3/6] Make oz_hcd_pd_arrived() return a struct pointer (instead of a void pointer)
> [patch 4/6] Make oz_hcd_pd_departed() take a struct pointer
> [patch 5/6] Swap arguments of oz_ep_alloc() to match kmalloc()
> [patch 6/6] Remove unneeded initializers
>
> Also it's better to separate the success path from the failure path
> because it means fewer intend levels. The way oz_hcd_pd_arrived()
> looks now it's easy to think we free "ep" but actually we do this
> spaghetti thing of setting it to NULL on success. This function
> should just be:
>
> frob();
> frob();
> ret = frob();
> if (ret)
> goto err_put;
> frob();
> frob();
> ret = frob();
> if (ret)
> goto err_free_ep;
> frob();
> frob();
> put();
> return hport;
>
> err_free_ep:
> free_ep();
> err_put:
> put();
> return NULL;
>
> But instead it is:
>
> frob();
> ret = frob();
> if (ret) {
> unlock();
> goto out;
> }
> frob();
> ret = frob();
> if (ret success) {
> frob();
> frob();
> ep = NULL;
> frob();
> unlock();
> frob();
> } else {
> unlock();
> }
> out:
> if (ep)
> free_ep();
> put();
> return something;
>
> In the second example most of the code is indented. It's so hard
> to read because there are unlocks scattered throughout. Meanwhile,
> if you separate success and failure then there are only two unlocks,
> one for success and one for failure.
>
> In the current code you have to set "ep" to NULL on the success path
> and then test it and or free it. If you separate them out then it's
> obvious that "ep" is not freed on success.
>
> If you separate them out then it's clear that we return NULL on
> failure. In the current code you have to scroll back to the start
> of the function.
>
> Obviously it's not an emergency to fix any of these style issues but
> it will need to be addressed eventually before it moves out of
> staging. I think as well that just cleaning things up helps to
> fix bugs.
>

Thank you Dan, really appreciate your comments. Your suggestions sounds
perfectly well.
I will work on it, once this patch series is applied to staging tree.

I am assuming that you have no objection for it, & I will follow up with
above style nits in follow on patches.

--
Regards,
Rupesh Gujare



\
 
 \ /
  Last update: 2013-08-06 12:41    [W:0.045 / U:0.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site