| Message ID | 9fafa688-f699-c587-ef77-840efa71bf76@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> 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
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
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.
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
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 ?
> 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
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.
> 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
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.
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 --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)