[PATCH 0/2] R-Car Display Unit: Fine-tuning for some function implementations

[PATCH 0/2] R-Car Display Unit: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 24 Oct 2017 17:55:43 +0200

Two update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Use common error handling code in rcar_du_encoders_init()
  Adjust 14 checks for null pointers

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c    |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c     |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c     | 22 +++++++++++-----------
 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_plane.c   |  6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c     |  6 +++---
 8 files changed, 22 insertions(+), 22 deletions(-)

-- 
2.14.3

[PATCH 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 24 Oct 2017 17:16:09 +0200

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 566d1a948c8f..d2c80cc9f8ee 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -377,10 +377,8 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 		int ret;
 
 		ret = of_graph_parse_endpoint(ep_node, &ep);
-		if (ret < 0) {
-			of_node_put(ep_node);
-			return ret;
-		}
+		if (ret < 0)
+			goto put_node;
 
 		/* Find the output route corresponding to the port number. */
 		for (i = 0; i < RCAR_DU_OUTPUT_MAX; ++i) {
@@ -401,10 +399,8 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 		/* Process the output pipeline. */
 		ret = rcar_du_encoders_init_one(rcdu, output, &ep);
 		if (ret < 0) {
-			if (ret == -EPROBE_DEFER) {
-				of_node_put(ep_node);
-				return ret;
-			}
+			if (ret == -EPROBE_DEFER)
+				goto put_node;
 
 			continue;
 		}
@@ -413,6 +409,10 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 	}
 
 	return num_encoders;
+
+put_node:
+	of_node_put(ep_node);
+	return ret;
 }
 
 static int rcar_du_properties_init(struct rcar_du_device *rcdu)
-- 
2.14.3

[PATCH 2/2] drm/rcar-du: Adjust 14 checks for null pointers

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 24 Oct 2017 17:47:37 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c    | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c     | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c     | 6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_plane.c   | 6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c     | 6 +++---
 8 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 301ea1a8018e..67567af6e348 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -410,7 +410,7 @@ void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
 	rcrtc->event = NULL;
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
-	if (event == NULL)
+	if (!event)
 		return;
 
 	spin_lock_irqsave(&dev->event_lock, flags);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index d2f29e6b1112..cf80476783bf 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -318,7 +318,7 @@ static int rcar_du_probe(struct platform_device *pdev)
 
 	/* Allocate and initialize the R-Car device structure. */
 	rcdu = devm_kzalloc(&pdev->dev, sizeof(*rcdu), GFP_KERNEL);
-	if (rcdu == NULL)
+	if (!rcdu)
 		return -ENOMEM;
 
 	rcdu->dev = &pdev->dev;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
index ba8d2804c1d1..8a9f7fa69ecc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
@@ -166,7 +166,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 	int ret;
 
 	renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);
-	if (renc == NULL)
+	if (!renc)
 		return -ENOMEM;
 
 	renc->output = output;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index d2c80cc9f8ee..f87c11a3df6c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -181,7 +181,7 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
 	unsigned int i;
 
 	format = rcar_du_format_info(mode_cmd->pixel_format);
