lkml.org 
[lkml]   [2017]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] clk: check ops pointer on clock register
From
Date
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.

\
 
 \ /
  Last update: 2017-12-18 22:12    [W:0.067 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site