| Message ID | 56818B7B.8040801@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Mon, 28 Dec 2015 20:20:27 +0100 SF Markus Elfring <elfring@users.sourceforge.net> escreveu: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Mon, 28 Dec 2015 20:10:30 +0100 > > This issue was detected by using the Coccinelle software. > > Split the previous if statement at the end so that each final log statement > will eventually be performed by a direct jump to these labels. > * report_failure > * report_success > > A check repetition can be excluded for the variable "ret" at the end then. > > > Apply also two recommendations from the script "checkpatch.pl". > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/media/tuners/xc5000.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c > index e6e5e90..1360677 100644 > --- a/drivers/media/tuners/xc5000.c > +++ b/drivers/media/tuners/xc5000.c > @@ -1166,7 +1166,7 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force) > > ret = xc5000_fwupload(fe, desired_fw, fw); > if (ret != 0) > - goto err; > + goto report_failure; > > msleep(20); > > @@ -1229,18 +1229,16 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force) > /* Default to "CABLE" mode */ > ret = xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE); > if (!ret) > - break; > + goto report_success; > printk(KERN_ERR "xc5000: can't set to cable mode."); It sounds worth to avoid adding a goto here. > } > > -err: > - if (!ret) > - printk(KERN_INFO "xc5000: Firmware %s loaded and running.\n", > - desired_fw->name); > - else > - printk(KERN_CONT " - too many retries. Giving up\n"); > - > +report_failure: > + pr_cont(" - too many retries. Giving up\n"); > return ret; > +report_success: > + pr_info("xc5000: Firmware %s loaded and running.\n", desired_fw->name); > + return 0; > } > > static void xc5000_do_timer_sleep(struct work_struct *timer_sleep) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> This issue was detected by using the Coccinelle software. >> >> Split the previous if statement at the end so that each final log statement >> will eventually be performed by a direct jump to these labels. >> * report_failure >> * report_success >> >> A check repetition can be excluded for the variable "ret" at the end then. >> >> >> Apply also two recommendations from the script "checkpatch.pl". >> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >> --- >> drivers/media/tuners/xc5000.c | 16 +++++++--------- >> 1 file changed, 7 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c >> index e6e5e90..1360677 100644 >> --- a/drivers/media/tuners/xc5000.c >> +++ b/drivers/media/tuners/xc5000.c >> @@ -1166,7 +1166,7 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force) >> >> ret = xc5000_fwupload(fe, desired_fw, fw); >> if (ret != 0) >> - goto err; >> + goto report_failure; >> >> msleep(20); >> >> @@ -1229,18 +1229,16 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force) >> /* Default to "CABLE" mode */ >> ret = xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE); >> if (!ret) >> - break; >> + goto report_success; >> printk(KERN_ERR "xc5000: can't set to cable mode."); > > It sounds worth to avoid adding a goto here. Are you interested in a bit of software optimisation for the implementation of the function "xc_load_fw_and_init_tuner"? >> } >> >> -err: >> - if (!ret) >> - printk(KERN_INFO "xc5000: Firmware %s loaded and running.\n", >> - desired_fw->name); >> - else >> - printk(KERN_CONT " - too many retries. Giving up\n"); >> - >> +report_failure: >> + pr_cont(" - too many retries. Giving up\n"); >> return ret; >> +report_success: >> + pr_info("xc5000: Firmware %s loaded and running.\n", desired_fw->name); >> + return 0; >> } >> >> static void xc5000_do_timer_sleep(struct work_struct *timer_sleep) Is the proposed source code restructuring interesting? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Are you interested in a bit of software optimisation for the implementation > of the function "xc_load_fw_and_init_tuner"? To be clear, absolutely none of the code in question is performance sensitive (i.e. saving a couple of extra CPU cycles has no value in this case). Hence given that I'm assuming you have no intention to actually test any of these patches with a real device I would recommend you do the bare minimum to prevent Coccinelle from complaining and not restructure any of the core business logic unless you plan to also do actual testing. Thanks, Devin
diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c index e6e5e90..1360677 100644 --- a/drivers/media/tuners/xc5000.c +++ b/drivers/media/tuners/xc5000.c @@ -1166,7 +1166,7 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force) ret = xc5000_fwupload(fe, desired_fw, fw); if (ret != 0) - goto err; + goto report_failure; msleep(20); @@ -1229,18 +1229,16 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force) /* Default to "CABLE" mode */ ret = xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE); if (!ret) - break; + goto report_success; printk(KERN_ERR "xc5000: can't set to cable mode."); } -err: - if (!ret) - printk(KERN_INFO "xc5000: Firmware %s loaded and running.\n", - desired_fw->name); - else - printk(KERN_CONT " - too many retries. Giving up\n"); - +report_failure: + pr_cont(" - too many retries. Giving up\n"); return ret; +report_success: + pr_info("xc5000: Firmware %s loaded and running.\n", desired_fw->name); + return 0; } static void xc5000_do_timer_sleep(struct work_struct *timer_sleep)