Re: [PATCH v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile_read to int

Re: [PATCH v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile_read to int

[Minu Jin]

Hi Dan,

Thanks for the feedback.
I'll drop the type-change patch and reorganize the series
into two patches for v6 as you suggested.

Regarding the return type,
I’ve audited the call sites including set_qos(), update_attrib(),
and rtw_xmitframe_coalesce(), along with their callers rtw_xmit() and xmit_frames().

Since these functions handle errors immediately and exit,
I initially thought a negative return value would be safe.

I'm curious if I missed a specific scenario in the current code,
or if your advice is more about general defensive programming—such as avoiding
future risks like implicit casting or pointer arithmetic issues.

I’d appreciate your insight on this
so I can better understand the preferred approach in the kernel.

Best regards

Minu Jin

Re: [PATCH v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile_read to int

[Dan Carpenter]

On Thu, Jan 22, 2026 at 10:23:11PM +0900, Minu Jin wrote:
> Hi Dan,
> 
> Thanks for the feedback.
> I'll drop the type-change patch and reorganize the series
> into two patches for v6 as you suggested.
> 
> Regarding the return type,
> I’ve audited the call sites including set_qos(), update_attrib(),
> and rtw_xmitframe_coalesce(), along with their callers rtw_xmit() and xmit_frames().

The issue is the ordering of the patches.  You introduced a negative
return and then fixed up the callers later.  There was only one
file which used the return but it treated the negative error code
like a byte count.

You can't break stuff and then fix it later in the series because
it break git bisect.

regards,
dan carpenter

Re: [PATCH v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile_read to int

[Minu Jin]

Hi Dan,

Sorry, I just realized what you meant.

You were suggesting to keep the 'Fix' (including the necessary type 
change for error codes) and 'Cleanup' (formatting and style) separate 
to make the fix easier to backport.

I misunderstood "leave the type alone" as "never change the type." 
I'll reorganize the series into the two patches 
you suggested and send v6 shortly.

Thanks for the clarification!

Best regards

Minu Jin

Re: [PATCH v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile_read to int

[Greg KH]

On Thu, Jan 22, 2026 at 11:23:06PM +0900, Minu Jin wrote:
> Hi Dan,
> 
> Sorry, I just realized what you meant.
> 
> You were suggesting to keep the 'Fix' (including the necessary type 
> change for error codes) and 'Cleanup' (formatting and style) separate 
> to make the fix easier to backport.
> 
> I misunderstood "leave the type alone" as "never change the type." 
> I'll reorganize the series into the two patches 
> you suggested and send v6 shortly.
> 
> Thanks for the clarification!

Please always keep the context of an email, otherwise the reader has no
idea what is going on.

Remember, some of us get over 1000 emails a day to deal with, context
matters.

thanks,

greg k-h

[PATCH v5 0/3] staging: rtl8723bs: improve error handling in _rtw_pktfile_read

[Minu Jin]

This patch series improves the error handling and data integrity of 
_rtw_pktfile_read() by preventing unsafe partial reads and ensuring 
proper error propagation.

Previously, _rtw_pktfile_read() allowed partial reads when requested 
data was insufficient, which could lead to incomplete packet parsing 
and potential errors in callers. This series updates the function to 
only update internal pointers when a full read is successful, and 
updates all callers to handle the new error codes correctly.

I do not have the physical hardware to test this, so I have carefully 
reviewed every place where _rtw_pktfile_read() is used in this driver. 
I have also confirmed that the driver builds without any errors or 
warnings with these patches applied.

Changes since v4:
    - Split the single large patch into a 3-patch series as suggested by 
      Greg Kroah-Hartman to ensure each patch performs one logical task.

    - Patch 1: Refactor return type to 'int' and replace non-standard 'uint' 
      parameter with 'unsigned int' for better type consistency and to 
      prepare for error propagation.

    - Patch 2: Add missing error checks and update existing handlers to 
      correctly recognize negative error codes. This includes inserting 
      new checks after _rtw_pktfile_read() and updating existing 
      '== _FAIL' logic to '!= _SUCCESS' to ensure all failures are caught.

    - Patch 3: Implement the core logic to prevent partial reads and 
      return -EINVAL when data is insufficient.

-- 
2.43.0

[PATCH v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile_read to int

[Minu Jin]

The current return type of _rtw_pktfile_read() is uint,
which makes it impossible to propagate negative error codes
from internal calls (skb_copy_bits()).

In preparation for returning proper error codes
(eg, skb_copy_bits return -EFAULT when error occurs)
when data is insufficient or copying fails, change the function's
return type to int.

Additionally, update the type of the 'rlen' parameter, 'len' variable
from 'uint' to 'unsigned int' to comply with the kernel coding style
as suggested by Andy Shevchenko.

Signed-off-by: Minu Jin <s9430939@naver.com>
---
 drivers/staging/rtl8723bs/include/xmit_osdep.h |  2 +-
 drivers/staging/rtl8723bs/os_dep/xmit_linux.c  | 14 +++++++++-----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8723bs/include/xmit_osdep.h b/drivers/staging/rtl8723bs/include/xmit_osdep.h
index 8704dced593a..880344bffe2f 100644
--- a/drivers/staging/rtl8723bs/include/xmit_osdep.h
+++ b/drivers/staging/rtl8723bs/include/xmit_osdep.h
@@ -35,7 +35,7 @@ void rtw_os_xmit_resource_free(struct adapter *padapter, struct xmit_buf *pxmitb
 
 extern uint rtw_remainder_len(struct pkt_file *pfile);
 extern void _rtw_open_pktfile(struct sk_buff *pkt, struct pkt_file *pfile);
-extern uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen);
+int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int rlen);
 extern signed int rtw_endofpktfile(struct pkt_file *pfile);
 
 extern void rtw_os_pkt_complete(struct adapter *padapter, struct sk_buff *pkt);
diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
index 944b9c724b32..ea54a573e025 100644
--- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
@@ -21,15 +21,19 @@ void _rtw_open_pktfile(struct sk_buff *pktptr, struct pkt_file *pfile)
 	pfile->cur_buffer = pfile->buf_start;
 }
 
-uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
+int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int rlen)
 {
-	uint	len = 0;
+	unsigned int len;
+	int ret;
 
 	len =  rtw_remainder_len(pfile);
-	len = (rlen > len) ? len : rlen;
+	len = (rlen > len) ? len : rlen;
 
-	if (rmem)
-		skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
+	if (rmem) {
+		ret = skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
+		if (ret < 0)
+			return ret;
+	}
 
 	pfile->cur_addr += len;
 	pfile->pkt_len -= len;
-- 
2.43.0

Re: [PATCH v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile_read to int

[Dan Carpenter]

On Thu, Jan 22, 2026 at 01:14:48PM +0900, Minu Jin wrote:
> The current return type of _rtw_pktfile_read() is uint,
> which makes it impossible to propagate negative error codes
> from internal calls (skb_copy_bits()).
> 
> In preparation for returning proper error codes
> (eg, skb_copy_bits return -EFAULT when error occurs)
> when data is insufficient or copying fails, change the function's
> return type to int.
> 
> Additionally, update the type of the 'rlen' parameter, 'len' variable
> from 'uint' to 'unsigned int' to comply with the kernel coding style
> as suggested by Andy Shevchenko.
> 
> Signed-off-by: Minu Jin <s9430939@naver.com>
> ---
>  drivers/staging/rtl8723bs/include/xmit_osdep.h |  2 +-
>  drivers/staging/rtl8723bs/os_dep/xmit_linux.c  | 14 +++++++++-----
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/include/xmit_osdep.h b/drivers/staging/rtl8723bs/include/xmit_osdep.h
> index 8704dced593a..880344bffe2f 100644
> --- a/drivers/staging/rtl8723bs/include/xmit_osdep.h
> +++ b/drivers/staging/rtl8723bs/include/xmit_osdep.h
> @@ -35,7 +35,7 @@ void rtw_os_xmit_resource_free(struct adapter *padapter, struct xmit_buf *pxmitb
>  
>  extern uint rtw_remainder_len(struct pkt_file *pfile);
>  extern void _rtw_open_pktfile(struct sk_buff *pkt, struct pkt_file *pfile);
> -extern uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen);
> +int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int rlen);
>  extern signed int rtw_endofpktfile(struct pkt_file *pfile);
>  
>  extern void rtw_os_pkt_complete(struct adapter *padapter, struct sk_buff *pkt);
> diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> index 944b9c724b32..ea54a573e025 100644
> --- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> +++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> @@ -21,15 +21,19 @@ void _rtw_open_pktfile(struct sk_buff *pktptr, struct pkt_file *pfile)
>  	pfile->cur_buffer = pfile->buf_start;
>  }
>  
> -uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
> +int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int rlen)
>  {
> -	uint	len = 0;
> +	unsigned int len;
> +	int ret;
>  
>  	len =  rtw_remainder_len(pfile);
> -	len = (rlen > len) ? len : rlen;
> +	len = (rlen > len) ? len : rlen;

How did you generate this diff?  The before and after lines are
exactly the same.

>  
> -	if (rmem)
> -		skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
> +	if (rmem) {
> +		ret = skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
> +		if (ret < 0)
> +			return ret;

No, you can't do this or it will cause a bug in rtw_xmitframe_coalesce().
So that also means don't change the return type from uint to int in this
patch either.

Normally, we would do the bugfix first and then the cleanup in the
follow on patches.  That way we could backport the fix and leave the
cleanup.  It doesn't matter in a practical sense in this code but
that's the better way.

regards,
dan carpenter

> +	}
>  
>  	pfile->cur_addr += len;
>  	pfile->pkt_len -= len;
> -- 
> 2.43.0