Re: [PATCH 8/8] clk: tegra: Add EMC clock driver

[Stephen Warren]

On 07/29/2014 02:19 PM, Mike Turquette wrote:
> Quoting Mikko Perttunen (2014-07-29 01:47:35)
> > On 22/07/14 19:57, Stephen Warren wrote:
> > > On 07/11/2014 08:18 AM, Mikko Perttunen wrote:
> > > > +static int emc_debug_rate_set(void *data, u64 rate)
> > > > +{
> > > > +    struct tegra_emc *tegra = data;
> > > > +
> > > > +    return clk_set_rate(tegra->hw.clk, rate);
> > > > +}
> > > > +
> > > > +DEFINE_SIMPLE_ATTRIBUTE(emc_debug_rate_fops, emc_debug_rate_get,
> > > > +                    emc_debug_rate_set, "%lld\n");
> > >
> > > I think the rate can already be obtained through
> > > ...debug/clock/clock_summary. I'm not sure about changing the rate, but
> > > shouldn't that be a feature of the common clock core, not individual
> > > drivers?
> >
> > The core doesn't allow writing to the rate debugfs files, so this is the
> > only way to trigger an EMC clock change for now. I agree that the core
> > might be a better place. I don't know if there are any philosophical
> > objections to that. I'd like to keep this in until a possible core
> > feature addition. Mike, any comments?
>
> Yes, there is a philosophical rejection to exposing rate-change knobs to
> userspace through debugfs. These can and will ship in real products
> (typically Android) with lots of nasty userspace hacks, and also
> represent pretty dangerous things to expose to userspace. I have always
> maintained that such knobs should remain out of tree or, with the advent
> of the custom debugfs entries, should be burden of the clock drivers.

That argument seems a bit inconsistent.

I can see the argument to disallow code that lets user-space fiddle with 
clocks. However, if that argument holds, then surely it must apply to 
either the clock core *or* a clock driver; the end effect of allowing 
the code in either place is that people will be able to implement the 
user-space hacks you want to avoid. Yet, if we allow the code because 
it's a useful debug tool, then surely it should be in the clock core so 
we don't implement it redundantly in each clock driver.

We could always taint the kernel if the feature is used. Admittedly that 
wouldn't stop people using the feature as a hack in Android/product 
kernels, but at least nobody would have to unknowingly debug problems 
due to such manipulation, in the context of an upstream kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/