LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] staging: rtl8723bs: use generic kernel error codes
@ 2021-05-04 16:07 Bryan Brattlof
  2021-05-04 16:17 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 5+ messages in thread
From: Bryan Brattlof @ 2021-05-04 16:07 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Bryan Brattlof, linux-staging, linux-kernel

The current _FAIL and _SUCCESS error codes are defined as:

        #define _FAIL    0
	#define _SUCCESS 1

which adds complexity (and confusion) when interacting with other
submodules in the kernel. These definitions can be removed and replaced
with the kernel's generic error codes.

Signed-off-by: Bryan Brattlof <hello@bryanbrattlof.com>
---
 drivers/staging/rtl8723bs/include/drv_types.h |  2 +-
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c  | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8723bs/include/drv_types.h b/drivers/staging/rtl8723bs/include/drv_types.h
index 83d43e5726dd..5b702ff432ae 100644
--- a/drivers/staging/rtl8723bs/include/drv_types.h
+++ b/drivers/staging/rtl8723bs/include/drv_types.h
@@ -385,7 +385,7 @@ struct adapter {
 	void *xmitThread;
 	void *recvThread;

-	u32 (*intf_init)(struct dvobj_priv *dvobj);
+	int (*intf_init)(struct dvobj_priv *dvobj);
 	void (*intf_deinit)(struct dvobj_priv *dvobj);
 	int (*intf_alloc_irq)(struct dvobj_priv *dvobj);
 	void (*intf_free_irq)(struct dvobj_priv *dvobj);
diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
index d2bf444117b8..e26c51d847b6 100644
--- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
+++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
@@ -112,7 +112,7 @@ static void sdio_free_irq(struct dvobj_priv *dvobj)
 	}
 }

-static u32 sdio_init(struct dvobj_priv *dvobj)
+static int sdio_init(struct dvobj_priv *dvobj)
 {
 	struct sdio_data *psdio_data;
 	struct sdio_func *func;
@@ -139,12 +139,11 @@ static u32 sdio_init(struct dvobj_priv *dvobj)
 	psdio_data->tx_block_mode = 1;
 	psdio_data->rx_block_mode = 1;

+	return err;
+
 release:
 	sdio_release_host(func);
-
-	if (err)
-		return _FAIL;
-	return _SUCCESS;
+	return err;
 }

 static void sdio_deinit(struct dvobj_priv *dvobj)
@@ -186,7 +185,7 @@ static struct dvobj_priv *sdio_dvobj_init(struct sdio_func *func)
 	psdio = &dvobj->intf_data;
 	psdio->func = func;

-	if (sdio_init(dvobj) != _SUCCESS)
+	if (sdio_init(dvobj) < 0)
 		goto free_dvobj;

 	rtw_reset_continual_io_error(dvobj);

base-commit: 9ccce092fc64d19504fa54de4fd659e279cc92e7
--
git-series 0.9.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] staging: rtl8723bs: use generic kernel error codes
  2021-05-04 16:07 [PATCH] staging: rtl8723bs: use generic kernel error codes Bryan Brattlof
@ 2021-05-04 16:17 ` Greg Kroah-Hartman
  2021-05-04 17:45   ` Bryan Brattlof
  0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2021-05-04 16:17 UTC (permalink / raw)
  To: Bryan Brattlof; +Cc: linux-staging, linux-kernel

