Re: [v3] [media] Use common error handling code in 19 functions
> @@ -656,18 +656,18 @@ static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev,
> tsfeed->priv = filter;
>
> ret = tsfeed->set(tsfeed, feed->pid, ts_type, ts_pes, timeout);
> - if (ret < 0) {
> - dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> - return ret;
> - }
> + if (ret < 0)
> + goto release_feed;
>
> ret = tsfeed->start_filtering(tsfeed);
> - if (ret < 0) {
> - dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> - return ret;
> - }
> + if (ret < 0)
> + goto release_feed;
>
> return 0;
> +
> +release_feed:
> + dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> + return ret;
> }
>
> There's *nothing* wrong with the above. It works fine,
I can agree to this view in principle according to the required control flow.
> avoids goto
How does this wording fit to information from the section
“7) Centralized exiting of functions” in the document “coding-style.rst”?
> and probably even produce the same code, as gcc will likely optimize it.
Would you like to clarify the current situation around supported
software optimisations any more?
> It is also easier to review, as the error handling is closer
> to the code.
Do we stumble on different coding style preferences once more?
> On the other hand, there's nothing wrong on taking the approach
> you're proposing.
Thanks for another bit of positive feedback.
> In the end, using goto or not on error handling like the above is
> a matter of personal taste - and taste changes with time
Do Linux guidelines need any adjustments?
> and with developer. I really don't have time to keep reviewing patches
> that are just churning the code just due to someone's personal taste.
I tried to apply another general source code transformation pattern.
> I'm pretty sure if I start accepting things like that,
> someone else would be on some future doing patches just reverting it,
> and I would be likely having to apply them too.
Why?
I hope also that the source code can be kept consistent to some degree.
> So, except if the patch is really fixing something - e.g. a broken
> error handling code, I'll just ignore such patches and mark as
> rejected without further notice/comments from now on.
I would find such a communication style questionable.
Do you distinguish between bug fixes and possible corrections for
other error categories (or software weaknesses)?
Regards,
Markus
Re: [v3] [media] Use common error handling code in 19 functions
Em Fri, 4 May 2018 18:08:59 +0200
SF Markus Elfring escreveu:
> > Adjust jump targets so that a bit of exception handling can be better
> > reused at the end of these functions.
>
> Why was this update suggestion rejected once more a moment ago?
>
> https://patchwork.linuxtv.org/patch/47827/
> lkml.kernel.org/r/<57ef3a56-2578-1d5f-1268-348b49b0c...@users.sourceforge.net>
> https://lkml.org/lkml/2018/3/9/823
Taking just the first diff there as an example:
diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c
index 61a750fae465..17d05b05fa9d 100644
--- a/drivers/media/dvb-core/dmxdev.c
+++ b/drivers/media/dvb-core/dmxdev.c
@@ -656,18 +656,18 @@ static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev,
tsfeed->priv = filter;
ret = tsfeed->set(tsfeed, feed->pid, ts_type, ts_pes, timeout);
- if (ret < 0) {
- dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
- return ret;
- }
+ if (ret < 0)
+ goto release_feed;
ret = tsfeed->start_filtering(tsfeed);
- if (ret < 0) {
- dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
- return ret;
- }
+ if (ret < 0)
+ goto release_feed;
return 0;
+
+release_feed:
+ dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
+ return ret;
}
There's *nothing* wrong with the above. It works fine, avoids goto
and probably even produce the same code, as gcc will likely optimize
it. It is also easier to review, as the error handling is closer
to the code. On the other hand, there's nothing wrong on taking
the approach you're proposing.
In the end, using goto or not on error handling like the above is
a matter of personal taste - and taste changes with time and with
developer. I really don't have time to keep reviewing patches that
are just churning the code just due to someone's personal taste.
I'm pretty sure if I start accepting things like that, someone
else would be on some future doing patches just reverting it,
and I would be likely having to apply them too.
So, except if the patch is really fixing something - e.g. a broken
error handling code, I'll just ignore such patches and mark as
rejected without further notice/comments from now on.
Thanks,
Mauro
Re: [v3] [media] Use common error handling code in 19 functions
> Adjust jump targets so that a bit of exception handling can be better > reused at the end of these functions. Why was this update suggestion rejected once more a moment ago? https://patchwork.linuxtv.org/patch/47827/ lkml.kernel.org/r/<57ef3a56-2578-1d5f-1268-348b49b0c...@users.sourceforge.net> https://lkml.org/lkml/2018/3/9/823 Would you like to integrate such a source code transformation after any further adjustments? Regards, Markus
Re: [PATCH v3] [media] Use common error handling code in 19 functions
Hi Markus,
Thank you for the patch.
On 9.03.2018 22:10, SF Markus Elfring wrote:
> From: Markus Elfring
> Date: Fri, 9 Mar 2018 21:00:12 +0100
>
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of these functions.
>
> This issue was partly detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring
> ---
>
> v3:
> Laurent Pinchart and Todor Tomov requested a few adjustments.
> Updates were rebased on source files from Linux next-20180308.
>
> v2:
> Hans Verkuil insisted on patch squashing. Thus several changes
> were recombined based on source files from Linux next-20180216.
>
> The implementation of the function "tda8261_set_params" was improved
> after a notification by Christoph Böhmwalder on 2017-09-26.
>
> drivers/media/dvb-core/dmxdev.c| 16
> drivers/media/dvb-frontends/tda1004x.c | 20 ++
> drivers/media/dvb-frontends/tda8261.c | 19 ++
> drivers/media/pci/bt8xx/dst.c | 19 ++
> drivers/media/pci/bt8xx/dst_ca.c | 30 +++
> drivers/media/pci/cx88/cx88-input.c| 17 +
> drivers/media/platform/omap3isp/ispvideo.c | 28 ++
> .../media/platform/qcom/camss-8x16/camss-csid.c| 19 +-
> drivers/media/tuners/tuner-xc2028.c| 30 +++
> drivers/media/usb/cpia2/cpia2_usb.c| 13 ---
> drivers/media/usb/gspca/gspca.c| 17 +
> drivers/media/usb/gspca/sn9c20x.c | 17 +
> drivers/media/usb/pvrusb2/pvrusb2-ioread.c | 10 +++--
> drivers/media/usb/tm6000/tm6000-cards.c| 7 ++--
> drivers/media/usb/tm6000/tm6000-dvb.c | 11 --
> drivers/media/usb/tm6000/tm6000-video.c| 13 ---
> drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c | 13 +++
> drivers/media/usb/ttusb-dec/ttusb_dec.c| 43
> --
> 18 files changed, 171 insertions(+), 171 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss-8x16/camss-csid.c
> b/drivers/media/platform/qcom/camss-8x16/camss-csid.c
> index 64df82817de3..5c790d8c1f80 100644
> --- a/drivers/media/platform/qcom/camss-8x16/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss-8x16/camss-csid.c
> @@ -328,16 +328,12 @@ static int csid_set_power(struct v4l2_subdev *sd, int
> on)
> return ret;
>
> ret = csid_set_clock_rates(csid);
> - if (ret < 0) {
> - regulator_disable(csid->vdda);
> - return ret;
> - }
> + if (ret < 0)
> + goto disable_regulator;
>
> ret = camss_enable_clocks(csid->nclocks, csid->clock, dev);
> - if (ret < 0) {
> - regulator_disable(csid->vdda);
> - return ret;
> - }
> + if (ret < 0)
> + goto disable_regulator;
>
> enable_irq(csid->irq);
>
> @@ -345,8 +341,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
> if (ret < 0) {
> disable_irq(csid->irq);
> camss_disable_clocks(csid->nclocks, csid->clock);
> - regulator_disable(csid->vdda);
> - return ret;
> + goto disable_regulator;
> }
>
> hw_version = readl_relaxed(csid->base + CAMSS_CSID_HW_VERSION);
> @@ -358,6 +353,10 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
> }
>
> return ret;
> +
> +disable_regulator:
> + regulator_disable(csid->vdda);
> + return ret;
> }
>
For the QComm CAMSS part of the patch:
Acked-by: Todor Tomov
--
Best regards,
Todor Tomov
[PATCH v3] [media] Use common error handling code in 19 functions
From: Markus Elfring
Date: Fri, 9 Mar 2018 21:00:12 +0100
Adjust jump targets so that a bit of exception handling can be better
reused at the end of these functions.
This issue was partly detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
v3:
Laurent Pinchart and Todor Tomov requested a few adjustments.
Updates were rebased on source files from Linux next-20180308.
v2:
Hans Verkuil insisted on patch squashing. Thus several changes
were recombined based on source files from Linux next-20180216.
The implementation of the function "tda8261_set_params" was improved
after a notification by Christoph Böhmwalder on 2017-09-26.
drivers/media/dvb-core/dmxdev.c| 16
drivers/media/dvb-frontends/tda1004x.c | 20 ++
drivers/media/dvb-frontends/tda8261.c | 19 ++
drivers/media/pci/bt8xx/dst.c | 19 ++
drivers/media/pci/bt8xx/dst_ca.c | 30 +++
drivers/media/pci/cx88/cx88-input.c| 17 +
drivers/media/platform/omap3isp/ispvideo.c | 28 ++
.../media/platform/qcom/camss-8x16/camss-csid.c| 19 +-
drivers/media/tuners/tuner-xc2028.c| 30 +++
drivers/media/usb/cpia2/cpia2_usb.c| 13 ---
drivers/media/usb/gspca/gspca.c| 17 +
drivers/media/usb/gspca/sn9c20x.c | 17 +
drivers/media/usb/pvrusb2/pvrusb2-ioread.c | 10 +++--
drivers/media/usb/tm6000/tm6000-cards.c| 7 ++--
drivers/media/usb/tm6000/tm6000-dvb.c | 11 --
drivers/media/usb/tm6000/tm6000-video.c| 13 ---
drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c | 13 +++
drivers/media/usb/ttusb-dec/ttusb_dec.c| 43 --
18 files changed, 171 insertions(+), 171 deletions(-)
diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c
index 61a750fae465..17d05b05fa9d 100644
--- a/drivers/media/dvb-core/dmxdev.c
+++ b/drivers/media/dvb-core/dmxdev.c
@@ -656,18 +656,18 @@ static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev,
tsfeed->priv = filter;
ret = tsfeed->set(tsfeed, feed->pid, ts_type, ts_pes, timeout);
- if (ret < 0) {
- dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
- return ret;
- }
+ if (ret < 0)
+ goto release_feed;
ret = tsfeed->start_filtering(tsfeed);
- if (ret < 0) {
- dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
- return ret;
- }
+ if (ret < 0)
+ goto release_feed;
return 0;
+
+release_feed:
+ dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
+ return ret;
}
static int dvb_dmxdev_filter_start(struct dmxdev_filter *filter)
diff --git a/drivers/media/dvb-frontends/tda1004x.c
b/drivers/media/dvb-frontends/tda1004x.c
index 58e3beff5adc..85ca111fc8c4 100644
--- a/drivers/media/dvb-frontends/tda1004x.c
+++ b/drivers/media/dvb-frontends/tda1004x.c
@@ -1299,20 +1299,22 @@ struct dvb_frontend* tda10045_attach(const struct
tda1004x_config* config,
id = tda1004x_read_byte(state, TDA1004X_CHIPID);
if (id < 0) {
printk(KERN_ERR "tda10045: chip is not answering. Giving
up.\n");
- kfree(state);
- return NULL;
+ goto free_state;
}
if (id != 0x25) {
printk(KERN_ERR "Invalid tda1004x ID = 0x%02x. Can't
proceed\n", id);
- kfree(state);
- return NULL;
+ goto free_state;
}
/* create dvb_frontend */
memcpy(&state->frontend.ops, &tda10045_ops, sizeof(struct
dvb_frontend_ops));
state->frontend.demodulator_priv = state;
return &state->frontend;
+
+free_state:
+ kfree(state);
+ return NULL;
}
static const struct dvb_frontend_ops tda10046_ops = {
@@ -1369,19 +1371,21 @@ struct dvb_frontend* tda10046_attach(const struct
tda1004x_config* config,
id = tda1004x_read_byte(state, TDA1004X_CHIPID);
if (id < 0) {
printk(KERN_ERR "tda10046: chip is not answering. Giving
up.\n");
- kfree(state);
- return NULL;
+ goto free_state;
}
if (id != 0x46) {
printk(KERN_ERR "Invalid tda1004x ID = 0x%02x. Can't
proceed\n", id);
- kfree(state);
- return NULL;
+ goto free_state;
}
/* create dvb_frontend */
memcpy(&state->frontend.ops, &tda10046_ops, sizeof(struct
dvb_frontend_ops));
state->frontend.demodulator_priv = state;
return &state->frontend;
+
+free_state:
+ kfree(state);
+ return NULL;
}
module_param(debug, int, 0644);
diff --git a/drivers/media/dvb-frontends/tda8261.c
b/d
