Re: [PATCH 1/1] hwlat_detector: A system hardware latency detector

From: Andrew Morton
Date: Mon Jun 22 2009 - 17:21:16 EST


On Mon, 22 Jun 2009 16:58:03 -0400
Jon Masters <jonathan@xxxxxxxxxxxxxx> wrote:

>
> > We seem to be forward-declaring functions which didn't need forward
> > declarations. This adds duplication and noise - personally I think
> > it's better to just get the functions in the correct order and only use
> > forward-decls where circularities are present.
> >
> > A couple of struct are needlessly forward-decalred too. etc.
>
> Well, call me pedantic but I was always taught to do it this way for
> completeness. If that's not how we want to do it in-kernel, I'm happy
> but I'd like to know where this is spelled out (if it is) so I can go
> add that to my to-read list and get this sorted out in my head. And
> conversely, if it's not documented then I should also be fixing it.

It's not a major issue by any means. It's just a little unusual for
kernel code.

> > > + char buf[U64STR_SIZE];
> > > + int csize = min(cnt, sizeof(buf));
> > > + u64 val = 0;
> > > + int err = 0;
> > > +
> > > + memset(buf, '\0', sizeof(buf));
> >
> > This is unneeded.
>
> I disagree. I always like to explicitly initialize everything to known
> values, even if it might be done for me. In this case, I want to ensure
> there is no way the copy_from_user will give me a non-terminated buffer
> (I add an explicit NULL later too) and that it is zeroed before use. If
> this were some hot-path that mattered, it would be different, but it's a
> trivial function that's already dealing with a copy_from_user. I'm not
> fussy about changing, but just so you understand my pedantic logic.


Well. The kernel prefers "fast, risky and well tested" over
"super-defensive".

copy_from_user(buf, ubuf, csize)

will reliably write to buf[0] .. buf[csize-1].


But what you want here is to toss all this code out and call the
not-yet-written u64_from_user(). I think I hinted at someone about
this a few days ago..

> > > + goto unlock;
> > > + enabled = 1;
> > > + __reset_stats();
> > > + if (start_kthread())
> > > + return -EFAULT;
> >
> > -EFAULT seems inappropriate.
>
> If you're unable to kick off the kernel thread, what should you return?

Propagate the kthread_run() return error code back up.

>
> So...I'll post another update (thanks again). Am I certainly too late
> for this merge window? That's ok too.

We're generally pretty relaxed about merging updates to just-added
subsystems. There's not much point in releasing them in a
known-to-be-unfinished form. And if a late fix _does_ break the
driver, that still isn't strictly a regression against the previous
kernel release.

--
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/