[PATCH 0/4] staging: ozwpan: Fix crash issues.

[PATCH 0/4] staging: ozwpan: Fix crash issues.

[Rupesh Gujare]

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

[PATCH 1/4] staging: ozwpan: Fixes crash due to invalid port aceess.

[Rupesh Gujare]

This patch fixes kernel crash issue, when we receive URB request
after de-enumerating device.

Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
---
 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

[PATCH 2/4] staging: ozwpan: Increment port number for new device.

[Rupesh Gujare]

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 <rupesh.gujare@atmel.com>
---
 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

[PATCH 3/4] staging: ozwpan: Reset port configuration number.

[Rupesh Gujare]

Make sure that we reset port configuration no. when PD departs.

Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
---
 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

[PATCH 4/4] staging: ozwpan: Return correct hub status.

[Rupesh Gujare]

Fix a bug where we were not returning correct hub status
for 8th port.

Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
---
 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

Re: [PATCH 2/4] staging: ozwpan: Increment port number for new device.

[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.
> 

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

Re: [PATCH 1/4] staging: ozwpan: Fixes crash due to invalid port aceess.

[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 <rupesh.gujare@atmel.com>
> ---
>  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

Re: [PATCH 3/4] staging: ozwpan: Reset port configuration number.

[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

Re: [PATCH 2/4] staging: ozwpan: Increment port number for new device.

[Rupesh Gujare]

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

Re: [PATCH 4/4] staging: ozwpan: Return correct hub status.

[Dan Carpenter]

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 <rupesh.gujare@atmel.com>
> ---
>  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

Re: [PATCH 3/4] staging: ozwpan: Reset port configuration number.

[Rupesh Gujare]

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

Re: [PATCH 4/4] staging: ozwpan: Return correct hub status.

[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 <rupesh.gujare@atmel.com>
>> ---
>>   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

Re: [PATCH 4/4] staging: ozwpan: Return correct hub status.

[Dan Carpenter]

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

Re: [PATCH 2/4] staging: ozwpan: Increment port number for new device.

[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

Re: [PATCH 2/4] staging: ozwpan: Increment port number for new device.

[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

Re: [PATCH 2/4] staging: ozwpan: Increment port number for new device.

[Rupesh Gujare]

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

Re: [PATCH 2/4] staging: ozwpan: Increment port number for new device.

[Dan Carpenter]

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

Re: [PATCH 2/4] staging: ozwpan: Increment port number for new device.

[Greg KH]

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 <rupesh.gujare@atmel.com>
> ---
>  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