[PATCH v2 2/7] cxl/region: Fix a checkpatch warning

[PATCH v2 2/7] cxl/region: Fix a checkpatch warning

[Jeongtae Park]

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>
---
 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);
-- 
2.34.1

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

[Jonathan Cameron]

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.

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);

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

[Jeongtae Park]

[-- Attachment #1: Type: text/plain, Size: 2374 bytes --]

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);
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]