This patch series fixes crash issues observed,
& fix a bug in hub status code.
Rupesh Gujare (4):
staging: ozwpan: Fixes crash due to invalid port aceess.
staging: ozwpan: Increment port number for new device.
staging: ozwpan: Reset port configuration number.
staging: ozwpan: Return correct hub status.
drivers/staging/ozwpan/ozhcd.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
--
1.7.9.5
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.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index d313a63..a739986 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -127,6 +127,7 @@ struct oz_hcd {
struct list_head urb_cancel_list;
struct list_head orphanage;
int conn_port; /* Port that is currently connecting, -1 if none.*/
+ int last_port;
struct oz_port ports[OZ_NB_PORTS];
uint flags;
struct usb_hcd *hcd;
@@ -645,7 +646,9 @@ void *oz_hcd_pd_arrived(void *hpd)
goto out;
}
for (i = 0; i < OZ_NB_PORTS; i++) {
- struct oz_port *port = &ozhcd->ports[i];
+ struct oz_port *port = &ozhcd->ports[ozhcd->last_port++];
+ if (ozhcd->last_port >= OZ_NB_PORTS)
+ ozhcd->last_port = 0;
spin_lock(&port->port_lock);
if ((port->flags & OZ_PORT_F_PRESENT) == 0) {
oz_acquire_port(port, hpd);
@@ -655,13 +658,16 @@ void *oz_hcd_pd_arrived(void *hpd)
spin_unlock(&port->port_lock);
}
if (i < OZ_NB_PORTS) {
- oz_dbg(ON, "Setting conn_port = %d\n", i);
- ozhcd->conn_port = i;
+ if (!ozhcd->last_port)
+ ozhcd->conn_port = OZ_NB_PORTS - 1;
+ else
+ ozhcd->conn_port = ozhcd->last_port - 1;
+ oz_dbg(ON, "Setting conn_port = %d\n", ozhcd->conn_port);
/* Attach out endpoint 0.
*/
- ozhcd->ports[i].out_ep[0] = ep;
+ ozhcd->ports[ozhcd->conn_port].out_ep[0] = ep;
ep = NULL;
- hport = &ozhcd->ports[i];
+ hport = &ozhcd->ports[ozhcd->conn_port];
spin_unlock_bh(&ozhcd->hcd_lock);
if (ozhcd->flags & OZ_HDC_F_SUSPENDED) {
oz_dbg(ON, "Resuming root hub\n");
--
1.7.9.5
Fix a bug where we were not returning correct hub status
for 8th port.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index b060e43..2f93a00 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -1871,17 +1871,24 @@ static int oz_hcd_hub_status_data(struct usb_hcd *hcd, char *buf)
int i;
buf[0] = 0;
+ buf[1] = 0;
spin_lock_bh(&ozhcd->hcd_lock);
for (i = 0; i < OZ_NB_PORTS; i++) {
if (ozhcd->ports[i].flags & OZ_PORT_F_CHANGED) {
oz_dbg(HUB, "Port %d changed\n", i);
ozhcd->ports[i].flags &= ~OZ_PORT_F_CHANGED;
- buf[0] |= 1<<(i+1);
+ if (i < 7)
+ buf[0] |= 1 << (i+1);
+ else
+ buf[1] |= 1 << (i-7);
}
}
spin_unlock_bh(&ozhcd->hcd_lock);
- return buf[0] ? 1 : 0;
+ if (buf[0] != 0 || buf[1] != 0)
+ return 2;
+ else
+ return 0;
}
/*------------------------------------------------------------------------------
* Context: process
--
1.7.9.5
Make sure that we reset port configuration no. when PD departs.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index a739986..b060e43 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -721,6 +721,7 @@ void oz_hcd_pd_departed(void *hport)
hpd = port->hpd;
port->hpd = NULL;
port->bus_addr = 0xff;
+ port->config_num = 0;
port->flags &= ~(OZ_PORT_F_PRESENT | OZ_PORT_F_DYING);
port->flags |= OZ_PORT_F_CHANGED;
port->status &= ~USB_PORT_STAT_CONNECTION;
--
1.7.9.5
This patch fixes kernel crash issue, when we receive URB request
after de-enumerating device.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index ed63868..d313a63 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -480,10 +480,14 @@ static int oz_enqueue_ep_urb(struct oz_port *port, u8 ep_addr, int in_dir,
oz_free_urb_link(urbl);
return 0;
}
- if (in_dir)
+ if (in_dir && port->in_ep[ep_addr])
ep = port->in_ep[ep_addr];
- else
+ else if (!in_dir && port->out_ep[ep_addr])
ep = port->out_ep[ep_addr];
+ else {
+ err = -ENOMEM;
+ goto out;
+ }
/*For interrupt endpoint check for buffered data
* & complete urb
@@ -505,6 +509,7 @@ static int oz_enqueue_ep_urb(struct oz_port *port, u8 ep_addr, int in_dir,
} else {
err = -EPIPE;
}
+out:
spin_unlock_bh(&port->ozhcd->hcd_lock);
if (err)
oz_free_urb_link(urbl);
--
1.7.9.5
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.
regards,
dan carpenter
On Mon, Aug 05, 2013 at 06:40:12PM +0100, Rupesh Gujare wrote:
> This patch fixes kernel crash issue, when we receive URB request
> after de-enumerating device.
>
In other words we were getting a NULL dereference dereferencing
"ep". There is an existing check already, which should be cleaned
up.
drivers/staging/ozwpan/ozhcd.c
498
499 if (ep && port->hpd) {
^^
This useless existing check should be removed.
500 list_add_tail(&urbl->link, &ep->urb_list);
501 if (!in_dir && ep_addr && (ep->credit < 0)) {
502 getrawmonotonic(&ep->timestamp);
503 ep->credit = 0;
504 }
505 } else {
506 err = -EPIPE;
507 }
I'm not sure that think -ENOMEM is the correct error code but I
also don't know what else to use.
I had a style nit pick as well, below.
> Signed-off-by: Rupesh Gujare <[email protected]>
> ---
> drivers/staging/ozwpan/ozhcd.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
> index ed63868..d313a63 100644
> --- a/drivers/staging/ozwpan/ozhcd.c
> +++ b/drivers/staging/ozwpan/ozhcd.c
> @@ -480,10 +480,14 @@ static int oz_enqueue_ep_urb(struct oz_port *port, u8 ep_addr, int in_dir,
> oz_free_urb_link(urbl);
> return 0;
> }
> - if (in_dir)
> + if (in_dir && port->in_ep[ep_addr])
> ep = port->in_ep[ep_addr];
> - else
> + else if (!in_dir && port->out_ep[ep_addr])
> ep = port->out_ep[ep_addr];
In the future, use kernel braces style. If one side of the if else
statement gets a brace then they both get one. So it's like this:
if (in_dir && port->in_ep[ep_addr]) {
ep = port->in_ep[ep_addr];
} else if (!in_dir && port->out_ep[ep_addr]) {
ep = port->out_ep[ep_addr];
} else {
err = -ENOMEM;
goto out;
}
Or another simpler way to write this would be:
ep = NULL;
if (in_dir)
ep = port->in_ep[ep_addr];
else
ep = port->out_ep[ep_addr];
if (!ep) {
err = -ENOMEM;
goto unlock;
}
regards,
dan carpenter
On Mon, Aug 05, 2013 at 06:40:14PM +0100, Rupesh Gujare wrote:
> Make sure that we reset port configuration no. when PD departs.
>
What happens if we don't do this? What is the user visible effect
of this patch?
regards,
dan carpenter
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.
--
Regards,
Rupesh Gujare
On Mon, Aug 05, 2013 at 06:40:15PM +0100, Rupesh Gujare wrote:
> Fix a bug where we were not returning correct hub status
> for 8th port.
What are the user visible effects of this bug?
Style nits below.
>
> Signed-off-by: Rupesh Gujare <[email protected]>
> ---
> drivers/staging/ozwpan/ozhcd.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
> index b060e43..2f93a00 100644
> --- a/drivers/staging/ozwpan/ozhcd.c
> +++ b/drivers/staging/ozwpan/ozhcd.c
> @@ -1871,17 +1871,24 @@ static int oz_hcd_hub_status_data(struct usb_hcd *hcd, char *buf)
> int i;
>
> buf[0] = 0;
> + buf[1] = 0;
>
> spin_lock_bh(&ozhcd->hcd_lock);
> for (i = 0; i < OZ_NB_PORTS; i++) {
> if (ozhcd->ports[i].flags & OZ_PORT_F_CHANGED) {
> oz_dbg(HUB, "Port %d changed\n", i);
> ozhcd->ports[i].flags &= ~OZ_PORT_F_CHANGED;
> - buf[0] |= 1<<(i+1);
> + if (i < 7)
> + buf[0] |= 1 << (i+1);
Put spaces around math operations:
buf[0] |= 1 << (i + 1);
> + else
> + buf[1] |= 1 << (i-7);
buf[1] |= 1 << (i - 7);
regards,
dan carpenter
On 05/08/13 19:21, Dan Carpenter wrote:
> On Mon, Aug 05, 2013 at 06:40:14PM +0100, Rupesh Gujare wrote:
>> Make sure that we reset port configuration no. when PD departs.
>>
> What happens if we don't do this? What is the user visible effect
> of this patch?
>
>
There is no user visible effect at the moment. Clearing this variable as
safeguard measure.
--
Regards,
Rupesh Gujare
On 05/08/13 19:56, Dan Carpenter wrote:
> On Mon, Aug 05, 2013 at 06:40:15PM +0100, Rupesh Gujare wrote:
>> Fix a bug where we were not returning correct hub status
>> for 8th port.
> What are the user visible effects of this bug?
8 th port is never assigned to new device & we loose one port.
> Style nits below.
>
>> Signed-off-by: Rupesh Gujare <[email protected]>
>> ---
>> drivers/staging/ozwpan/ozhcd.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
>> index b060e43..2f93a00 100644
>> --- a/drivers/staging/ozwpan/ozhcd.c
>> +++ b/drivers/staging/ozwpan/ozhcd.c
>> @@ -1871,17 +1871,24 @@ static int oz_hcd_hub_status_data(struct usb_hcd *hcd, char *buf)
>> int i;
>>
>> buf[0] = 0;
>> + buf[1] = 0;
>>
>> spin_lock_bh(&ozhcd->hcd_lock);
>> for (i = 0; i < OZ_NB_PORTS; i++) {
>> if (ozhcd->ports[i].flags & OZ_PORT_F_CHANGED) {
>> oz_dbg(HUB, "Port %d changed\n", i);
>> ozhcd->ports[i].flags &= ~OZ_PORT_F_CHANGED;
>> - buf[0] |= 1<<(i+1);
>> + if (i < 7)
>> + buf[0] |= 1 << (i+1);
> Put spaces around math operations:
> buf[0] |= 1 << (i + 1);
>
>> + else
>> + buf[1] |= 1 << (i-7);
> buf[1] |= 1 << (i - 7);
>
>
Ok. I will rework on style nits.
--
Regards,
Rupesh Gujare
On Mon, Aug 05, 2013 at 08:00:59PM +0100, Rupesh Gujare wrote:
> Ok. I will rework on style nits.
Do it in a follow on patch. It's fine.
regards,
dan carpenter
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.
regards,
dan carpenter
Here is what oz_hcd_pd_arrived() looks like after my changes:
These lines:
- if (ozhcd == NULL)
+ if (!ozhcd)
were personal preference and not official kernel style guidelines.
Either way is fine for pointers.
oz_ep_alloc() can return NULL. There was no check for failure in
the original code.
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index f81a0c5..6b9fc02 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -620,58 +621,59 @@ static inline void oz_hcd_put(struct oz_hcd *ozhcd)
* probably very rare indeed.
* Context: softirq
*/
-void *oz_hcd_pd_arrived(void *hpd)
+struct oz_port *oz_hcd_pd_arrived(void *hpd)
{
- int i;
- void *hport = NULL;
- struct oz_hcd *ozhcd = NULL;
+ struct oz_port *hport = NULL;
+ struct oz_hcd *ozhcd;
struct oz_endpoint *ep;
+ int i;
+
ozhcd = oz_hcd_claim();
- if (ozhcd == NULL)
+ if (!ozhcd)
return NULL;
- /* Allocate an endpoint object in advance (before holding hcd lock) to
- * use for out endpoint 0.
- */
+
+ /* Allocate an endpoint object to use for out endpoint 0. */
ep = oz_ep_alloc(GFP_ATOMIC, 0);
+ if (!ep)
+ goto err_put;
+
spin_lock_bh(&ozhcd->hcd_lock);
- if (ozhcd->conn_port >= 0) {
- spin_unlock_bh(&ozhcd->hcd_lock);
- oz_dbg(ON, "conn_port >= 0\n");
- goto out;
- }
+ if (ozhcd->conn_port >= 0)
+ goto err_unlock;
+
for (i = 0; i < OZ_NB_PORTS; i++) {
struct oz_port *port = &ozhcd->ports[i];
+
spin_lock(&port->port_lock);
- if ((port->flags & OZ_PORT_F_PRESENT) == 0) {
+ if (!(port->flags & OZ_PORT_F_PRESENT)) {
oz_acquire_port(port, hpd);
spin_unlock(&port->port_lock);
break;
}
spin_unlock(&port->port_lock);
}
- if (i < OZ_NB_PORTS) {
- oz_dbg(ON, "Setting conn_port = %d\n", i);
- ozhcd->conn_port = i;
- /* Attach out endpoint 0.
- */
- ozhcd->ports[i].out_ep[0] = ep;
- ep = NULL;
- hport = &ozhcd->ports[i];
- spin_unlock_bh(&ozhcd->hcd_lock);
- if (ozhcd->flags & OZ_HDC_F_SUSPENDED) {
- oz_dbg(ON, "Resuming root hub\n");
- usb_hcd_resume_root_hub(ozhcd->hcd);
- }
- usb_hcd_poll_rh_status(ozhcd->hcd);
- } else {
- spin_unlock_bh(&ozhcd->hcd_lock);
- }
-out:
- if (ep) /* ep is non-null if not used. */
- oz_ep_free(NULL, ep);
+ if (i == OZ_NB_PORTS)
+ goto err_unlock;
+
+ ozhcd->conn_port = i;
+ hport = &ozhcd->ports[i];
+ hport->out_ep[0] = ep;
+ spin_unlock_bh(&ozhcd->hcd_lock);
+ if (ozhcd->flags & OZ_HDC_F_SUSPENDED)
+ usb_hcd_resume_root_hub(ozhcd->hcd);
+ usb_hcd_poll_rh_status(ozhcd->hcd);
oz_hcd_put(ozhcd);
+
return hport;
+
+err_unlock:
+ spin_unlock_bh(&ozhcd->hcd_lock);
+ oz_ep_free(NULL, ep);
+err_put:
+ oz_hcd_put(ozhcd);
+ return NULL;
}
+
/*------------------------------------------------------------------------------
* This is called by the protocol handler to notify that the PD has gone away.
* We need to deallocate all resources and then request that the root hub is
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
On Tue, Aug 06, 2013 at 11:26:14AM +0100, Rupesh Gujare wrote:
> 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.
Yes. That's fine.
regards,
dan carpenter
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.
>
> Signed-off-by: Rupesh Gujare <[email protected]>
> ---
> drivers/staging/ozwpan/ozhcd.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
No, don't paper over this, fix it correctly with reference counting as
Dan has suggested.
thanks,
greg k-h