On Tue, Apr 08, 2025 at 12:41:52PM +0800, Wentao Liang wrote:
The sdio_read32() calls sd_read(), but does not handle the error if sd_read() fails. This could lead to subsequent operations processing invalid data. A proper implementation can be found in sdio_readN(), which has an error handling for the sd_read().
Great, why not move to that instead?
Add error handling for the sd_read() to free tmpbuf and return error code if sd_read() fails. This ensure that the memcpy() is only performed when the read operation is successful.
Since none of the callers check for the errors, there is no need to return the error code propagated from sd_read(). Returning SDIO_ERR_VAL32 might be a better choice, which is a specialized error code for SDIO.
Again, fixing the callers would be best.
Another problem of returning propagated error code is that the error code is a s32 type value, which is not fit with the u32 type return value of the sdio_read32().
Then that too should be fixed :)
An practical option would be to go through all the callers and add error handling, which need to pass a pointer to u32 *val and return zero on success or negative on failure. It is not a better choice since will cost unnecessary effort on the error code.
I don't understand why this would be unnecessary effort, it would do the right thing, correct?
The other opion is to replace sd_read() by sd_read32(), which return an u32 type error code that can be directly used as the return value of sdio_read32(). But, it is also a bad choice to use sd_read32() in a alignment failed branch.
What do you mean by "alignment failed branch"?
Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver") Cc: stable@vger.kernel.org # v4.12+
Why is this cc: stable? Can you duplicate this problem on your system? Have you tested this change?
Signed-off-by: Wentao Liang vulab@iscas.ac.cn
v7: Fix error code and add patch explanation v6: Fix improper code to propagate error code v5: Fix error code v4: Add change log and fix error code v3: Add Cc flag v2: Change code to initialize val
drivers/staging/rtl8723bs/hal/sdio_ops.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c b/drivers/staging/rtl8723bs/hal/sdio_ops.c index 21e9f1858745..d79d41727042 100644 --- a/drivers/staging/rtl8723bs/hal/sdio_ops.c +++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c @@ -185,7 +185,12 @@ static u32 sdio_read32(struct intf_hdl *intfhdl, u32 addr) return SDIO_ERR_VAL32; ftaddr &= ~(u16)0x3;
sd_read(intfhdl, ftaddr, 8, tmpbuf);
err = sd_read(intfhdl, ftaddr, 8, tmpbuf);if (err) {kfree(tmpbuf);return SDIO_ERR_VAL32;}
Again, I think this whole "hal wrapper" should be removed instead, and not papered over like this. If you dig deep enough, it all boils down to a call to sdio_readb(), which is an 8 bit read, so the alignment issues are not a problem, and if an error happens the proper error value is returned from that saying what happened. Why not work on that like I recommended? That would allow for at least 3, if not more, layers of indirection to be removed from this driver, making it more easy to understand and maintain over time.
thanks,
greg k-h