On Tue, May 04, 2021 at 04:07:48PM +0000, Bryan Brattlof wrote:
> The current _FAIL and _SUCCESS error codes are defined as:
> 
>         #define _FAIL    0
> 	#define _SUCCESS 1
> 
> which adds complexity (and confusion) when interacting with other
> submodules in the kernel. These definitions can be removed and replaced
> with the kernel's generic error codes.
> 
> Signed-off-by: Bryan Brattlof <hello@bryanbrattlof.com>
> ---
>  drivers/staging/rtl8723bs/include/drv_types.h |  2 +-
>  drivers/staging/rtl8723bs/os_dep/sdio_intf.c  | 11 +++++------
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/include/drv_types.h b/drivers/staging/rtl8723bs/include/drv_types.h
> index 83d43e5726dd..5b702ff432ae 100644
> --- a/drivers/staging/rtl8723bs/include/drv_types.h
> +++ b/drivers/staging/rtl8723bs/include/drv_types.h
> @@ -385,7 +385,7 @@ struct adapter {
>  	void *xmitThread;
>  	void *recvThread;
> 
> -	u32 (*intf_init)(struct dvobj_priv *dvobj);
> +	int (*intf_init)(struct dvobj_priv *dvobj);
>  	void (*intf_deinit)(struct dvobj_priv *dvobj);
>  	int (*intf_alloc_irq)(struct dvobj_priv *dvobj);
>  	void (*intf_free_irq)(struct dvobj_priv *dvobj);
> diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> index d2bf444117b8..e26c51d847b6 100644
> --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> @@ -112,7 +112,7 @@ static void sdio_free_irq(struct dvobj_priv *dvobj)
>  	}
>  }
> 
> -static u32 sdio_init(struct dvobj_priv *dvobj)
> +static int sdio_init(struct dvobj_priv *dvobj)
>  {
>  	struct sdio_data *psdio_data;
>  	struct sdio_func *func;
> @@ -139,12 +139,11 @@ static u32 sdio_init(struct dvobj_priv *dvobj)
>  	psdio_data->tx_block_mode = 1;
>  	psdio_data->rx_block_mode = 1;
> 
> +	return err;
> +
>  release:
>  	sdio_release_host(func);
> -
> -	if (err)
> -		return _FAIL;
> -	return _SUCCESS;
> +	return err;
>  }

You just changed the logic here, are you SURE that was ok to do?


> 
>  static void sdio_deinit(struct dvobj_priv *dvobj)
> @@ -186,7 +185,7 @@ static struct dvobj_priv *sdio_dvobj_init(struct sdio_func *func)
>  	psdio = &dvobj->intf_data;
>  	psdio->func = func;
> 
> -	if (sdio_init(dvobj) != _SUCCESS)
> +	if (sdio_init(dvobj) < 0)
>  		goto free_dvobj;
> 
>  	rtw_reset_continual_io_error(dvobj);
> 
> base-commit: 9ccce092fc64d19504fa54de4fd659e279cc92e7
> --
> git-series 0.9.1
> 
> 

And that's all to remove the need for these crazy error values?  If so,
why not also remove the #defines for them as well?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] staging: rtl8723bs: use generic kernel error codes
  2021-05-04 16:17 ` Greg Kroah-Hartman
@ 2021-05-04 17:45   ` Bryan Brattlof
  2021-05-04 17:56     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 5+ messages in thread
From: Bryan Brattlof @ 2021-05-04 17:45 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-staging, linux-kernel

Sorry for the spam Greg I dropped the mailing lists from the first
email.  :(

On Tue, May 04, 2021 at 06:17:15PM +0200, Greg Kroah-Hartman wrote:
>On Tue, May 04, 2021 at 04:07:48PM +0000, Bryan Brattlof wrote:

<snip>

>> @@ -139,12 +139,11 @@ static u32 sdio_init(struct dvobj_priv *dvobj)
>>  	psdio_data->tx_block_mode = 1;
>>  	psdio_data->rx_block_mode = 1;
>>
>> +	return err;
>> +
>>  release:
>>  	sdio_release_host(func);
>> -
>> -	if (err)
>> -		return _FAIL;
>> -	return _SUCCESS;
>> +	return err;
>>  }
>
>You just changed the logic here, are you SURE that was ok to do?
>

I can't say my brain didn't bleed a little trying to keep this straight
in my head while walking through this. (For what ever reason my brain
sees negative integers as False) ¯\_(ツ)_/¯

Both the sdio_enable_func() and sdio_set_block_size() will return a
negative integer if they fail, which we evaluate as True, allowing us to
jump to release, free the card and propagate the error backwards.

If everything worked, we'll skip all the jumps until we get to the first
'return err;' statement, returning our 0 for success.

Inside sdio_dvobj_init() if we see 'anything below 0' (This probably
should be changed to 'anything True') we jump to free_dvobj where we
free the dvobj and return NULL

If I've looked at this long enough I don't thing I changed the logic.

Hopefully. :)

