Re: [PATCH] clk: check ops pointer on clock register

[Jerome Brunet <jbrunet@baylibre.com>]

On Mon, 2017-12-18 at 13:06 -0800, Stephen Boyd wrote:
> On 12/18, Michael Turquette wrote:
> > Hi Jerome & Stephen,
> > 
> > On Mon, Dec 18, 2017 at 12:06 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote:
> > > > On 12/18, Jerome Brunet wrote:
> > > > > Nothing really prevents a provider from (trying to) register a clock
> > > > > without providing the clock ops structure.
> > > > > 
> > > > > We do check the individual fields before using them, but not the
> > > > > structure pointer itself. This may have the usual nasty consequences when
> > > > > the pointer is dereferenced, mostly likely when checking one the field
> > > > > during the initialization.
> > > > 
> > > > Yes, that nasty consequence should be a kernel oops,
> > > 
> > > Precisely
> > > 
> > > > and the
> > > > developer should notice that before submitting the driver for
> > > > inclusion.
> > > 
> > > Agreed. But people may make mistakes, which is why (at least partly) we
> > > do checks, isn't it ?
> > 
> > Agreed the developers should test before submitting, but procedurally
> > generated clocks (e.g. registering clocks in a loop using a
> > predictable register map, etc) could lead to a situation where a
> > developer doesn't test every possible iteration.
> > 
> > Hypothetical, but easy easy easy to fix with Jerome's patch.
> > 
> > > 
> > > > I don't think we really care to return an error here
> > > > if this happens.
> > > > 
> > > 
> > > I don't understand why we would let a oops happen when can catch the error
> > > properly ?
> > > 
> > 
> > Agreed with Jerome on this one.
> > 
> > Let's flip it on its head: any downside to this patch? If not I can merge.
> > 
> 
> If code is not checking return values from clk_register(), then
> an oops turns into a silently ignored error. 

It would really be asking for trouble, wouldn't it ?

> Hunting that down is
> going to take some time vs. an oops when we attempt to call the
> clk ops that aren't there.
> 
> The idea is fine, but I would change two things. First I would
> throw a WARN_ON() around the condition so developers notice
> quicker that something is wrong, and second I would strip off the
> 'Fixes' tag because this isn't really fixing anything that we
> need to backport to stable trees. It just converts an oops into a
> warning.
> 

Fine by me.