Messages in this thread | | | Date | Mon, 22 Jun 2009 14:20:14 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 1/1] hwlat_detector: A system hardware latency detector |
| |
On Mon, 22 Jun 2009 16:58:03 -0400 Jon Masters <jonathan@jonmasters.org> 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.
| |