>
>>
>>  static void sdio_deinit(struct dvobj_priv *dvobj)
>> @@ -186,7 +185,7 @@ static struct dvobj_priv *sdio_dvobj_init(struct sdio_func *func)
>>  	psdio = &dvobj->intf_data;
>>  	psdio->func = func;
>>
>> -	if (sdio_init(dvobj) != _SUCCESS)
>> +	if (sdio_init(dvobj) < 0)
>>  		goto free_dvobj;
>>
>>  	rtw_reset_continual_io_error(dvobj);
>>
>> base-commit: 9ccce092fc64d19504fa54de4fd659e279cc92e7
>> --
>> git-series 0.9.1
>>
>>
>
>And that's all to remove the need for these crazy error values?  If so,
>why not also remove the #defines for them as well?
>

I might have over sold this patch. :)

There are quite a few functions like this still here that need to be
converted before we can get rid of the _SUCCESS and _FAIL definitions.

Would it be better if I bundled these up in a series?

--
~Bryan

>
>thanks,
>
>greg k-h


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] staging: rtl8723bs: use generic kernel error codes
  2021-05-04 17:45   ` Bryan Brattlof
@ 2021-05-04 17:56     ` Greg Kroah-Hartman
  2021-05-04 18:52       ` Bryan Brattlof
  0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2021-05-04 17:56 UTC (permalink / raw)
  To: Bryan Brattlof; +Cc: linux-staging, linux-kernel

On Tue, May 04, 2021 at 05:45:37PM +0000, Bryan Brattlof wrote:
> Sorry for the spam Greg I dropped the mailing lists from the first
> email.  :(
> 
> On Tue, May 04, 2021 at 06:17:15PM +0200, Greg Kroah-Hartman wrote:
> >On Tue, May 04, 2021 at 04:07:48PM +0000, Bryan Brattlof wrote:
> 
> <snip>
> 
> >> @@ -139,12 +139,11 @@ static u32 sdio_init(struct dvobj_priv *dvobj)
> >>  	psdio_data->tx_block_mode = 1;
> >>  	psdio_data->rx_block_mode = 1;
> >>
> >> +	return err;
> >> +
> >>  release:
> >>  	sdio_release_host(func);
> >> -
> >> -	if (err)
> >> -		return _FAIL;
> >> -	return _SUCCESS;
> >> +	return err;
> >>  }
> >
> >You just changed the logic here, are you SURE that was ok to do?
> >
> 
> I can't say my brain didn't bleed a little trying to keep this straight
> in my head while walking through this. (For what ever reason my brain
> sees negative integers as False) ¯\_(ツ)_/¯
> 
> Both the sdio_enable_func() and sdio_set_block_size() will return a
> negative integer if they fail, which we evaluate as True, allowing us to
> jump to release, free the card and propagate the error backwards.
> 
> If everything worked, we'll skip all the jumps until we get to the first
> 'return err;' statement, returning our 0 for success.
> 
> Inside sdio_dvobj_init() if we see 'anything below 0' (This probably
> should be changed to 'anything True') we jump to free_dvobj where we
> free the dvobj and return NULL
> 
> If I've looked at this long enough I don't thing I changed the logic.
> 
> Hopefully. :)

So you need to document this really well, showing that the function
whose error you changed, is being evaluated here now differently too.

> >>  static void sdio_deinit(struct dvobj_priv *dvobj)
> >> @@ -186,7 +185,7 @@ static struct dvobj_priv *sdio_dvobj_init(struct sdio_func *func)
> >>  	psdio = &dvobj->intf_data;
> >>  	psdio->func = func;
> >>
> >> -	if (sdio_init(dvobj) != _SUCCESS)
> >> +	if (sdio_init(dvobj) < 0)
> >>  		goto free_dvobj;
> >>
> >>  	rtw_reset_continual_io_error(dvobj);
> >>
> >> base-commit: 9ccce092fc64d19504fa54de4fd659e279cc92e7
> >> --
> >> git-series 0.9.1
> >>
> >>
> >
> >And that's all to remove the need for these crazy error values?  If so,
> >why not also remove the #defines for them as well?
> >
> 
> I might have over sold this patch. :)
> 
> There are quite a few functions like this still here that need to be
> converted before we can get rid of the _SUCCESS and _FAIL definitions.
> 
> Would it be better if I bundled these up in a series?

Do it one function "call-chain" at a time, and yes, a series would be
great.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] staging: rtl8723bs: use generic kernel error codes
  2021-05-04 17:56     ` Greg Kroah-Hartman