-	if (format == NULL) {
+	if (!format) {
 		dev_dbg(dev->dev, "unsupported pixel format %08x\n",
 			mode_cmd->pixel_format);
 		return ERR_PTR(-EINVAL);
@@ -419,7 +419,7 @@ static int rcar_du_properties_init(struct rcar_du_device *rcdu)
 {
 	rcdu->props.alpha =
 		drm_property_create_range(rcdu->ddev, 0, "alpha", 0, 255);
-	if (rcdu->props.alpha == NULL)
+	if (!rcdu->props.alpha)
 		return -ENOMEM;
 
 	/*
@@ -430,7 +430,7 @@ static int rcar_du_properties_init(struct rcar_du_device *rcdu)
 	rcdu->props.colorkey =
 		drm_property_create_range(rcdu->ddev, 0, "colorkey",
 					  0, 0x01ffffff);
-	if (rcdu->props.colorkey == NULL)
+	if (!rcdu->props.colorkey)
 		return -ENOMEM;
 
 	return 0;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c b/drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
index b373ad48ef5f..189e71b3d5d4 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
@@ -63,7 +63,7 @@ int rcar_du_lvds_connector_init(struct rcar_du_device *rcdu,
 	int ret;
 
 	rcon = devm_kzalloc(rcdu->dev, sizeof(*rcon), GFP_KERNEL);
-	if (rcon == NULL)
+	if (!rcon)
 		return -ENOMEM;
 
 	connector = &rcon->connector;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c b/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
index 12d22f3db1af..ab2576e5d700 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
@@ -253,7 +253,7 @@ int rcar_du_lvdsenc_init(struct rcar_du_device *rcdu)
 
 	for (i = 0; i < rcdu->info->num_lvds; ++i) {
 		lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
-		if (lvds == NULL)
+		if (!lvds)
 			return -ENOMEM;
 
 		lvds->dev = rcdu;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index 61833cc1c699..f48a990214ed 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -584,7 +584,7 @@ static int rcar_du_plane_atomic_check(struct drm_plane *plane,
 	}
 
 	rstate->format = rcar_du_format_info(state->fb->format->format);
-	if (rstate->format == NULL) {
+	if (!rstate->format) {
 		dev_dbg(rcdu->dev, "%s: unsupported format %08x\n", __func__,
 			state->fb->format->format);
 		return -EINVAL;
@@ -637,7 +637,7 @@ rcar_du_plane_atomic_duplicate_state(struct drm_plane *plane)
 
 	state = to_rcar_plane_state(plane->state);
 	copy = kmemdup(state, sizeof(*state), GFP_KERNEL);
-	if (copy == NULL)
+	if (!copy)
 		return NULL;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, &copy->state);
@@ -662,7 +662,7 @@ static void rcar_du_plane_reset(struct drm_plane *plane)
 	}
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state == NULL)
+	if (!state)
 		return;
 
 	state->hwindex = -1;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 2c96147bc444..e8e261362d9a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -283,7 +283,7 @@ static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
 	}
 
 	rstate->format = rcar_du_format_info(state->fb->format->format);
-	if (rstate->format == NULL) {
+	if (!rstate->format) {
 		dev_dbg(rcdu->dev, "%s: unsupported format %08x\n", __func__,
 			state->fb->format->format);
 		return -EINVAL;
@@ -323,7 +323,7 @@ rcar_du_vsp_plane_atomic_duplicate_state(struct drm_plane *plane)
 
 	state = to_rcar_vsp_plane_state(plane->state);
 	copy = kmemdup(state, sizeof(*state), GFP_KERNEL);
-	if (copy == NULL)
+	if (!copy)
 		return NULL;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, &copy->state);
@@ -348,7 +348,7 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane)
 	}
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state == NULL)
+	if (!state)
 		return;
 
 	state->alpha = 255;
-- 
2.14.3

Re: [PATCH 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[Dan Carpenter]

This is a subtle thing but my preference on this type of thing is the
way the original code is written.  I'm still slightly annoyed that
someone once made me rewrite a patch using the new style...  But anyways
I guess other people sometimes disagree with me.

Unwinding is for when you allocate five things in a row.  You have
to undo four if the last allocation fails.  But say you have to take a
lock part way through and drop it before the end of the function.  The
lock/unlock is not part of the list of five resources that you want the
function to take so it doesn't belong in the unwind code.

If you add the lock/unlock to the unwind code, then it makes things a
bit tricky because then you have to do funny things like:

free_four:
	free(four);
	goto free_three:  <-- little bunny hop
unlock:                   <-- less useful label
	unlock();
free_three:
	free_three();
free_two:
	free(two);
free_one:
	free(one);

	return ret;

It's better to just do the unlocking before the goto.  That way the
lock and unlock are close together.

	if (!four) {
		unlock();
		ret = -EFAIL;
		goto free_three;
	}

Of course, having a big unlock label makes sense if you take a lock at
the start of the function and need to drop it at the end.  But in this
case we are taking a  lock then dropping it, and taking the next, then
dropping it and so on.  It's a different situation.

regards,
dan carpenter

Re: [PATCH 2/2] drm/rcar-du: Adjust 14 checks for null pointers

[Dan Carpenter]

On Tue, Oct 24, 2017 at 06:02:30PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 24 Oct 2017 17:47:37 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The script “checkpatch.pl” pointed information out like the following.
> 
> Comparison to NULL could be written !…
> 
> Thus fix the affected source code places.
> 

This one is fine except for the commit message.

regards,
dan carpenter

Re: Clarification for approaches around exception handling

[SF Markus Elfring]

> But anyways I guess other people sometimes disagree with me.

Am I one of them?   ;-)


> Unwinding is for when you allocate five things in a row.

This is a general issue.

I find that it is also needed in this function as usual.


> You have to undo four if the last allocation fails.

Concrete numbers might help to clarify another example.


> But say you have to take a lock part way through and drop it before
> the end of the function.  The lock/unlock is not part of the list
> of five resources that you want the function to take so it doesn't
> belong in the unwind code.

Such a view is useful to some degree.


> If you add the lock/unlock to the unwind code, then it makes things a
> bit tricky because then you have to do funny things like:
> 
> free_four:
> 	free(four);
> 	goto free_three:  <-- little bunny hop
> unlock:                   <-- less useful label
> 	unlock();
> free_three:
> 	free_three();
> free_two:
> 	free(two);
> free_one:
> 	free(one);
> 
> 	return ret;
> 
> It's better to just do the unlocking before the goto.

I would prefer to store such an action also only so often in the code
as it is really required.


> That way the lock and unlock are close together.

It might look nice occasionally.


> 	if (!four) {
> 		unlock();
> 		ret = -EFAIL;
> 		goto free_three;
> 	}
> 
> Of course, having a big unlock label makes sense if you take a lock at
> the start of the function and need to drop it at the end.  But in this
> case we are taking a  lock then dropping it, and taking the next, then
> dropping it and so on.  It's a different situation.

Lock scopes can interfere with a preferred control flow, can't they?


I have got the impression that your detailed reply could have been
more appropriate for update suggestions around other software modules.

Regards,
Markus

Re: drm/rcar-du: Adjust 14 checks for null pointers

[SF Markus Elfring]

>> The script “checkpatch.pl” pointed information out like the following.
>>
>> Comparison to NULL could be written !…
>>
>> Thus fix the affected source code places.
>>
> 
> This one is fine

This kind of feedback is nice.


> except for the commit message.

Would you like to support Unicode characters there?

Regards,
Markus

Re: drm/rcar-du: Adjust 14 checks for null pointers

[Dan Carpenter]

On Wed, Oct 25, 2017 at 08:44:38AM +0200, SF Markus Elfring wrote:
> >> The script “checkpatch.pl” pointed information out like the following.
> >>
> >> Comparison to NULL could be written !…
> >>
> >> Thus fix the affected source code places.
> >>
> > 
> > This one is fine
> 
> This kind of feedback is nice.
> 
> 
> > except for the commit message.
> 
> Would you like to support Unicode characters there?
> 

Multiple people have answered this question already and I have answered
it multiple times.

regards,
dan carpenter

Re: Unicode characters in commit messages?

[SF Markus Elfring]

>> Would you like to support Unicode characters there?
> 
> Multiple people have answered this question already and I have answered
> it multiple times.

I found the corresponding feedback not sufficient so far to reach
a final consensus.
Will this topic evolve any further?

Regards,
Markus

Re: Unicode characters in commit messages?

[Dan Carpenter]

On Wed, Oct 25, 2017 at 09:39:39AM +0200, SF Markus Elfring wrote:
> >> Would you like to support Unicode characters there?
> > 
> > Multiple people have answered this question already and I have answered
> > it multiple times.
> 
> I found the corresponding feedback not sufficient so far to reach
> a final consensus.

Markus, you really have to listen better or you're going to get banned
from more subsystems.  These long email threads are a waste of time when
we already answered your questions completely and over and over.  The
feedback was clear.

regards,
dan carpenter

Re: Unicode characters in commit messages?

[SF Markus Elfring]

> These long email threads are a waste of time

They occur for different topics.


> when we already answered your questions completely

There were some attempts for specific details.


> and over and over.

I hope that further useful adjustments can be achieved for
involved information sources.


> The feedback was clear.

I disagree on some aspects.

Regards,
Markus

Re: [PATCH 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[kbuild test robot]

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

Hi Markus,

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.14-rc6 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/R-Car-Display-Unit-Fine-tuning-for-some-function-implementations/20171026-160928
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All error/warnings (new ones prefixed by >>):

   drivers/gpu/drm/rcar-du/rcar_du_kms.c: In function 'rcar_du_encoders_init':
>> drivers/gpu/drm/rcar-du/rcar_du_kms.c:415:9: error: 'ret' undeclared (first use in this function)
     return ret;
            ^~~
   drivers/gpu/drm/rcar-du/rcar_du_kms.c:415:9: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/rcar-du/rcar_du_kms.c:416:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +/ret +415 drivers/gpu/drm/rcar-du/rcar_du_kms.c

   362	
   363	static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
   364	{
   365		struct device_node *np = rcdu->dev->of_node;
   366		struct device_node *ep_node;
   367		unsigned int num_encoders = 0;
   368	
   369		/*
   370		 * Iterate over the endpoints and create one encoder for each output
   371		 * pipeline.
   372		 */
   373		for_each_endpoint_of_node(np, ep_node) {
   374			enum rcar_du_output output;
   375			struct of_endpoint ep;
   376			unsigned int i;
   377			int ret;
   378	
   379			ret = of_graph_parse_endpoint(ep_node, &ep);
   380			if (ret < 0)
   381				goto put_node;
   382	
   383			/* Find the output route corresponding to the port number. */
   384			for (i = 0; i < RCAR_DU_OUTPUT_MAX; ++i) {
   385				if (rcdu->info->routes[i].possible_crtcs &&
   386				    rcdu->info->routes[i].port == ep.port) {
   387					output = i;
   388					break;
   389				}
   390			}
   391	
   392			if (i == RCAR_DU_OUTPUT_MAX) {
   393				dev_warn(rcdu->dev,
   394					 "port %u references unexisting output, skipping\n",
   395					 ep.port);
   396				continue;
   397			}
   398	
   399			/* Process the output pipeline. */
   400			ret = rcar_du_encoders_init_one(rcdu, output, &ep);
   401			if (ret < 0) {
   402				if (ret == -EPROBE_DEFER)
   403					goto put_node;
   404	
   405				continue;
   406			}
   407	
   408			num_encoders++;
   409		}
   410	
   411		return num_encoders;
   412	
   413	put_node:
   414		of_node_put(ep_node);
 > 415		return ret;
 > 416	}
   417	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36854 bytes --]

Re: [PATCH 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[Jani Nikula]

On Tue, 24 Oct 2017, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.

Please also look into the GCC software, which will detect that your
patch does not compile.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[Geert Uytterhoeven]

On Fri, Oct 27, 2017 at 8:45 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 24 Oct 2017, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
>> Add a jump target so that a bit of exception handling can be better reused
>> at the end of this function.
>>
>> This issue was detected by using the Coccinelle software.
>
> Please also look into the GCC software, which will detect that your
> patch does not compile.

+1 ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[Laurent Pinchart]

Hi Jani,

On Friday, 27 October 2017 21:45:17 EET Jani Nikula wrote:
> On Tue, 24 Oct 2017, SF Markus Elfring wrote:
> > Add a jump target so that a bit of exception handling can be better reused
> > at the end of this function.
> > 
> > This issue was detected by using the Coccinelle software.
> 
> Please also look into the GCC software, which will detect that your
> patch does not compile.

Just for the record, I've been bitten in the past by applying one of Markus' 
patches that seemed to make sense, only to discover later that it introduced a 
security hole. I now drop his patches altogether, so could you please keep an 
eye open to make sure none of them touching the rcar-du driver will be applied 
through drm-misc ?

-- 
Regards,

Laurent Pinchart

Re: drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[SF Markus Elfring]

> Just for the record, I've been bitten in the past by applying one of Markus' 
> patches that seemed to make sense, only to discover later that it introduced a 
> security hole.

How do you think about to take another look at the circumstances
under which a questionable commit happened in the referenced case?

A few glitches occur occasionally. They can be fixed by appropriate collaboration.


> I now drop his patches altogether, so could you please keep an eye open
> to make sure none of them touching the rcar-du driver will be applied
> through drm-misc ?

I hope that you can become interested in a more constructive software development
dialogue (also with me) again.

* Can additional ideas be taken into account here?

* Will any other contributor pick related update suggestions up?

Regards,
Markus

Re: [PATCH 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[Jani Nikula]

On Sun, 29 Oct 2017, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Jani,
>
> On Friday, 27 October 2017 21:45:17 EET Jani Nikula wrote:
>> On Tue, 24 Oct 2017, SF Markus Elfring wrote:
>> > Add a jump target so that a bit of exception handling can be better reused
>> > at the end of this function.
>> > 
>> > This issue was detected by using the Coccinelle software.
>> 
>> Please also look into the GCC software, which will detect that your
>> patch does not compile.
>
> Just for the record, I've been bitten in the past by applying one of Markus' 
> patches that seemed to make sense, only to discover later that it introduced a 
> security hole. I now drop his patches altogether, so could you please keep an 
> eye open to make sure none of them touching the rcar-du driver will be applied 
> through drm-misc ?

Ack. You're the maintainer, and we need to respect that.

In general, I'll pick up any patches that are good, but the current
track record is that Markus' patches need extra scrutiny, and many of
the patches contain subjective changes that lead to debate that is not
constructive. There's no return on investment here.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

Re: drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[SF Markus Elfring]

> In general, I'll pick up any patches that are good,

This is usual.


> but the current track record is that Markus' patches need extra scrutiny,

I find that this can be fine according to a safe review for presented
update suggestions.


> and many of the patches contain subjective changes that lead to debate

Some discussion is occasionally needed to achieve the desired change acceptance,
isn't it?


> that is not constructive.

I got an other opinion here.


> There's no return on investment here.

I hope that the view can become more positive.
How will the clarification evolve further?

Regards,
Markus

Re: [PATCH 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[Laurent Pinchart]

Hi Jani,

On Monday, 30 October 2017 11:52:07 EET Jani Nikula wrote:
> On Sun, 29 Oct 2017, Laurent Pinchart wrote:
> > On Friday, 27 October 2017 21:45:17 EET Jani Nikula wrote:
> >> On Tue, 24 Oct 2017, SF Markus Elfring wrote:
> >>> Add a jump target so that a bit of exception handling can be better
> >>> reused at the end of this function.
> >>> 
> >>> This issue was detected by using the Coccinelle software.
> >> 
> >> Please also look into the GCC software, which will detect that your
> >> patch does not compile.
> > 
> > Just for the record, I've been bitten in the past by applying one of
> > Markus' patches that seemed to make sense, only to discover later that it
> > introduced a security hole. I now drop his patches altogether, so could
> > you please keep an eye open to make sure none of them touching the
> > rcar-du driver will be applied through drm-misc ?
> 
> Ack. You're the maintainer, and we need to respect that.
> 
> In general, I'll pick up any patches that are good, but the current
> track record is that Markus' patches need extra scrutiny, and many of
> the patches contain subjective changes that lead to debate that is not
> constructive. There's no return on investment here.

That's how I see it too. If there's an important fix I'll look into it, but 
patches that have little value are just a waste of bandwidth.

-- 
Regards,

Laurent Pinchart

[PATCH v2 0/2] R-Car Display Unit: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 1 Nov 2017 16:23:45 +0100

Two update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  drm/rcar-du: Use common error handling code in rcar_du_encoders_init()
  drm/rcar-du: Adjust 14 checks for null pointers
---

v2:
Advice was taken into account from the first round of corresponding
source code review.

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c    |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c     |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c     | 24 ++++++++++++------------
 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_plane.c   |  6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c     |  6 +++---
 8 files changed, 23 insertions(+), 23 deletions(-)

-- 
2.14.3

[PATCH v2 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 1 Nov 2017 15:57:31 +0100

* Add a jump target so that a bit of exception handling can be better
  reused at the end of this function.

* Increase the scope for the variable "ret".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
The declaration for the variable "ret" was moved out of a loop.

 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 566d1a948c8f..b6b341164aab 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -365,6 +365,7 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 	struct device_node *np = rcdu->dev->of_node;
 	struct device_node *ep_node;
 	unsigned int num_encoders = 0;
+	int ret;
 
 	/*
 	 * Iterate over the endpoints and create one encoder for each output
@@ -374,13 +375,10 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 		enum rcar_du_output output;
 		struct of_endpoint ep;
 		unsigned int i;
-		int ret;
 
 		ret = of_graph_parse_endpoint(ep_node, &ep);
-		if (ret < 0) {
-			of_node_put(ep_node);
-			return ret;
-		}
+		if (ret < 0)
+			goto put_node;
 
 		/* Find the output route corresponding to the port number. */
 		for (i = 0; i < RCAR_DU_OUTPUT_MAX; ++i) {
@@ -401,10 +399,8 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 		/* Process the output pipeline. */
 		ret = rcar_du_encoders_init_one(rcdu, output, &ep);
 		if (ret < 0) {
-			if (ret == -EPROBE_DEFER) {
-				of_node_put(ep_node);
-				return ret;
-			}
+			if (ret == -EPROBE_DEFER)
+				goto put_node;
 
 			continue;
 		}
@@ -413,6 +409,10 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 	}
 
 	return num_encoders;
+
+put_node:
+	of_node_put(ep_node);
+	return ret;
 }
 
 static int rcar_du_properties_init(struct rcar_du_device *rcdu)
-- 
2.14.3

[PATCH v2 2/2] drm/rcar-du: Adjust 14 checks for null pointers

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 1 Nov 2017 16:00:46 +0100

The script "checkpatch.pl" pointed information out like the following.

Comparison to NULL could be written !...

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
The commit message was written only with ASCII characters.

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c    | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c     | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c     | 6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_plane.c   | 6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c     | 6 +++---
 8 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 301ea1a8018e..67567af6e348 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -410,7 +410,7 @@ void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
 	rcrtc->event = NULL;
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
-	if (event == NULL)
+	if (!event)
 		return;
 
 	spin_lock_irqsave(&dev->event_lock, flags);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index d2f29e6b1112..cf80476783bf 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -318,7 +318,7 @@ static int rcar_du_probe(struct platform_device *pdev)
 
 	/* Allocate and initialize the R-Car device structure. */
 	rcdu = devm_kzalloc(&pdev->dev, sizeof(*rcdu), GFP_KERNEL);
-	if (rcdu == NULL)
+	if (!rcdu)
 		return -ENOMEM;
 
 	rcdu->dev = &pdev->dev;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
index ba8d2804c1d1..8a9f7fa69ecc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
@@ -166,7 +166,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 	int ret;
 
 	renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);
-	if (renc == NULL)
+	if (!renc)
 		return -ENOMEM;
 
 	renc->output = output;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index b6b341164aab..c15aa38fe308 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -181,7 +181,7 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
 	unsigned int i;
 
 	format = rcar_du_format_info(mode_cmd->pixel_format);
-	if (format == NULL) {
+	if (!format) {
 		dev_dbg(dev->dev, "unsupported pixel format %08x\n",
 			mode_cmd->pixel_format);
 		return ERR_PTR(-EINVAL);
@@ -419,7 +419,7 @@ static int rcar_du_properties_init(struct rcar_du_device *rcdu)
 {
 	rcdu->props.alpha =
 		drm_property_create_range(rcdu->ddev, 0, "alpha", 0, 255);
-	if (rcdu->props.alpha == NULL)
+	if (!rcdu->props.alpha)
 		return -ENOMEM;
 
 	/*
@@ -430,7 +430,7 @@ static int rcar_du_properties_init(struct rcar_du_device *rcdu)
 	rcdu->props.colorkey =
 		drm_property_create_range(rcdu->ddev, 0, "colorkey",
 					  0, 0x01ffffff);
-	if (rcdu->props.colorkey == NULL)
+	if (!rcdu->props.colorkey)
 		return -ENOMEM;
 
 	return 0;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c b/drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
index b373ad48ef5f..189e71b3d5d4 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
@@ -63,7 +63,7 @@ int rcar_du_lvds_connector_init(struct rcar_du_device *rcdu,
 	int ret;
 
 	rcon = devm_kzalloc(rcdu->dev, sizeof(*rcon), GFP_KERNEL);
-	if (rcon == NULL)
+	if (!rcon)
 		return -ENOMEM;
 
 	connector = &rcon->connector;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c b/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
index 12d22f3db1af..ab2576e5d700 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
@@ -253,7 +253,7 @@ int rcar_du_lvdsenc_init(struct rcar_du_device *rcdu)
 
 	for (i = 0; i < rcdu->info->num_lvds; ++i) {
 		lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
-		if (lvds == NULL)
+		if (!lvds)
 			return -ENOMEM;
 
 		lvds->dev = rcdu;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index 61833cc1c699..f48a990214ed 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -584,7 +584,7 @@ static int rcar_du_plane_atomic_check(struct drm_plane *plane,
 	}
 
 	rstate->format = rcar_du_format_info(state->fb->format->format);
-	if (rstate->format == NULL) {
+	if (!rstate->format) {
 		dev_dbg(rcdu->dev, "%s: unsupported format %08x\n", __func__,
 			state->fb->format->format);
 		return -EINVAL;
@@ -637,7 +637,7 @@ rcar_du_plane_atomic_duplicate_state(struct drm_plane *plane)
 
 	state = to_rcar_plane_state(plane->state);
 	copy = kmemdup(state, sizeof(*state), GFP_KERNEL);
-	if (copy == NULL)
+	if (!copy)
 		return NULL;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, &copy->state);
@@ -662,7 +662,7 @@ static void rcar_du_plane_reset(struct drm_plane *plane)
 	}
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state == NULL)
+	if (!state)
 		return;
 
 	state->hwindex = -1;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 2c96147bc444..e8e261362d9a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -283,7 +283,7 @@ static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
 	}
 
 	rstate->format = rcar_du_format_info(state->fb->format->format);
-	if (rstate->format == NULL) {
+	if (!rstate->format) {
 		dev_dbg(rcdu->dev, "%s: unsupported format %08x\n", __func__,
 			state->fb->format->format);
 		return -EINVAL;
@@ -323,7 +323,7 @@ rcar_du_vsp_plane_atomic_duplicate_state(struct drm_plane *plane)
 
 	state = to_rcar_vsp_plane_state(plane->state);
 	copy = kmemdup(state, sizeof(*state), GFP_KERNEL);
-	if (copy == NULL)
+	if (!copy)
 		return NULL;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, &copy->state);
@@ -348,7 +348,7 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane)
 	}
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state == NULL)
+	if (!state)
 		return;
 
 	state->alpha = 255;
-- 
2.14.3