Re: [PATCH] sched_clock: add data pointer argument to read callback
Hi Mans, On ven., oct. 09 2015, Mans Rullgardwrote: > This passes a data pointer specified in the sched_clock_register() > call to the read callback allowing simpler implementations thereof. > > In this patch, existing uses of this interface are simply updated > with a null pointer. > > Signed-off-by: Mans Rullgard > --- [...] > diff --git a/drivers/clocksource/time-armada-370-xp.c > b/drivers/clocksource/time-armada-370-xp.c > index 2162796..a13b73b 100644 > --- a/drivers/clocksource/time-armada-370-xp.c > +++ b/drivers/clocksource/time-armada-370-xp.c > @@ -92,7 +92,7 @@ static void local_timer_ctrl_clrset(u32 clr, u32 set) > local_base + TIMER_CTRL_OFF); > } > > -static u64 notrace armada_370_xp_read_sched_clock(void) > +static u64 notrace armada_370_xp_read_sched_clock(void *data) > { > return ~readl(timer_base + TIMER0_VAL_OFF); > } > @@ -290,7 +290,8 @@ static void __init armada_370_xp_timer_common_init(struct > device_node *np) > /* >* Set scale and timer for sched_clock. >*/ > - sched_clock_register(armada_370_xp_read_sched_clock, 32, timer_clk); > + sched_clock_register(armada_370_xp_read_sched_clock, 32, timer_clk, > + NULL); > > clocksource_mmio_init(timer_base + TIMER0_VAL_OFF, > "armada_370_xp_clocksource", For the time-armada-370-xp.c file: Acked-by: Gregory CLEMENT Thanks, Gregory -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sched_clock: add data pointer argument to read callback
Mans, On Fri, 9 Oct 2015, Mans Rullgard wrote: > This passes a data pointer specified in the sched_clock_register() > call to the read callback allowing simpler implementations thereof. > > In this patch, existing uses of this interface are simply updated > with a null pointer. I can't see any simplifaction of any driver. This is called in hot pathes and that extra unused argument just adds register pressure for no value. What are you actually trying to solve? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sched_clock: add data pointer argument to read callback
On Sat, Oct 10, 2015 at 01:42:47AM +0100, Måns Rullgård wrote: > Russell King - ARM Linuxwrites: > > > On Sat, Oct 10, 2015 at 12:48:22AM +0100, Måns Rullgård wrote: > >> Russell King - ARM Linux writes: > >> > >> > On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote: > >> >> This passes a data pointer specified in the sched_clock_register() > >> >> call to the read callback allowing simpler implementations thereof. > >> >> > >> >> In this patch, existing uses of this interface are simply updated > >> >> with a null pointer. > >> > > >> > This is a bad description. It tells us what the patch is doing, > >> > (which we can see by reading the patch) but not _why_. Please include > >> > information on why the change is necessary - describe what you are > >> > trying to achieve. > >> > >> Currently most of the callbacks use a global variable to store the > >> address of a counter register. This has several downsides: > >> > >> - Loading the address of a global variable can be more expensive than > >> keeping a pointer next to the function pointer. > >> > >> - It makes it impossible to have multiple instances of a driver call > >> sched_clock_register() since the caller can't know which clock will > >> win in the end. > >> > >> - Many of the existing callbacks are practically identical and could be > >> replaced with a common generic function if it had a pointer argument. > >> > >> If I've missed something that makes this a stupid idea, please tell. > > > > So my next question is whether you intend to pass an iomem pointer > > through this, or a some kind of structure, or both. It matters, > > because iomem pointers have a __iomem attribute to keep sparse > > happy. Having to force that attribute on and off pointers is frowned > > upon, as it defeats the purpose of the sparse static checker. > > So this is an instance where tools like sparse get in the way of doing > the simplest, most efficient, and obviously correct thing. Who wins in > such cases? In that case, NAK on the patch. I don't have time for your stupid games. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sched_clock: add data pointer argument to read callback
On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote: > This passes a data pointer specified in the sched_clock_register() > call to the read callback allowing simpler implementations thereof. > > In this patch, existing uses of this interface are simply updated > with a null pointer. This is a bad description. It tells us what the patch is doing, (which we can see by reading the patch) but not _why_. Please include information on why the change is necessary - describe what you are trying to achieve. I generally don't accept patches what add new stuff to the kernel with no users of that new stuff - that's called experience, experience of people who submit stuff like that, and then vanish leaving their junk in the kernel without any users. Please ensure that this gets a user very quickly, or better still, submit this patch as part of a series which makes use of it. Also, copying soo many people is guaranteed to be silently dropped by mailing lists. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sched_clock: add data pointer argument to read callback
Russell King - ARM Linuxwrites: > On Sat, Oct 10, 2015 at 12:48:22AM +0100, Måns Rullgård wrote: >> Russell King - ARM Linux writes: >> >> > On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote: >> >> This passes a data pointer specified in the sched_clock_register() >> >> call to the read callback allowing simpler implementations thereof. >> >> >> >> In this patch, existing uses of this interface are simply updated >> >> with a null pointer. >> > >> > This is a bad description. It tells us what the patch is doing, >> > (which we can see by reading the patch) but not _why_. Please include >> > information on why the change is necessary - describe what you are >> > trying to achieve. >> >> Currently most of the callbacks use a global variable to store the >> address of a counter register. This has several downsides: >> >> - Loading the address of a global variable can be more expensive than >> keeping a pointer next to the function pointer. >> >> - It makes it impossible to have multiple instances of a driver call >> sched_clock_register() since the caller can't know which clock will >> win in the end. >> >> - Many of the existing callbacks are practically identical and could be >> replaced with a common generic function if it had a pointer argument. >> >> If I've missed something that makes this a stupid idea, please tell. > > So my next question is whether you intend to pass an iomem pointer > through this, or a some kind of structure, or both. It matters, > because iomem pointers have a __iomem attribute to keep sparse > happy. Having to force that attribute on and off pointers is frowned > upon, as it defeats the purpose of the sparse static checker. So this is an instance where tools like sparse get in the way of doing the simplest, most efficient, and obviously correct thing. Who wins in such cases? -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sched_clock: add data pointer argument to read callback
Russell King - ARM Linuxwrites: > On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote: >> This passes a data pointer specified in the sched_clock_register() >> call to the read callback allowing simpler implementations thereof. >> >> In this patch, existing uses of this interface are simply updated >> with a null pointer. > > This is a bad description. It tells us what the patch is doing, > (which we can see by reading the patch) but not _why_. Please include > information on why the change is necessary - describe what you are > trying to achieve. Currently most of the callbacks use a global variable to store the address of a counter register. This has several downsides: - Loading the address of a global variable can be more expensive than keeping a pointer next to the function pointer. - It makes it impossible to have multiple instances of a driver call sched_clock_register() since the caller can't know which clock will win in the end. - Many of the existing callbacks are practically identical and could be replaced with a common generic function if it had a pointer argument. If I've missed something that makes this a stupid idea, please tell. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sched_clock: add data pointer argument to read callback
On Sat, Oct 10, 2015 at 12:48:22AM +0100, Måns Rullgård wrote: > Russell King - ARM Linuxwrites: > > > On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote: > >> This passes a data pointer specified in the sched_clock_register() > >> call to the read callback allowing simpler implementations thereof. > >> > >> In this patch, existing uses of this interface are simply updated > >> with a null pointer. > > > > This is a bad description. It tells us what the patch is doing, > > (which we can see by reading the patch) but not _why_. Please include > > information on why the change is necessary - describe what you are > > trying to achieve. > > Currently most of the callbacks use a global variable to store the > address of a counter register. This has several downsides: > > - Loading the address of a global variable can be more expensive than > keeping a pointer next to the function pointer. > > - It makes it impossible to have multiple instances of a driver call > sched_clock_register() since the caller can't know which clock will > win in the end. > > - Many of the existing callbacks are practically identical and could be > replaced with a common generic function if it had a pointer argument. > > If I've missed something that makes this a stupid idea, please tell. So my next question is whether you intend to pass an iomem pointer through this, or a some kind of structure, or both. It matters, because iomem pointers have a __iomem attribute to keep sparse happy. Having to force that attribute on and off pointers is frowned upon, as it defeats the purpose of the sparse static checker. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html