We are already checking "ep" earlier in function. Do not
need to check again.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 2be15f4..8633f0c 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -527,7 +527,7 @@ static int oz_enqueue_ep_urb(struct oz_port *port, u8 ep_addr, int in_dir,
return 0;
}
- if (ep && port->hpd) {
+ if (port->hpd) {
list_add_tail(&urbl->link, &ep->urb_list);
if (!in_dir && ep_addr && (ep->credit < 0)) {
getrawmonotonic(&ep->timestamp);
--
1.7.9.5
oz_hcd_pd_arrived returns struct oz_port *, change function
declaration to avoid ambiguity.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 4 ++--
drivers/staging/ozwpan/ozhcd.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 8633f0c..73d80f2 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -660,10 +660,10 @@ 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_port *hport = NULL;
struct oz_hcd *ozhcd = NULL;
struct oz_endpoint *ep;
diff --git a/drivers/staging/ozwpan/ozhcd.h b/drivers/staging/ozwpan/ozhcd.h
index 9b30dfd..ef3202f 100644
--- a/drivers/staging/ozwpan/ozhcd.h
+++ b/drivers/staging/ozwpan/ozhcd.h
@@ -7,8 +7,8 @@
int oz_hcd_init(void);
void oz_hcd_term(void);
-void *oz_hcd_pd_arrived(void *ctx);
void oz_hcd_pd_departed(void *ctx);
+struct oz_port *oz_hcd_pd_arrived(void *ctx);
void oz_hcd_pd_reset(void *hpd, void *hport);
#endif /* _OZHCD_H */
--
1.7.9.5
oz_hcd_pd_departed() takes struct oz_port pointer instead of
void *, change function declaration to avoid ambiguity.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 4 ++--
drivers/staging/ozwpan/ozhcd.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 73d80f2..ed3ffeb 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -720,9 +720,9 @@ out:
* polled. We release the reference we hold on the PD.
* Context: softirq
*/
-void oz_hcd_pd_departed(void *hport)
+void oz_hcd_pd_departed(struct oz_port *hport)
{
- struct oz_port *port = (struct oz_port *)hport;
+ struct oz_port *port = hport;
struct oz_hcd *ozhcd;
void *hpd;
struct oz_endpoint *ep = NULL;
diff --git a/drivers/staging/ozwpan/ozhcd.h b/drivers/staging/ozwpan/ozhcd.h
index ef3202f..55e97b1 100644
--- a/drivers/staging/ozwpan/ozhcd.h
+++ b/drivers/staging/ozwpan/ozhcd.h
@@ -7,8 +7,8 @@
int oz_hcd_init(void);
void oz_hcd_term(void);
-void oz_hcd_pd_departed(void *ctx);
struct oz_port *oz_hcd_pd_arrived(void *ctx);
+void oz_hcd_pd_departed(struct oz_port *hport);
void oz_hcd_pd_reset(void *hpd, void *hport);
#endif /* _OZHCD_H */
--
1.7.9.5
Remove variable initialization wherever it is not required.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozeltbuf.c | 2 +-
drivers/staging/ozwpan/ozhcd.c | 18 +++++++++---------
drivers/staging/ozwpan/ozpd.c | 4 ++--
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/ozwpan/ozeltbuf.c b/drivers/staging/ozwpan/ozeltbuf.c
index 4844d9f..cfc0f39 100644
--- a/drivers/staging/ozwpan/ozeltbuf.c
+++ b/drivers/staging/ozwpan/ozeltbuf.c
@@ -67,7 +67,7 @@ void oz_elt_buf_term(struct oz_elt_buf *buf)
*/
struct oz_elt_info *oz_elt_info_alloc(struct oz_elt_buf *buf)
{
- struct oz_elt_info *ei = NULL;
+ struct oz_elt_info *ei;
spin_lock_bh(&buf->lock);
if (buf->free_elts && buf->elt_pool) {
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index ed3ffeb..0fd3fea 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -373,7 +373,7 @@ static void oz_complete_urb(struct usb_hcd *hcd, struct urb *urb,
{
struct oz_hcd *ozhcd = oz_hcd_private(hcd);
unsigned long irq_state;
- struct oz_urb_link *cancel_urbl = NULL;
+ struct oz_urb_link *cancel_urbl;
spin_lock_irqsave(&g_tasklet_lock, irq_state);
usb_hcd_unlink_urb_from_ep(hcd, urb);
@@ -585,7 +585,7 @@ static struct urb *oz_find_urb_by_id(struct oz_port *port, int ep_ix,
{
struct oz_hcd *ozhcd = port->ozhcd;
struct urb *urb = NULL;
- struct oz_urb_link *urbl = NULL;
+ struct oz_urb_link *urbl;
struct oz_endpoint *ep;
spin_lock_bh(&ozhcd->hcd_lock);
@@ -664,7 +664,7 @@ struct oz_port *oz_hcd_pd_arrived(void *hpd)
{
int i;
struct oz_port *hport = NULL;
- struct oz_hcd *ozhcd = NULL;
+ struct oz_hcd *ozhcd;
struct oz_endpoint *ep;
ozhcd = oz_hcd_claim();
@@ -1423,7 +1423,7 @@ static void oz_clean_endpoints_for_config(struct usb_hcd *hcd,
*/
static void *oz_claim_hpd(struct oz_port *port)
{
- void *hpd = NULL;
+ void *hpd;
struct oz_hcd *ozhcd = port->ozhcd;
spin_lock_bh(&ozhcd->hcd_lock);
@@ -1444,7 +1444,7 @@ static void oz_process_ep0_urb(struct oz_hcd *ozhcd, struct urb *urb,
unsigned windex;
unsigned wvalue;
unsigned wlength;
- void *hpd = NULL;
+ void *hpd;
u8 req_id;
int rc = 0;
unsigned complete = 0;
@@ -1798,7 +1798,7 @@ static int oz_hcd_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
gfp_t mem_flags)
{
struct oz_hcd *ozhcd = oz_hcd_private(hcd);
- int rc = 0;
+ int rc;
int port_ix;
struct oz_port *port;
unsigned long irq_state;
@@ -1851,7 +1851,7 @@ static int oz_hcd_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
static struct oz_urb_link *oz_remove_urb(struct oz_endpoint *ep,
struct urb *urb)
{
- struct oz_urb_link *urbl = NULL;
+ struct oz_urb_link *urbl;
struct list_head *e;
if (unlikely(ep == NULL))
@@ -1878,7 +1878,7 @@ static struct oz_urb_link *oz_remove_urb(struct oz_endpoint *ep,
static int oz_hcd_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
{
struct oz_hcd *ozhcd = oz_hcd_private(hcd);
- struct oz_urb_link *urbl = NULL;
+ struct oz_urb_link *urbl;
int rc;
unsigned long irq_state;
@@ -2141,7 +2141,7 @@ static int oz_clear_port_feature(struct usb_hcd *hcd, u16 wvalue, u16 windex)
static int oz_get_port_status(struct usb_hcd *hcd, u16 windex, char *buf)
{
struct oz_hcd *ozhcd;
- u32 status = 0;
+ u32 status;
if ((windex < 1) || (windex > OZ_NB_PORTS))
return -EPIPE;
diff --git a/drivers/staging/ozwpan/ozpd.c b/drivers/staging/ozwpan/ozpd.c
index 95109fd..2514d79 100644
--- a/drivers/staging/ozwpan/ozpd.c
+++ b/drivers/staging/ozwpan/ozpd.c
@@ -337,7 +337,7 @@ void oz_pd_heartbeat(struct oz_pd *pd, u16 apps)
*/
void oz_pd_stop(struct oz_pd *pd)
{
- u16 stop_apps = 0;
+ u16 stop_apps;
oz_dbg(ON, "oz_pd_stop() State = 0x%x\n", pd->state);
oz_pd_indicate_farewells(pd);
@@ -362,7 +362,7 @@ void oz_pd_stop(struct oz_pd *pd)
int oz_pd_sleep(struct oz_pd *pd)
{
int do_stop = 0;
- u16 stop_apps = 0;
+ u16 stop_apps;
oz_polling_lock_bh();
if (pd->state & (OZ_PD_S_SLEEP | OZ_PD_S_STOPPED)) {
--
1.7.9.5
Swap arguments of oz_ep_alloc() to match kmalloc() for better readability.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 0fd3fea..0b21c9f 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -327,7 +327,7 @@ static void oz_empty_link_pool(void)
* allocated it immediately follows the endpoint structure.
* Context: softirq
*/
-static struct oz_endpoint *oz_ep_alloc(gfp_t mem_flags, int buffer_size)
+static struct oz_endpoint *oz_ep_alloc(int buffer_size, gfp_t mem_flags)
{
struct oz_endpoint *ep =
kzalloc(sizeof(struct oz_endpoint)+buffer_size, mem_flags);
@@ -673,7 +673,7 @@ struct oz_port *oz_hcd_pd_arrived(void *hpd)
/* Allocate an endpoint object in advance (before holding hcd lock) to
* use for out endpoint 0.
*/
- ep = oz_ep_alloc(GFP_ATOMIC, 0);
+ ep = oz_ep_alloc(0, GFP_ATOMIC);
spin_lock_bh(&ozhcd->hcd_lock);
if (ozhcd->conn_port >= 0) {
spin_unlock_bh(&ozhcd->hcd_lock);
@@ -1268,7 +1268,7 @@ static int oz_build_endpoints_for_interface(struct usb_hcd *hcd,
}
}
- ep = oz_ep_alloc(mem_flags, buffer_size);
+ ep = oz_ep_alloc(buffer_size, mem_flags);
if (!ep) {
oz_clean_endpoints_for_interface(hcd, port, if_ix);
return -ENOMEM;
--
1.7.9.5
From: Dan Carpenter <[email protected]>
This patch separates success & failure block along with fixing
following issues:-
1. 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.
2. It is hard to read it because there are unlocks scattered throughout.
3. Currently we set "ep" to NULL on the success path and then test it and or
free it. In current code you have to scroll to the start of the function
to read code.
Original patch was submitted by Dan here :-
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2013-August/040113.html
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 54 ++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 0b21c9f..4cd08da 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -668,50 +668,50 @@ struct oz_port *oz_hcd_pd_arrived(void *hpd)
struct oz_endpoint *ep;
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.
*/
ep = oz_ep_alloc(0, GFP_ATOMIC);
+ 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;
}
/*------------------------------------------------------------------------------
--
1.7.9.5
Hello.
On 08/13/2013 09:29 PM, Rupesh Gujare wrote:
> oz_hcd_pd_departed() takes struct oz_port pointer instead of
> void *, change function declaration to avoid ambiguity.
> Reported-by: Dan Carpenter <[email protected]>
> Signed-off-by: Rupesh Gujare <[email protected]>
> ---
> drivers/staging/ozwpan/ozhcd.c | 4 ++--
> drivers/staging/ozwpan/ozhcd.h | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
> diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
> index 73d80f2..ed3ffeb 100644
> --- a/drivers/staging/ozwpan/ozhcd.c
> +++ b/drivers/staging/ozwpan/ozhcd.c
> @@ -720,9 +720,9 @@ out:
> * polled. We release the reference we hold on the PD.
> * Context: softirq
> */
> -void oz_hcd_pd_departed(void *hport)
> +void oz_hcd_pd_departed(struct oz_port *hport)
> {
> - struct oz_port *port = (struct oz_port *)hport;
> + struct oz_port *port = hport;
Do you really need a copy? Isn't it better to rename the parameter and
remove this line altogether?
WBR, Sergei
On Tue, 2013-08-13 at 18:29 +0100, Rupesh Gujare wrote:
> Swap arguments of oz_ep_alloc() to match kmalloc() for better readability.
[]
> diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
[]
> -static struct oz_endpoint *oz_ep_alloc(gfp_t mem_flags, int buffer_size)
> +static struct oz_endpoint *oz_ep_alloc(int buffer_size, gfp_t mem_flags)
could use size_t buffer_size instead.
On 13/08/13 18:35, Sergei Shtylyov wrote:
> Hello.
>
> On 08/13/2013 09:29 PM, Rupesh Gujare wrote:
>
>> oz_hcd_pd_departed() takes struct oz_port pointer instead of
>> void *, change function declaration to avoid ambiguity.
>
>> Reported-by: Dan Carpenter <[email protected]>
>> Signed-off-by: Rupesh Gujare <[email protected]>
>> ---
>> drivers/staging/ozwpan/ozhcd.c | 4 ++--
>> drivers/staging/ozwpan/ozhcd.h | 2 +-
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>
>> diff --git a/drivers/staging/ozwpan/ozhcd.c
>> b/drivers/staging/ozwpan/ozhcd.c
>> index 73d80f2..ed3ffeb 100644
>> --- a/drivers/staging/ozwpan/ozhcd.c
>> +++ b/drivers/staging/ozwpan/ozhcd.c
>> @@ -720,9 +720,9 @@ out:
>> * polled. We release the reference we hold on the PD.
>> * Context: softirq
>> */
>> -void oz_hcd_pd_departed(void *hport)
>> +void oz_hcd_pd_departed(struct oz_port *hport)
>> {
>> - struct oz_port *port = (struct oz_port *)hport;
>> + struct oz_port *port = hport;
>
> Do you really need a copy? Isn't it better to rename the parameter
> and remove this line altogether?
>
> WBR, Sergei
>
Yes, that is the idea, for next patch series, as I don't want to mix
two changes in single patch.
--
Regards,
Rupesh Gujare
On 08/13/2013 09:40 PM, Rupesh Gujare wrote:
>>> oz_hcd_pd_departed() takes struct oz_port pointer instead of
>>> void *, change function declaration to avoid ambiguity.
>>> Reported-by: Dan Carpenter <[email protected]>
>>> Signed-off-by: Rupesh Gujare <[email protected]>
>>> ---
>>> drivers/staging/ozwpan/ozhcd.c | 4 ++--
>>> drivers/staging/ozwpan/ozhcd.h | 2 +-
>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>> diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
>>> index 73d80f2..ed3ffeb 100644
>>> --- a/drivers/staging/ozwpan/ozhcd.c
>>> +++ b/drivers/staging/ozwpan/ozhcd.c
>>> @@ -720,9 +720,9 @@ out:
>>> * polled. We release the reference we hold on the PD.
>>> * Context: softirq
>>> */
>>> -void oz_hcd_pd_departed(void *hport)
>>> +void oz_hcd_pd_departed(struct oz_port *hport)
>>> {
>>> - struct oz_port *port = (struct oz_port *)hport;
>>> + struct oz_port *port = hport;
>> Do you really need a copy? Isn't it better to rename the parameter and
>> remove this line altogether?
>> WBR, Sergei
> Yes, that is the idea, for next patch series, as I don't want to mix two
> changes in single patch.
Don't think it's worth another series.
WBR, Sergei
On Tue, Aug 13, 2013 at 06:29:26PM +0100, Rupesh Gujare wrote:
> From: Dan Carpenter <[email protected]>
>
> This patch separates success & failure block along with fixing
> following issues:-
>
> 1. 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.
>
> 2. It is hard to read it because there are unlocks scattered throughout.
>
> 3. Currently we set "ep" to NULL on the success path and then test it and or
> free it. In current code you have to scroll to the start of the function
> to read code.
>
> Original patch was submitted by Dan here :-
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2013-August/040113.html
>
> Signed-off-by: Rupesh Gujare <[email protected]>
Since you gave me the author tag for this then I'll sign off on this
as well.
Signed-off-by: Dan Carpenter <[email protected]>
regards,
dan carpenter
On Tue, Aug 13, 2013 at 09:46:36PM +0400, Sergei Shtylyov wrote:
>
> >> Do you really need a copy? Isn't it better to rename the parameter and
> >>remove this line altogether?
>
> >>WBR, Sergei
>
> >Yes, that is the idea, for next patch series, as I don't want to mix two
> >changes in single patch.
>
> Don't think it's worth another series.
>
Yeah, it should have been done in the same patch, you're obviously
right. But Greg already merged them so it will have to be fixed in
follow on patches.
regards,
dan carpenter