[PATCH] mwifiex: remove duplicated NULL checks

[PATCH] mwifiex: remove duplicated NULL checks

[Bing Zhao]

From: Amitkumar Karwar <akarwar@marvell.com>

Some functions have unnecessory NULL check for an input parameter
because the caller to those functions is already checking the parameter
for NULL before calling them.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
 drivers/net/wireless/mwifiex/cmdevt.c    |   25 -------------------------
 drivers/net/wireless/mwifiex/scan.c      |    5 +----
 drivers/net/wireless/mwifiex/sta_ioctl.c |    6 ------
 drivers/net/wireless/mwifiex/util.c      |    2 --
 4 files changed, 1 insertions(+), 37 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
index 60aeea4..5c512d5 100644
--- a/drivers/net/wireless/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/mwifiex/cmdevt.c
@@ -41,11 +41,6 @@ mwifiex_init_cmd_node(struct mwifiex_private *priv,
 {
 	ENTER();
 
-	if (cmd_node == NULL) {
-		LEAVE();
-		return;
-	}
-
 	cmd_node->priv = priv;
 	cmd_node->cmd_oid = cmd_oid;
 	cmd_node->ioctl_buf = ioctl_buf;
@@ -74,11 +69,6 @@ mwifiex_get_cmd_node(struct mwifiex_adapter *adapter)
 
 	ENTER();
 
-	if (adapter == NULL) {
-		LEAVE();
-		return NULL;
-	}
-
 	spin_lock_irqsave(&adapter->cmd_free_q_lock, flags);
 	if (list_empty(&adapter->cmd_free_q)) {
 		PRINTM(MERROR,
@@ -112,10 +102,6 @@ mwifiex_clean_cmd_node(struct mwifiex_adapter *adapter,
 {
 	ENTER();
 
-	if (cmd_node == NULL) {
-		LEAVE();
-		return;
-	}
 	cmd_node->cmd_oid = 0;
 	cmd_node->cmd_flag = 0;
 	cmd_node->ioctl_buf = NULL;
@@ -720,11 +706,6 @@ mwifiex_insert_cmd_to_pending_q(struct mwifiex_adapter *adapter,
 
 	ENTER();
 
-	if (cmd_node == NULL) {
-		PRINTM(MERROR, "QUEUE_CMD: cmd_node is NULL\n");
-		goto done;
-	}
-
 	host_cmd = (struct host_cmd_ds_command *) (cmd_node->cmd_buf->buffer +
 					       cmd_node->cmd_buf->data_offset);
 	if (host_cmd == NULL) {
@@ -782,12 +763,6 @@ mwifiex_exec_next_cmd(struct mwifiex_adapter *adapter)
 
 	ENTER();
 
-	/* Sanity test */
-	if (adapter == NULL) {
-		PRINTM(MERROR, "EXEC_NEXT_CMD: adapter is NULL\n");
-		ret = MWIFIEX_STATUS_FAILURE;
-		goto done;
-	}
 	/* Check if already in processing */
 	if (adapter->curr_cmd) {
 		PRINTM(MERROR,
diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
index df4a77c..6da0612 100644
--- a/drivers/net/wireless/mwifiex/scan.c
+++ b/drivers/net/wireless/mwifiex/scan.c
@@ -3162,13 +3162,10 @@ mwifiex_queue_scan_cmd(struct mwifiex_private *priv,
 
 	ENTER();
 
-	if (cmd_node == NULL)
-		goto done;
-
 	spin_lock_irqsave(&adapter->scan_pending_q_lock, flags);
 	list_add_tail(&cmd_node->list, &adapter->scan_pending_q);
 	spin_unlock_irqrestore(&adapter->scan_pending_q_lock, flags);
-done:
+
 	LEAVE();
 }
 
diff --git a/drivers/net/wireless/mwifiex/sta_ioctl.c b/drivers/net/wireless/mwifiex/sta_ioctl.c
index 04a6154..f4fcb74 100644
--- a/drivers/net/wireless/mwifiex/sta_ioctl.c
+++ b/drivers/net/wireless/mwifiex/sta_ioctl.c
@@ -42,12 +42,6 @@ mwifiex_ioctl(struct mwifiex_adapter *adapter,
 
 	ENTER();
 
-	if (ioctl_req == NULL) {
-		PRINTM(MERROR, "%s: IOCTL request buffer is NULL\n", __func__);
-		ret = MWIFIEX_STATUS_FAILURE;
-		goto exit;
-	}
-
 	if (ioctl_req->action == MWIFIEX_ACT_CANCEL) {
 		mwifiex_cancel_pending_ioctl(adapter, ioctl_req);
 		ret = MWIFIEX_STATUS_SUCCESS;
diff --git a/drivers/net/wireless/mwifiex/util.c b/drivers/net/wireless/mwifiex/util.c
index 7e1bc9b..c16f5f5 100644
--- a/drivers/net/wireless/mwifiex/util.c
+++ b/drivers/net/wireless/mwifiex/util.c
@@ -546,8 +546,6 @@ mwifiex_process_ioctl_resp(struct mwifiex_private *priv,
 {
 	ENTER();
 
-	if (priv == NULL)
-		return;
 	switch (req->req_id) {
 	case MWIFIEX_IOCTL_GET_INFO:
 		mwifiex_ioctl_get_info_resp(priv,
-- 
1.7.0.2

Re: [PATCH] mwifiex: remove duplicated NULL checks

[Julian Calaby]

On Sat, Jan 15, 2011 at 06:36, Bing Zhao <bzhao@marvell.com> wrote:
> From: Amitkumar Karwar <akarwar@marvell.com>
>
> Some functions have unnecessory NULL check for an input parameter
> because the caller to those functions is already checking the parameter
> for NULL before calling them.

It may be more readable and efficient to eliminate the NULL checks in
the callers - and adjust the code to inform the callers of the case
where the pointer passed is null and make the callers to properly
handle this case.

This should also make the driver more robust as failures can be passed
up the stack to places that can deal with them more effectively than
just printing a message.

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

RE: [PATCH] mwifiex: remove duplicated NULL checks

[Bing Zhao]

Hi Julian,

> -----Original Message-----
> From: Julian Calaby [mailto:julian.calaby@gmail.com]
> Sent: Friday, January 14, 2011 6:59 PM
> To: Bing Zhao
> Cc: linux-wireless@vger.kernel.org; John W. Linville; Johannes Berg; Amitkumar Karwar; Kiran Divekar;
> Frank Huang
> Subject: Re: [PATCH] mwifiex: remove duplicated NULL checks
> 
> On Sat, Jan 15, 2011 at 06:36, Bing Zhao <bzhao@marvell.com> wrote:
> > From: Amitkumar Karwar <akarwar@marvell.com>
> >
> > Some functions have unnecessory NULL check for an input parameter
> > because the caller to those functions is already checking the parameter
> > for NULL before calling them.
> 
> It may be more readable and efficient to eliminate the NULL checks in
> the callers - and adjust the code to inform the callers of the case
> where the pointer passed is null and make the callers to properly
> handle this case.

Thank you very much for your comments.

There are still some cases that the NULL check needs to be done at the first place. For example,


/* the caller function */
enum mwifiex_status mwifiex_prepare_cmd(struct mwifiex_private *priv,
                u16 cmd_no, u16 cmd_action, u32 cmd_oid,
		    void *ioctl_buf, void *data_buf)

	......

      cmd_node = mwifiex_get_cmd_node(adapter);

      if (cmd_node == NULL) {
              PRINTM(MERROR, "PREP_CMD: No free cmd node\n");
              ret = MWIFIEX_STATUS_FAILURE;
              goto done;
      }
	/*
	  Here the cmd_node is allocated and the NULL check is preformed
        immediately. If the pointer is NULL then the error code is
	  returned for proper handling.
	*/


	/*
	  Now cmd_node is passed to the callee as a parameter
 	*/
      mwifiex_init_cmd_node(priv, cmd_node, cmd_oid, ioctl_buf,
                              data_buf);

	......
}

/* the callee function */
static void mwifiex_init_cmd_node(struct mwifiex_private *priv,
                      struct cmd_ctrl_node *cmd_node,
                      u32 cmd_oid, void *ioctl_buf, void *data_buf)
{
      ENTER();

-	if (cmd_node == NULL) {
-		LEAVE();
-		return;
-	}
-
	/*
	  Here it is redundant to check it again since the cmd_node
        has already been validated by the caller. 
	*/

      cmd_node->priv = priv;
      cmd_node->cmd_oid = cmd_oid;
      cmd_node->ioctl_buf = ioctl_buf;

	......
}

> 
> This should also make the driver more robust as failures can be passed
> up the stack to places that can deal with them more effectively than
> just printing a message.

I could have misunderstood your point, however. If so, could you please explain a little bit more on how you would make the changes based on above example?

Thanks in advance for your help!

Bing

> 
> Thanks,
> 
> --
> 
> Julian Calaby
> 
> Email: julian.calaby@gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/
> .Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH] mwifiex: remove duplicated NULL checks

[Julian Calaby]

On Tue, Jan 18, 2011 at 14:07, Bing Zhao <bzhao@marvell.com> wrote:
> Hi Julian,
>
>> -----Original Message-----
>> From: Julian Calaby [mailto:julian.calaby@gmail.com]
>> Sent: Friday, January 14, 2011 6:59 PM
>> To: Bing Zhao
>> Cc: linux-wireless@vger.kernel.org; John W. Linville; Johannes Berg; Amitkumar Karwar; Kiran Divekar;
>> Frank Huang
>> Subject: Re: [PATCH] mwifiex: remove duplicated NULL checks
>>
>> On Sat, Jan 15, 2011 at 06:36, Bing Zhao <bzhao@marvell.com> wrote:
>> > From: Amitkumar Karwar <akarwar@marvell.com>
>> >
>> > Some functions have unnecessory NULL check for an input parameter
>> > because the caller to those functions is already checking the parameter
>> > for NULL before calling them.
>>
>> It may be more readable and efficient to eliminate the NULL checks in
>> the callers - and adjust the code to inform the callers of the case
>> where the pointer passed is null and make the callers to properly
>> handle this case.
>
> Thank you very much for your comments.
>
> There are still some cases that the NULL check needs to be done at the first place. For example,
>
[snip]

In this case, dropping the null check is the right thing to do, given
that you can guarantee that all callers of mwiflex_init_cmd_node()
will check that the pointer isn't null before calling it.

>> This should also make the driver more robust as failures can be passed
>> up the stack to places that can deal with them more effectively than
>> just printing a message.
>
> I could have misunderstood your point, however. If so, could you please explain a little bit more on how you would make the changes based on above example?

I was referring more to that fact that functions that appear to do
things, e.g. mwifiex_init_cmd_node(), don't return anything - hence if
something fails within them, e.g. being passed a null pointer, they
can't notify their caller, e.g. mwifiex_prepare_cmd() that something
has gone wrong.

That said, I haven't looked at the code except through your patches,
so I can't say if this is a scenario that actually could happen, and
whether this added functionality is worthwhile.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

RE: [PATCH] mwifiex: remove duplicated NULL checks

[Bing Zhao]

Hi Julian,

> -----Original Message-----
> From: Julian Calaby [mailto:julian.calaby@gmail.com]
> Sent: Monday, January 17, 2011 7:26 PM
> To: Bing Zhao
> Cc: linux-wireless@vger.kernel.org; John W. Linville; Johannes Berg; Amitkumar Karwar; Kiran Divekar;
> Frank Huang
> Subject: Re: [PATCH] mwifiex: remove duplicated NULL checks
> 
> On Tue, Jan 18, 2011 at 14:07, Bing Zhao <bzhao@marvell.com> wrote:
> > Hi Julian,
> >
> >> -----Original Message-----
> >> From: Julian Calaby [mailto:julian.calaby@gmail.com]
> >> Sent: Friday, January 14, 2011 6:59 PM
> >> To: Bing Zhao
> >> Cc: linux-wireless@vger.kernel.org; John W. Linville; Johannes Berg; Amitkumar Karwar; Kiran
> Divekar;
> >> Frank Huang
> >> Subject: Re: [PATCH] mwifiex: remove duplicated NULL checks
> >>
> >> On Sat, Jan 15, 2011 at 06:36, Bing Zhao <bzhao@marvell.com> wrote:
> >> > From: Amitkumar Karwar <akarwar@marvell.com>
> >> >
> >> > Some functions have unnecessory NULL check for an input parameter
> >> > because the caller to those functions is already checking the parameter
> >> > for NULL before calling them.
> >>
> >> It may be more readable and efficient to eliminate the NULL checks in
> >> the callers - and adjust the code to inform the callers of the case
> >> where the pointer passed is null and make the callers to properly
> >> handle this case.
> >
> > Thank you very much for your comments.
> >
> > There are still some cases that the NULL check needs to be done at the first place. For example,
> >
> [snip]
> 
> In this case, dropping the null check is the right thing to do, given
> that you can guarantee that all callers of mwiflex_init_cmd_node()
> will check that the pointer isn't null before calling it.
> 
> >> This should also make the driver more robust as failures can be passed
> >> up the stack to places that can deal with them more effectively than
> >> just printing a message.
> >
> > I could have misunderstood your point, however. If so, could you please explain a little bit more
> on how you would make the changes based on above example?
> 
> I was referring more to that fact that functions that appear to do
> things, e.g. mwifiex_init_cmd_node(), don't return anything - hence if
> something fails within them, e.g. being passed a null pointer, they
> can't notify their caller, e.g. mwifiex_prepare_cmd() that something
> has gone wrong.
> 
> That said, I haven't looked at the code except through your patches,
> so I can't say if this is a scenario that actually could happen, and
> whether this added functionality is worthwhile.

Thanks for your comments.

mwifiex_init_cmd_node() just initializes the cmd_node structure with variables that have been validated before. But what you said makes sense in general. The function should return something to the caller if anything can go wrong.

Best regards,

Bing

> 
> Thanks,
> 
> --
> Julian Calaby
> 
> Email: julian.calaby@gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/
> .Plan: http://sites.google.com/site/juliancalaby/