Re: [PATCH v2 1/4] staging: r8188eu: add error handling of rtw_read8

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Greg,

On 6/6/22 08:48, Greg KH wrote:
-	rtw_read8(padapter, REG_FMETHR);
+	/* FIXME: should this read be removed? */
+	res = rtw_read8(padapter, REG_FMETHR, &reg);
+	(void)res;

What is that?  We don't do "empty" lines like this in the kernel for no
good reason.  If the return value must be checked, then that's fine, but
don't do it this way.  Shouldn't the function itself return an error?

And reading a value is sometimes required by hardware in order to have
the write call go through.  But that's for PCI devices, not normally USB
devices, but we could be wrong.  I wouldn't put a FIXME in here unless
you have some plan for how to eventually solve it, otherwise someone
might just drop it without knowing why the FIXME was ever added.



Ok, I will just make this function return an error. Thanks

[snip]

diff --git a/drivers/staging/r8188eu/hal/HalPhyRf_8188e.c b/drivers/staging/r8188eu/hal/HalPhyRf_8188e.c
index b944c8071a3b..a5b7980dfcee 100644
--- a/drivers/staging/r8188eu/hal/HalPhyRf_8188e.c
+++ b/drivers/staging/r8188eu/hal/HalPhyRf_8188e.c
@@ -463,6 +463,7 @@ void _PHY_SaveADDARegisters(struct adapter *adapt, u32 *ADDAReg, u32 *ADDABackup
 	}
 }
+/* FIXME: return an error to caller */

When are these FIXME going to be resolved?  I don't like adding them for
no good reason.


After this series will go in. I really don't want to make this series huge, since ideally read errors should be passed up to callers. This driver has a lot of very deep call-chains, so fixing them in one series is just unreal

I have a plan to address these FIXMEs, that's why I've planted them.


thanks for review,


With regards,
Pavel Skripkin




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux