| Message ID | 1523662294-17971-1-git-send-email-jasmin@anw.at (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1f78Ad-0000wI-EY; Fri, 13 Apr 2018 23:31:55 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751032AbeDMXbw (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 13 Apr 2018 19:31:52 -0400 Received: from mail.anw.at ([195.234.101.228]:34263 "EHLO mail.anw.at" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750940AbeDMXbw (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 13 Apr 2018 19:31:52 -0400 Received: from hoppel.217.196.72.190 (anwhome.anw.at [195.234.103.23]) by mail.anw.at (8.14.4/8.14.4/Debian-4.1ubuntu1) with ESMTP id w3DNVk2p006508; Sat, 14 Apr 2018 01:31:46 +0200 From: "Jasmin J." <jasmin@anw.at> To: linux-media@vger.kernel.org Cc: mchehab@s-opensource.com, jasmin@anw.at, Thomas Gleixner <tglx@linutronix.de> Subject: [PATCH] media: Revert cleanup ktime_set() usage Date: Sat, 14 Apr 2018 01:31:34 +0200 Message-Id: <1523662294-17971-1-git-send-email-jasmin@anw.at> X-Mailer: git-send-email 2.7.4 X-Antivirus: checked in 0.019sec at mail.anw.at ([195.234.102.72]) by smf-clamd v1.2.1 - http://smfs.sf.net/ Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Jasmin J.
April 13, 2018, 11:31 p.m. UTC
From: Jasmin Jessich <jasmin@anw.at> This reverts 8b0e195314fa, because this will not compile for Kernels older than 4.10. Cc: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Jasmin Jessich <jasmin@anw.at> --- drivers/media/dvb-core/dmxdev.c | 2 +- drivers/media/pci/cx88/cx88-input.c | 6 ++++-- drivers/media/pci/pt3/pt3.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-)
Comments
Em Sat, 14 Apr 2018 01:31:34 +0200 "Jasmin J." <jasmin@anw.at> escreveu: > From: Jasmin Jessich <jasmin@anw.at> > > This reverts 8b0e195314fa, because this will not compile for Kernels > older than 4.10. This patch looks fine, but not for the above-mentioned. The thing is that it is not consistent to have some places with things like: timeout = ktime_set(1, ir->polling * 1000000); and others with: timeout = ir->polling * 1000000; We should either use ktime_set() everywhere or remove it as a whole. My preference is to keep using it, as it makes it better documented. In any case, gcc should be smart enough to discard multiply by a zero constant when evaluating ktime_set() macro (if not, it is a gcc issue). The fact that it makes maintainership of the media_build backport tree easier is just a plus, but it makes no sense upstream. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Jasmin Jessich <jasmin@anw.at> > --- > drivers/media/dvb-core/dmxdev.c | 2 +- > drivers/media/pci/cx88/cx88-input.c | 6 ++++-- > drivers/media/pci/pt3/pt3.c | 2 +- > 3 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c > index 61a750f..cb078d6 100644 > --- a/drivers/media/dvb-core/dmxdev.c > +++ b/drivers/media/dvb-core/dmxdev.c > @@ -622,7 +622,7 @@ static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev, > struct dmxdev_filter *filter, > struct dmxdev_feed *feed) > { > - ktime_t timeout = 0; > + ktime_t timeout = ktime_set(0, 0); > struct dmx_pes_filter_params *para = &filter->params.pes; > enum dmx_output otype; > int ret; > diff --git a/drivers/media/pci/cx88/cx88-input.c b/drivers/media/pci/cx88/cx88-input.c > index 6f4e692..b13243d 100644 > --- a/drivers/media/pci/cx88/cx88-input.c > +++ b/drivers/media/pci/cx88/cx88-input.c > @@ -180,7 +180,8 @@ static enum hrtimer_restart cx88_ir_work(struct hrtimer *timer) > struct cx88_IR *ir = container_of(timer, struct cx88_IR, timer); > > cx88_ir_handle_key(ir); > - missed = hrtimer_forward_now(&ir->timer, ir->polling * 1000000LL); > + missed = hrtimer_forward_now(&ir->timer, > + ktime_set(0, ir->polling * 1000000)); > if (missed > 1) > ir_dprintk("Missed ticks %ld\n", missed - 1); > > @@ -200,7 +201,8 @@ static int __cx88_ir_start(void *priv) > if (ir->polling) { > hrtimer_init(&ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > ir->timer.function = cx88_ir_work; > - hrtimer_start(&ir->timer, ir->polling * 1000000LL, > + hrtimer_start(&ir->timer, > + ktime_set(0, ir->polling * 1000000), > HRTIMER_MODE_REL); > } > if (ir->sampling) { > diff --git a/drivers/media/pci/pt3/pt3.c b/drivers/media/pci/pt3/pt3.c > index da74828..8ced807 100644 > --- a/drivers/media/pci/pt3/pt3.c > +++ b/drivers/media/pci/pt3/pt3.c > @@ -464,7 +464,7 @@ static int pt3_fetch_thread(void *data) > > pt3_proc_dma(adap); > > - delay = PT3_FETCH_DELAY * NSEC_PER_MSEC; > + delay = ktime_set(0, PT3_FETCH_DELAY * NSEC_PER_MSEC); > set_current_state(TASK_UNINTERRUPTIBLE); > freezable_schedule_hrtimeout_range(&delay, > PT3_FETCH_DELAY_DELTA * NSEC_PER_MSEC, Thanks, Mauro
Hello Mauro! > This patch looks fine, but not for the above-mentioned. So I shall reword the commit message? > The thing is that it is not consistent to have some places with > things like: > timeout = ktime_set(1, ir->polling * 1000000); > > and others with: > timeout = ir->polling * 1000000; With my patch applied I can find ONLY two "ir->polling * 1000000" and both are used together with ktime_set. $ grep -r "ir->polling" * media/i2c/ir-kbd-i2c.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling_interval)); media/i2c/ir-kbd-i2c.c: ir->polling_interval = DEFAULT_POLLING_INTERVAL; media/i2c/ir-kbd-i2c.c: ir->polling_interval = init_data->polling_interval; media/usb/em28xx/em28xx-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); media/usb/em28xx/em28xx-input.c: ir->polling = 100; /* ms */ media/usb/em28xx/em28xx-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); media/usb/tm6000/tm6000-input.c: if (!ir->polling) media/usb/tm6000/tm6000-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); media/usb/tm6000/tm6000-input.c: ir->polling = 50; media/usb/tm6000/tm6000-input.c: if (!ir->polling) media/usb/au0828/au0828-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); media/usb/au0828/au0828-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); media/usb/au0828/au0828-input.c: ir->polling = 100; /* ms */ media/usb/au0828/au0828-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); media/pci/saa7134/saa7134-input.c: if (ir->polling) { media/pci/saa7134/saa7134-input.c: if (ir->polling) { media/pci/saa7134/saa7134-input.c: if (!ir->polling && !ir->raw_decode) { media/pci/saa7134/saa7134-input.c: mod_timer(&ir->timer, jiffies + msecs_to_jiffies(ir->polling)); media/pci/saa7134/saa7134-input.c: if (ir->polling) { media/pci/saa7134/saa7134-input.c: if (ir->polling) media/pci/saa7134/saa7134-input.c: ir->polling = polling; media/pci/cx88/cx88-input.c: if (ir->polling) { media/pci/cx88/cx88-input.c: ir->polling ? "poll" : "irq", media/pci/cx88/cx88-input.c: ktime_set(0, ir->polling * 1000000)); media/pci/cx88/cx88-input.c: if (ir->polling) { media/pci/cx88/cx88-input.c: ktime_set(0, ir->polling * 1000000), media/pci/cx88/cx88-input.c: if (ir->polling) media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 1; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 5; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 10; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 1; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 1; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 1; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ media/pci/cx88/cx88-input.c: ir->polling = 100; /* ms */ media/pci/bt8xx/bttv-input.c: if (ir->polling) { media/pci/bt8xx/bttv-input.c: ir->polling ? "poll" : "irq", media/pci/bt8xx/bttv-input.c: else if (!ir->polling) media/pci/bt8xx/bttv-input.c: mod_timer(&ir->timer, jiffies + msecs_to_jiffies(ir->polling)); media/pci/bt8xx/bttv-input.c: if (ir->polling) { media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms media/pci/bt8xx/bttv-input.c: ir->polling = 50; /* ms */ media/pci/bt8xx/bttv-input.c: ir->polling = 50; /* ms */ media/pci/bt8xx/bttv-input.c: ir->polling = 1; /* ms */ Currently I am using the exact same patch in media-build. media-build would not be able to compile without error for all Kernels, if I would have overlook a setting of ktime_t without ktime_set. Please point me where I can find the "timeout = ir->polling * 1000000;" you mentioned. > My preference is to keep using it, as it makes it better documented. This is also my preference. > The fact that it makes maintainership of the media_build backport > tree easier is just a plus, but it makes no sense upstream. We fixed all those issues in the past in media-tree. All drivers which are able (enabled) to be compiled for Kernels older than 4.10 are using ktime_set. When you mean we shall change all drivers to use ktime_set (even the ones which are currently not compiled for <4.10), then I can try to find those and add it to the patch. Even the mentioned 8b0e195314fa commit did not fix all existing ktime_set(0, 0): $ grep -r ktime_set | grep "0, 0" drivers/scsi/ufs/ufshcd.c: hba->lrb[task_tag].compl_time_stamp = ktime_set(0, 0); drivers/scsi/ufs/ufshcd.c: hba->ufs_stats.last_hibern8_exit_tstamp = ktime_set(0, 0); drivers/scsi/ufs/ufshcd.c: hba->ufs_stats.last_hibern8_exit_tstamp = ktime_set(0, 0); drivers/scsi/ufs/ufshcd.c: hba->ufs_stats.last_hibern8_exit_tstamp = ktime_set(0, 0); drivers/ntb/test/ntb_perf.c: pthr->duration = ktime_set(0, 0); drivers/net/can/usb/peak_usb/pcan_usb_core.c: time_ref->tv_host = ktime_set(0, 0); drivers/thermal/thermal_sysfs.c: stats->time_in_state[i] = ktime_set(0, 0); drivers/dma-buf/sync_file.c: ktime_set(0, 0); drivers/staging/greybus/loopback.c: gb->ts = ktime_set(0, 0); arch/mips/kvm/emulate.c: ktime_t now = ktime_set(0, 0); /* silence bogus GCC warning */ BR, Jasmin
Em Fri, 27 Apr 2018 20:15:00 +0200 "Jasmin J." <jasmin@anw.at> escreveu: > Hello Mauro! > > > This patch looks fine, but not for the above-mentioned. > So I shall reword the commit message? Yes. > > > The thing is that it is not consistent to have some places with > > things like: > > timeout = ktime_set(1, ir->polling * 1000000); > > > > and others with: > > timeout = ir->polling * 1000000; > With my patch applied I can find ONLY two "ir->polling * 1000000" and both are used > together with ktime_set. > > $ grep -r "ir->polling" * > media/i2c/ir-kbd-i2c.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling_interval)); > media/i2c/ir-kbd-i2c.c: ir->polling_interval = DEFAULT_POLLING_INTERVAL; > media/i2c/ir-kbd-i2c.c: ir->polling_interval = init_data->polling_interval; > media/usb/em28xx/em28xx-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); > media/usb/em28xx/em28xx-input.c: ir->polling = 100; /* ms */ > media/usb/em28xx/em28xx-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); > media/usb/tm6000/tm6000-input.c: if (!ir->polling) > media/usb/tm6000/tm6000-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); > media/usb/tm6000/tm6000-input.c: ir->polling = 50; > media/usb/tm6000/tm6000-input.c: if (!ir->polling) > media/usb/au0828/au0828-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); > media/usb/au0828/au0828-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); > media/usb/au0828/au0828-input.c: ir->polling = 100; /* ms */ > media/usb/au0828/au0828-input.c: schedule_delayed_work(&ir->work, msecs_to_jiffies(ir->polling)); > media/pci/saa7134/saa7134-input.c: if (ir->polling) { > media/pci/saa7134/saa7134-input.c: if (ir->polling) { > media/pci/saa7134/saa7134-input.c: if (!ir->polling && !ir->raw_decode) { > media/pci/saa7134/saa7134-input.c: mod_timer(&ir->timer, jiffies + msecs_to_jiffies(ir->polling)); > media/pci/saa7134/saa7134-input.c: if (ir->polling) { > media/pci/saa7134/saa7134-input.c: if (ir->polling) > media/pci/saa7134/saa7134-input.c: ir->polling = polling; > media/pci/cx88/cx88-input.c: if (ir->polling) { > media/pci/cx88/cx88-input.c: ir->polling ? "poll" : "irq", > media/pci/cx88/cx88-input.c: ktime_set(0, ir->polling * 1000000)); > media/pci/cx88/cx88-input.c: if (ir->polling) { > media/pci/cx88/cx88-input.c: ktime_set(0, ir->polling * 1000000), > media/pci/cx88/cx88-input.c: if (ir->polling) > media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 1; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 5; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 10; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 1; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 1; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 1; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 50; /* ms */ > media/pci/cx88/cx88-input.c: ir->polling = 100; /* ms */ > media/pci/bt8xx/bttv-input.c: if (ir->polling) { > media/pci/bt8xx/bttv-input.c: ir->polling ? "poll" : "irq", > media/pci/bt8xx/bttv-input.c: else if (!ir->polling) > media/pci/bt8xx/bttv-input.c: mod_timer(&ir->timer, jiffies + msecs_to_jiffies(ir->polling)); > media/pci/bt8xx/bttv-input.c: if (ir->polling) { > media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms > media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms > media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms > media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms > media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms > media/pci/bt8xx/bttv-input.c: ir->polling = 50; // ms > media/pci/bt8xx/bttv-input.c: ir->polling = 50; /* ms */ > media/pci/bt8xx/bttv-input.c: ir->polling = 50; /* ms */ > media/pci/bt8xx/bttv-input.c: ir->polling = 1; /* ms */ > > Currently I am using the exact same patch in media-build. media-build would > not be able to compile without error for all Kernels, if I would have > overlook a setting of ktime_t without ktime_set. > Please point me where I can find the "timeout = ir->polling * 1000000;" > you mentioned. I don't really care on how many places it is used. The point is that, at least on media, we should either call ktime_set() everywhere or don't call it at all. I think that using the macro makes it better documented. > > My preference is to keep using it, as it makes it better documented. > This is also my preference. > > > The fact that it makes maintainership of the media_build backport > > tree easier is just a plus, but it makes no sense upstream. > We fixed all those issues in the past in media-tree. All drivers which > are able (enabled) to be compiled for Kernels older than 4.10 are using > ktime_set. That is irrelevant for upstream. We should confine backport-only fixes at media_build, as we won't be preventing kAPI changes due to backports. I mean: if some day ktime_set() gets replaced with something else, we'll do such change upstream no matter how much efforts it would mean for backports. Yet, in this specific case, ktime_set() is not being removed, and, IMO, mixing it with direct assigns makes the API very confusing. > When you mean we shall change all drivers to use ktime_set (even the ones > which are currently not compiled for <4.10), then I can try to find those > and add it to the patch. > > Even the mentioned 8b0e195314fa commit did not fix all existing ktime_set(0, 0): > $ grep -r ktime_set | grep "0, 0" > drivers/scsi/ufs/ufshcd.c: hba->lrb[task_tag].compl_time_stamp = ktime_set(0, 0); > drivers/scsi/ufs/ufshcd.c: hba->ufs_stats.last_hibern8_exit_tstamp = ktime_set(0, 0); > drivers/scsi/ufs/ufshcd.c: hba->ufs_stats.last_hibern8_exit_tstamp = ktime_set(0, 0); > drivers/scsi/ufs/ufshcd.c: hba->ufs_stats.last_hibern8_exit_tstamp = ktime_set(0, 0); > drivers/ntb/test/ntb_perf.c: pthr->duration = ktime_set(0, 0); > drivers/net/can/usb/peak_usb/pcan_usb_core.c: time_ref->tv_host = ktime_set(0, 0); > drivers/thermal/thermal_sysfs.c: stats->time_in_state[i] = ktime_set(0, 0); > drivers/dma-buf/sync_file.c: ktime_set(0, 0); > drivers/staging/greybus/loopback.c: gb->ts = ktime_set(0, 0); > arch/mips/kvm/emulate.c: ktime_t now = ktime_set(0, 0); /* silence bogus GCC warning */ I mean all "media drivers". We should stick with either: ktime_t now = 10; or ktime_t now = ktime_set(0, 10); everywhere along the subsystem. It is up to other subsystem maintainers to decide what works best for them. Nothing prevents you to contact other subsystems and submit patches to make it more consistent along the subsystem. On such case, there's no need to c/c linux-media (or me). > > BR, > Jasmin Thanks, Mauro
diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c index 61a750f..cb078d6 100644 --- a/drivers/media/dvb-core/dmxdev.c +++ b/drivers/media/dvb-core/dmxdev.c @@ -622,7 +622,7 @@ static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev, struct dmxdev_filter *filter, struct dmxdev_feed *feed) { - ktime_t timeout = 0; + ktime_t timeout = ktime_set(0, 0); struct dmx_pes_filter_params *para = &filter->params.pes; enum dmx_output otype; int ret; diff --git a/drivers/media/pci/cx88/cx88-input.c b/drivers/media/pci/cx88/cx88-input.c index 6f4e692..b13243d 100644 --- a/drivers/media/pci/cx88/cx88-input.c +++ b/drivers/media/pci/cx88/cx88-input.c @@ -180,7 +180,8 @@ static enum hrtimer_restart cx88_ir_work(struct hrtimer *timer) struct cx88_IR *ir = container_of(timer, struct cx88_IR, timer); cx88_ir_handle_key(ir); - missed = hrtimer_forward_now(&ir->timer, ir->polling * 1000000LL); + missed = hrtimer_forward_now(&ir->timer, + ktime_set(0, ir->polling * 1000000)); if (missed > 1) ir_dprintk("Missed ticks %ld\n", missed - 1); @@ -200,7 +201,8 @@ static int __cx88_ir_start(void *priv) if (ir->polling) { hrtimer_init(&ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); ir->timer.function = cx88_ir_work; - hrtimer_start(&ir->timer, ir->polling * 1000000LL, + hrtimer_start(&ir->timer, + ktime_set(0, ir->polling * 1000000), HRTIMER_MODE_REL); } if (ir->sampling) { diff --git a/drivers/media/pci/pt3/pt3.c b/drivers/media/pci/pt3/pt3.c index da74828..8ced807 100644 --- a/drivers/media/pci/pt3/pt3.c +++ b/drivers/media/pci/pt3/pt3.c @@ -464,7 +464,7 @@ static int pt3_fetch_thread(void *data) pt3_proc_dma(adap); - delay = PT3_FETCH_DELAY * NSEC_PER_MSEC; + delay = ktime_set(0, PT3_FETCH_DELAY * NSEC_PER_MSEC); set_current_state(TASK_UNINTERRUPTIBLE); freezable_schedule_hrtimeout_range(&delay, PT3_FETCH_DELAY_DELTA * NSEC_PER_MSEC,