Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Thu, 5 Oct 2023 17:08:34 +0900
From:   Jeongtae Park <jtp.park@samsung.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc:     Dan Williams <dan.j.williams@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Dave Jiang <dave.jiang@intel.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Fan Ni <fan.ni@samsung.com>, linux-cxl@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Kyungsan Kim <ks0204.kim@samsung.com>,
        Wonjae Lee <wj28.lee@samsung.com>,
        Hojin Nam <hj96.nam@samsung.com>,
        Junhyeok Im <junhyeok.im@samsung.com>,
        Jehoon Park <jehoon.park@samsung.com>,
        Jeongtae Park <jeongtae.park@gmail.com>
Subject: Re: [PATCH v2 2/7] cxl/region: Fix a checkpatch warning
Message-ID: <ZR5vAgpn9ZfTaKFt@jtpark-7920>
MIME-Version: 1.0
In-Reply-To: <20230925110745.00001127@Huawei.com>
Content-Type: multipart/mixed;
        boundary="----E9vxk1eHFJMo0Hi_Q_rTTD2vXp5VKj0nvvVRz8vbSgBSYVSL=_9a285_"
CMS-TYPE: 102P
DLP-Filter: Pass
References: <CGME20230922113233epcas2p2cd57df10dde36b0da3b2fc5a916132e8@epcas2p2.samsung.com>
        <20230922113520.3298976-1-jtp.park@samsung.com>
        <20230925110745.00001127@Huawei.com>
Precedence: bulk

------E9vxk1eHFJMo0Hi_Q_rTTD2vXp5VKj0nvvVRz8vbSgBSYVSL=_9a285_
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline

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@samsung.com> wrote:
> 
> > WARNING: else is not generally useful after a break or return
> > 
> > Signed-off-by: Jeongtae Park <jtp.park@samsung.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.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);
> 

------E9vxk1eHFJMo0Hi_Q_rTTD2vXp5VKj0nvvVRz8vbSgBSYVSL=_9a285_
Content-Type: text/plain; charset="utf-8"


------E9vxk1eHFJMo0Hi_Q_rTTD2vXp5VKj0nvvVRz8vbSgBSYVSL=_9a285_--