linux-kernel - Re: [PATCH v2 2/7] cxl/region: Fix a checkpatch warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZR5vAgpn9ZfTaKFt@jtpark-7920>
Date:   Thu, 5 Oct 2023 17:08:34 +0900
From:   Jeongtae Park <jtp.park@...sung.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc:     Dan Williams <dan.j.williams@...el.com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        Dave Jiang <dave.jiang@...el.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Fan Ni <fan.ni@...sung.com>, linux-cxl@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kyungsan Kim <ks0204.kim@...sung.com>,
        Wonjae Lee <wj28.lee@...sung.com>,
        Hojin Nam <hj96.nam@...sung.com>,
        Junhyeok Im <junhyeok.im@...sung.com>,
        Jehoon Park <jehoon.park@...sung.com>,
        Jeongtae Park <jeongtae.park@...il.com>
Subject: Re: [PATCH v2 2/7] cxl/region: Fix a checkpatch warning

On Mon, Sep 25, 2023 at 11:07:45AM +0100, Jonathan Cameron wrote:
> On Fri, 22 Sep 2023 20:35:20 +0900
> Jeongtae Park <jtp.park@...sung.com> wrote:
> 
> > WARNING: else is not generally useful after a break or return
> > 
> > Signed-off-by: Jeongtae Park <jtp.park@...sung.com>
> > Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> 
> This one is a little ugly. I'd prefer to see the error
> condition remain out of line (vs the warning one)
> 
> 		if (!IS_ENABLED(CONFIG_CXL_REGION_INVALIDATION_TEST)) {
> 			dev_err(&cxlr->dev,
> 				"Failed ...");
> 			return -ENOXIO
> 		}
> 
> 		dev_warn_once(...
> 
> 		return 0;
> 
> Or keep the else.

I think I changed it too mechanically ignoring the meanings.
Your comment made me think about it a bit more. If we don't
really need the error message to be printed every calling,
how about remove it and modify the condition statements like
below. It would make more efficient or small codes
when 'CONFIG_CXL_REGION_INVALIDATION_TEST' is not set.

static int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
{
        if (IS_ENABLED(CONFIG_CXL_REGION_INVALIDATION_TEST)
                        && cpu_cache_has_invalidate_memregion()) {
                dev_warn_once(&cxlr->dev,
                        "Bypassing cpu_cache_invalidate_memregion() for testing!\n");
                return 0;
        }

        return cpu_cache_invalidate_memregion(IORES_DESC_CXL);
}

> 
> Not that important though as code is small enough that less
> than ideal in / out of line doesn't matter that much to readability.
> 
> Jonathan
> 
> 
> > ---
> >  drivers/cxl/core/region.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e115ba382e04..1fc9d01c1ac0 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -133,11 +133,10 @@ static int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
> >  				&cxlr->dev,
> >  				"Bypassing cpu_cache_invalidate_memregion() for testing!\n");
> >  			return 0;
> > -		} else {
> > -			dev_err(&cxlr->dev,
> > -				"Failed to synchronize CPU cache state\n");
> > -			return -ENXIO;
> >  		}
> > +
> > +		dev_err(&cxlr->dev, "Failed to synchronize CPU cache state\n");
> > +		return -ENXIO;
> >  	}
> >  
> >  	cpu_cache_invalidate_memregion(IORES_DESC_CXL);
>