diff mbox

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

Message ID 9fafa688-f699-c587-ef77-840efa71bf76@users.sourceforge.net (mailing list archive)
State New, archived
Headers show

Commit Message

SF Markus Elfring Oct. 24, 2017, 4:01 p.m. UTC
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(-)

Comments

Dan Carpenter Oct. 25, 2017, 6:01 a.m. UTC | #1
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
SF Markus Elfring Oct. 25, 2017, 6:35 a.m. UTC | #2
> 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
kernel test robot Oct. 26, 2017, 12:40 p.m. UTC | #3
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
Jani Nikula Oct. 27, 2017, 6:45 p.m. UTC | #4
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.
Geert Uytterhoeven Oct. 29, 2017, 11:01 a.m. UTC | #5
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
Laurent Pinchart Oct. 29, 2017, 5:12 p.m. UTC | #6
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 ?
SF Markus Elfring Oct. 29, 2017, 6:19 p.m. UTC | #7
> 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
Jani Nikula Oct. 30, 2017, 9:52 a.m. UTC | #8
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.
SF Markus Elfring Oct. 30, 2017, 10:03 a.m. UTC | #9
> 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
Laurent Pinchart Oct. 30, 2017, 1:18 p.m. UTC | #10
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.
SF Markus Elfring Nov. 1, 2017, 3:38 p.m. UTC | #11
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(-)
diff mbox

Patch

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)