* Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail
2022-06-20 8:54 [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail Kate Hsuan
@ 2022-06-20 9:00 ` Greg Kroah-Hartman
2022-06-20 9:25 ` Kate Hsuan
2022-06-20 9:02 ` Hans de Goede
2022-06-20 14:05 ` Larry Finger
2 siblings, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2022-06-20 9:00 UTC (permalink / raw)
To: Kate Hsuan
Cc: Hans de Goede, Larry Finger, Phillip Potter, Michael Straube,
Fabio M . De Francesco, linux-staging, linux-kernel
On Mon, Jun 20, 2022 at 04:54:13PM +0800, Kate Hsuan wrote:
> Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b
In the future, please reference commit ids as the documentation asks, as
it's a much nicer way to understand things. Also this commit id is not
in Linus's tree or any tree that I can see, where did it come from?
> fixed and prevented dereferencing a NULL pointer through checking the
> return pointer. The NULL pointer check work properly but the return
> values (-ENOMEM on fail and 0 on success). This work fixed the return
> values to make sure the caller function will return the correct status.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
> drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index f4e9f6102539..2f8720db21d9 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -180,10 +180,8 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>
> res = rtw_alloc_hwxmits(padapter);
> - if (res) {
> - res = _FAIL;
> + if (res == _FAIL)
> goto exit;
> - }
>
> rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>
> @@ -1510,7 +1508,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
>
> pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
> if (!pxmitpriv->hwxmits)
> - return -ENOMEM;
> + return _FAIL;
No, please let's fix up the callers to properly detect normal kernel
error values and get rid of all of the crazy _FAIL and _SUCCESS values
in this driver.
>
> hwxmits = pxmitpriv->hwxmits;
>
> @@ -1528,7 +1526,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> } else {
> }
>
> - return 0;
> + return _SUCCESS;
Same here, fix up the callers if they are checking this incorrectly.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail
2022-06-20 9:00 ` Greg Kroah-Hartman
@ 2022-06-20 9:25 ` Kate Hsuan
0 siblings, 0 replies; 7+ messages in thread
From: Kate Hsuan @ 2022-06-20 9:25 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Hans de Goede, Larry Finger, Phillip Potter, Michael Straube,
Fabio M . De Francesco, linux-staging, linux-kernel
On Mon, Jun 20, 2022 at 5:01 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 20, 2022 at 04:54:13PM +0800, Kate Hsuan wrote:
> > Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> > functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b
>
> In the future, please reference commit ids as the documentation asks, as
> it's a much nicer way to understand things. Also this commit id is not
> in Linus's tree or any tree that I can see, where did it come from?
Sorry about this. I'll improve this in my v2 patch.
>
> > fixed and prevented dereferencing a NULL pointer through checking the
> > return pointer. The NULL pointer check work properly but the return
> > values (-ENOMEM on fail and 0 on success). This work fixed the return
> > values to make sure the caller function will return the correct status.
> >
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> > drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> > index f4e9f6102539..2f8720db21d9 100644
> > --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> > +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> > @@ -180,10 +180,8 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> > pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
> >
> > res = rtw_alloc_hwxmits(padapter);
> > - if (res) {
> > - res = _FAIL;
> > + if (res == _FAIL)
> > goto exit;
> > - }
> >
> > rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
> >
> > @@ -1510,7 +1508,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> >
> > pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
> > if (!pxmitpriv->hwxmits)
> > - return -ENOMEM;
> > + return _FAIL;
>
> No, please let's fix up the callers to properly detect normal kernel
> error values and get rid of all of the crazy _FAIL and _SUCCESS values
> in this driver.
Okay.
>
> >
> > hwxmits = pxmitpriv->hwxmits;
> >
> > @@ -1528,7 +1526,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> > } else {
> > }
> >
> > - return 0;
> > + return _SUCCESS;
>
> Same here, fix up the callers if they are checking this incorrectly.
>
> thanks,
>
> greg k-h
>
Thank you :)
--
BR,
Kate
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail
2022-06-20 8:54 [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail Kate Hsuan
2022-06-20 9:00 ` Greg Kroah-Hartman
@ 2022-06-20 9:02 ` Hans de Goede
2022-06-20 9:37 ` Kate Hsuan
2022-06-20 14:05 ` Larry Finger
2 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2022-06-20 9:02 UTC (permalink / raw)
To: Kate Hsuan, Larry Finger, Phillip Potter, Greg Kroah-Hartman,
Michael Straube, Fabio M . De Francesco, linux-staging,
linux-kernel
Hi Kate,
Good catch!
On 6/20/22 10:54, Kate Hsuan wrote:
> Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b
This is the commit hash from one of the stable series backports, you
should always use the commit hash from Linus' master branch which is
f94b47c6bde6 and the format for referencing commits in a commit-message is:
commit <12 char hash> ("commit subject")
so in this case this should have been:
commit f94b47c6bde6 ("staging: r8188eu: add check for kzalloc")
Note that checkpatch.pl would have complained about the wrong format
(but not the wrong hash)
> fixed and prevented dereferencing a NULL pointer through checking the
> return pointer. The NULL pointer check work properly but the return
> values (-ENOMEM on fail and 0 on success). This work fixed the return
> values to make sure the caller function will return the correct status.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
This should have a fixes tag:
Fixes: f94b47c6bde6 ("staging: r8188eu: add check for kzalloc")
But while looking up the torvalds/master branch hash I noticed
someone alreayd beat you to it. Linus' master already has
a fix for this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/r8188eu?id=5b7419ae1d208cab1e2826d473d8dab045aa75c7
So this patch can be dropped since it is a duplicate.
Regards,
hans
> ---
> drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index f4e9f6102539..2f8720db21d9 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -180,10 +180,8 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>
> res = rtw_alloc_hwxmits(padapter);
> - if (res) {
> - res = _FAIL;
> + if (res == _FAIL)
> goto exit;
> - }
>
> rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>
> @@ -1510,7 +1508,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
>
> pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
> if (!pxmitpriv->hwxmits)
> - return -ENOMEM;
> + return _FAIL;
>
> hwxmits = pxmitpriv->hwxmits;
>
> @@ -1528,7 +1526,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> } else {
> }
>
> - return 0;
> + return _SUCCESS;
> }
>
> void rtw_free_hwxmits(struct adapter *padapter)
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail
2022-06-20 9:02 ` Hans de Goede
@ 2022-06-20 9:37 ` Kate Hsuan
0 siblings, 0 replies; 7+ messages in thread
From: Kate Hsuan @ 2022-06-20 9:37 UTC (permalink / raw)
To: Hans de Goede
Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman,
Michael Straube, Fabio M . De Francesco, linux-staging,
linux-kernel
On Mon, Jun 20, 2022 at 5:02 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Kate,
>
> Good catch!
>
> On 6/20/22 10:54, Kate Hsuan wrote:
> > Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> > functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b
>
> This is the commit hash from one of the stable series backports, you
> should always use the commit hash from Linus' master branch which is
> f94b47c6bde6 and the format for referencing commits in a commit-message is:
>
> commit <12 char hash> ("commit subject")
>
> so in this case this should have been:
>
> commit f94b47c6bde6 ("staging: r8188eu: add check for kzalloc")
>
> Note that checkpatch.pl would have complained about the wrong format
> (but not the wrong hash)
Thanks for this. I'll improve my commit message.
>
> > fixed and prevented dereferencing a NULL pointer through checking the
> > return pointer. The NULL pointer check work properly but the return
> > values (-ENOMEM on fail and 0 on success). This work fixed the return
> > values to make sure the caller function will return the correct status.
> >
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
>
> This should have a fixes tag:
>
> Fixes: f94b47c6bde6 ("staging: r8188eu: add check for kzalloc")
>
> But while looking up the torvalds/master branch hash I noticed
> someone alreayd beat you to it. Linus' master already has
> a fix for this:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/r8188eu?id=5b7419ae1d208cab1e2826d473d8dab045aa75c7
>
> So this patch can be dropped since it is a duplicate.
Ah~ I have to check before starting to fix it.
Thank you :)
>
> Regards,
>
> hans
>
>
>
> > ---
> > drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> > index f4e9f6102539..2f8720db21d9 100644
> > --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> > +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> > @@ -180,10 +180,8 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> > pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
> >
> > res = rtw_alloc_hwxmits(padapter);
> > - if (res) {
> > - res = _FAIL;
> > + if (res == _FAIL)
> > goto exit;
> > - }
> >
> > rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
> >
> > @@ -1510,7 +1508,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> >
> > pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
> > if (!pxmitpriv->hwxmits)
> > - return -ENOMEM;
> > + return _FAIL;
> >
> > hwxmits = pxmitpriv->hwxmits;
> >
> > @@ -1528,7 +1526,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> > } else {
> > }
> >
> > - return 0;
> > + return _SUCCESS;
> > }
> >
> > void rtw_free_hwxmits(struct adapter *padapter)
>
--
BR,
Kate
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail
2022-06-20 8:54 [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail Kate Hsuan
2022-06-20 9:00 ` Greg Kroah-Hartman
2022-06-20 9:02 ` Hans de Goede
@ 2022-06-20 14:05 ` Larry Finger
2022-06-22 12:25 ` Phillip Potter
2 siblings, 1 reply; 7+ messages in thread
From: Larry Finger @ 2022-06-20 14:05 UTC (permalink / raw)
To: Kate Hsuan, Hans de Goede, Phillip Potter, Greg Kroah-Hartman,
Michael Straube, Fabio M . De Francesco, linux-staging,
linux-kernel
On 6/20/22 03:54, Kate Hsuan wrote:
> Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b
> fixed and prevented dereferencing a NULL pointer through checking the
> return pointer. The NULL pointer check work properly but the return
> values (-ENOMEM on fail and 0 on success). This work fixed the return
> values to make sure the caller function will return the correct status.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
> drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index f4e9f6102539..2f8720db21d9 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -180,10 +180,8 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>
> res = rtw_alloc_hwxmits(padapter);
> - if (res) {
> - res = _FAIL;
> + if (res == _FAIL)
> goto exit;
> - }
>
> rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>
This problem was fixed in mid May with commit 5b7419ae1d20 ("staging: r8188eu:
fix rtw_alloc_hwxmits error detection for now"). The fix was
@@ -178,8 +178,7 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct
adapter *padapter)
pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
- res = rtw_alloc_hwxmits(padapter);
- if (res) {
+ if (rtw_alloc_hwxmits(padapter)) {
res = _FAIL;
goto exit;
}
The "for now" part is that Phillip plans to get rid of _FAIL and _SUCCESS, and
replace the logic with a normal 1 for fail, etc.; however, this will be a major
change that must be done carefully.
In any case NACK for this patch.
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail
2022-06-20 14:05 ` Larry Finger
@ 2022-06-22 12:25 ` Phillip Potter
0 siblings, 0 replies; 7+ messages in thread
From: Phillip Potter @ 2022-06-22 12:25 UTC (permalink / raw)
To: Larry Finger
Cc: Kate Hsuan, Hans de Goede, Greg Kroah-Hartman, Michael Straube,
Fabio M . De Francesco, linux-staging, linux-kernel,
Pavel Skripkin
On Mon, Jun 20, 2022 at 09:05:17AM -0500, Larry Finger wrote:
>
> The "for now" part is that Phillip plans to get rid of _FAIL and _SUCCESS,
> and replace the logic with a normal 1 for fail, etc.; however, this will be
> a major change that must be done carefully.
>
> In any case NACK for this patch.
>
> Larry
>
>
>
Hi all,
I would be happy to work on purging _FAIL and _SUCCESS from the driver
by all means - definitely a worthwhile goal. I know Pavel said he
might take a look too though - copying him in therefore to see if we can
coordinate an approach - happy to do as much or as little as desired.
Regards,
Phil
^ permalink raw reply [flat|nested] 7+ messages in thread