Messages in this thread | | | Subject | Re: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function | From | Pavel Skripkin <> | Date | Mon, 30 Aug 2021 11:06:04 +0300 |
| |
On 8/30/21 2:18 AM, Phillip Potter wrote: > On Sun, Aug 29, 2021 at 02:54:14PM +0300, Pavel Skripkin wrote: >> On 8/29/21 12:24 AM, Phillip Potter wrote: >> > Simplify c2h_evt_hdl function by removing majority of its code. The >> > function always returns _FAIL anyway, due to the wrapper function it >> > calls always returning _FAIL. For this reason, it is better to just >> > return _FAIL directly. >> > >> > Leave the call to c2h_evt_read in place, as without it, event handling >> > semantics of the driver would be changed, despite nothing actually being >> > done with the event. >> > >> > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> >> > --- >> > drivers/staging/r8188eu/core/rtw_cmd.c | 21 +++------------------ >> > 1 file changed, 3 insertions(+), 18 deletions(-) >> > >> > diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c >> > index ce73ac7cf973..b520c6b43c03 100644 >> > --- a/drivers/staging/r8188eu/core/rtw_cmd.c >> > +++ b/drivers/staging/r8188eu/core/rtw_cmd.c >> > @@ -1854,27 +1854,12 @@ u8 rtw_c2h_wk_cmd(struct adapter *padapter, u8 *c2h_evt) >> > static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter) >> > { >> > - s32 ret = _FAIL; >> > u8 buf[16]; >> > - if (!c2h_evt) { >> > - /* No c2h event in cmd_obj, read c2h event before handling*/ >> > - if (c2h_evt_read(adapter, buf) == _SUCCESS) { >> > - c2h_evt = (struct c2h_evt_hdr *)buf; >> > + if (!c2h_evt) >> > + c2h_evt_read(adapter, buf); >> > - if (filter && !filter(c2h_evt->id)) >> > - goto exit; >> > - >> > - ret = rtw_hal_c2h_handler(adapter, c2h_evt); >> > - } >> > - } else { >> > - if (filter && !filter(c2h_evt->id)) >> > - goto exit; >> > - >> > - ret = rtw_hal_c2h_handler(adapter, c2h_evt); >> > - } >> > -exit: >> > - return ret; >> > + return _FAIL; >> >> >> Hi, Phillip! >> >> >> Do we really need to return _FAIL every time? The only one caller of >> c2h_evt_hdl() does not rely on it's return value. Shouldn't we make this >> function return void to simplify the code? > > Dear Pavel, > > Thanks for the review. Good point on the return type, I'll publish a v2 > series. > >> >> >> BTW, this function does nothing now, as I understand. It reads to local >> buffer and returns. I think, it can be removed >> >> >> >> > } >> > static void c2h_wk_callback(struct work_struct *work) >> > > > Not sure if you're referring to c2h_wk_callback or c2h_evt_hdl, but > either way, they both call (indirectly or otherwise) c2h_evt_read and > c2h_evt_clear as well as rtw_c2h_wk_cmd in the case of c2h_wk_callback. > To just delete them wholesale therefore would be unsafe I think, due to > the effect on event semantics. > > Certainly, it is possible to handle this c2h stuff by just reading from > register and ignoring though - another series is a better place for that > though I think. Admittedly, I may be talking nonsense here :-) >
I was referring to c2h_evt_hdl. I missed, that c2h_evt_read() writes to register, sorry :)
With regards, Pavel Skripkin
| |