@ 2021-05-04 18:52       ` Bryan Brattlof
  0 siblings, 0 replies; 5+ messages in thread
From: Bryan Brattlof @ 2021-05-04 18:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-staging, linux-kernel

On Tue, May 04, 2021 at 07:56:35PM +0200, Greg Kroah-Hartman wrote:
>On Tue, May 04, 2021 at 05:45:37PM +0000, Bryan Brattlof wrote:
>> Sorry for the spam Greg I dropped the mailing lists from the first
>> email.  :(
>>
>> On Tue, May 04, 2021 at 06:17:15PM +0200, Greg Kroah-Hartman wrote:
>> >On Tue, May 04, 2021 at 04:07:48PM +0000, Bryan Brattlof wrote:
>>
>> <snip>
>>
>> >> @@ -139,12 +139,11 @@ static u32 sdio_init(struct dvobj_priv *dvobj)
>> >>  	psdio_data->tx_block_mode = 1;
>> >>  	psdio_data->rx_block_mode = 1;
>> >>
>> >> +	return err;
>> >> +
>> >>  release:
>> >>  	sdio_release_host(func);
>> >> -
>> >> -	if (err)
>> >> -		return _FAIL;
>> >> -	return _SUCCESS;
>> >> +	return err;
>> >>  }
>> >
>> >You just changed the logic here, are you SURE that was ok to do?
>> >
>>
>> I can't say my brain didn't bleed a little trying to keep this straight
>> in my head while walking through this. (For what ever reason my brain
>> sees negative integers as False) ¯\_(ツ)_/¯
>>
>> Both the sdio_enable_func() and sdio_set_block_size() will return a
>> negative integer if they fail, which we evaluate as True, allowing us to
>> jump to release, free the card and propagate the error backwards.
>>
>> If everything worked, we'll skip all the jumps until we get to the first
>> 'return err;' statement, returning our 0 for success.
>>
>> Inside sdio_dvobj_init() if we see 'anything below 0' (This probably
>> should be changed to 'anything True') we jump to free_dvobj where we
>> free the dvobj and return NULL
>>
>> If I've looked at this long enough I don't thing I changed the logic.
>>
>> Hopefully. :)
>
>So you need to document this really well, showing that the function
>whose error you changed, is being evaluated here now differently too.
>

Sounds good. Ill update the commit log

>
>> >>  static void sdio_deinit(struct dvobj_priv *dvobj)
>> >> @@ -186,7 +185,7 @@ static struct dvobj_priv *sdio_dvobj_init(struct sdio_func *func)
>> >>  	psdio = &dvobj->intf_data;
>> >>  	psdio->func = func;
>> >>
>> >> -	if (sdio_init(dvobj) != _SUCCESS)
>> >> +	if (sdio_init(dvobj) < 0)
>> >>  		goto free_dvobj;
>> >>
>> >>  	rtw_reset_continual_io_error(dvobj);
>> >>
>> >> base-commit: 9ccce092fc64d19504fa54de4fd659e279cc92e7
>> >> --
>> >> git-series 0.9.1
>> >>
>> >>
>> >
>> >And that's all to remove the need for these crazy error values?  If so,
>> >why not also remove the #defines for them as well?
>> >
>>
>> I might have over sold this patch. :)
>>
>> There are quite a few functions like this still here that need to be
>> converted before we can get rid of the _SUCCESS and _FAIL definitions.
>>
>> Would it be better if I bundled these up in a series?
>
>Do it one function "call-chain" at a time, and yes, a series would be
>great.
>

Thanks Greg! I'll bundle this up into a series.

--
~Bryan

>
>thanks,
>
>greg k-h


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04 16:07 [PATCH] staging: rtl8723bs: use generic kernel error codes Bryan Brattlof
2021-05-04 16:17 ` Greg Kroah-Hartman
2021-05-04 17:45   ` Bryan Brattlof
2021-05-04 17:56     ` Greg Kroah-Hartman
2021-05-04 18:52       ` Bryan Brattlof

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git