lkml.org 
[lkml]   [2022]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] staging: r8188eu: convert rtw_pwr_wakeup to correct error code semantics
    On Mon, Jul 25, 2022 at 01:40:27PM +0300, Dan Carpenter wrote:
    > On Sun, Jul 24, 2022 at 05:30:55PM +0100, Phillip Potter wrote:
    > > Convert the rtw_pwr_wakeup function to use 0 on success and -EPERM on
    > > error - in all places where we handle this response, we use either -1 or
    > > -EPERM currently anyway, which are equivalent. Also, for other places
    > > along the same call chain where we are using -1, use -EPERM.
    > >
    >
    > I can't get behind a change to -EPERM. Try to pick an appropriate
    > error code. I'm not going to be very strict on it, but we have to at
    > least *try*.
    >
    > Probably, leave the return -1; lines alone. Fixing that seems like an
    > unrelated change. We need to do this kind of change but I got bitten by
    > it before so I want to avoid that next time. My reviews will hopefully
    > be more careful now.
    >
    > > This gets the driver closer to removal of the non-standard _SUCCESS and
    > > _FAIL definitions, which are inverted compared to the standard in-kernel
    > > error code mechanism.
    > >
    > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
    > > ---
    > > drivers/staging/r8188eu/core/rtw_p2p.c | 4 +--
    > > drivers/staging/r8188eu/core/rtw_pwrctrl.c | 10 +++---
    > > drivers/staging/r8188eu/os_dep/ioctl_linux.c | 32 ++++++++++----------
    > > 3 files changed, 23 insertions(+), 23 deletions(-)
    > >
    > > diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
    > > index c306aafa183b..bd654d4ff8b4 100644
    > > --- a/drivers/staging/r8188eu/core/rtw_p2p.c
    > > +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
    > > @@ -1888,7 +1888,7 @@ int rtw_p2p_enable(struct adapter *padapter, enum P2P_ROLE role)
    > >
    > > if (role == P2P_ROLE_DEVICE || role == P2P_ROLE_CLIENT || role == P2P_ROLE_GO) {
    > > /* leave IPS/Autosuspend */
    > > - if (rtw_pwr_wakeup(padapter) == _FAIL) {
    > > + if (rtw_pwr_wakeup(padapter)) {
    > > ret = _FAIL;
    > > goto exit;
    > > }
    >
    > Fine. The caller now changes from negative error codes to _SUCCESS/_FAIL.
    > Later we will transition this to normal error codes so we'll update it
    > to preserve the error code from rtw_pwr_wakeup() at that point.
    >
    > > diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
    > > index 930bb4aea435..e0ae0c3c51f8 100644
    > > --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
    > > +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
    > > @@ -689,7 +689,7 @@ static int rtw_wx_set_mode(struct net_device *dev, struct iw_request_info *a,
    > >
    > >
    > >
    > > - if (_FAIL == rtw_pwr_wakeup(padapter)) {
    > > + if (rtw_pwr_wakeup(padapter)) {
    > > ret = -EPERM;
    > > goto exit;
    > > }
    >
    > This code is returning negative error codes so it should preserve the
    > code from rtw_pwr_wakeup().
    >
    > ret = rtw_pwr_wakeup(padapter);
    > if (ret)
    > goto exit;
    >
    > > @@ -933,13 +933,13 @@ static int rtw_wx_set_wap(struct net_device *dev,
    > >
    > >
    > >
    > > - if (_FAIL == rtw_pwr_wakeup(padapter)) {
    > > - ret = -1;
    > > + if (rtw_pwr_wakeup(padapter)) {
    > > + ret = -EPERM;
    >
    > Same.
    >
    > > goto exit;
    > > }
    > >
    > > if (!padapter->bup) {
    > > - ret = -1;
    > > + ret = -EPERM;
    >
    > Unrelated.
    >
    > > goto exit;
    > > }
    > >
    >
    > [ snip ]
    >
    > > @@ -1252,13 +1252,13 @@ static int rtw_wx_set_essid(struct net_device *dev,
    > >
    > > uint ret = 0, len;
    > >
    > > - if (_FAIL == rtw_pwr_wakeup(padapter)) {
    > > - ret = -1;
    > > + if (rtw_pwr_wakeup(padapter)) {
    > > + ret = -EPERM;
    >
    > Preserve the error code.
    >
    > > goto exit;
    > > }
    >
    > regards,
    > dan carpenter

    Hi Dan,

    Thanks very much for the review. I'll get a V2 sent out shortly.

    Regards,
    Phil

    \
     
     \ /
      Last update: 2022-07-25 23:04    [W:3.339 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site