Re: [PATCH v4 2/3] random: Return nbytes filled from hw RNG

[Tobin C. Harding]

On Tue, May 15, 2018 at 05:35:46PM -0400, Steven Rostedt wrote:
> On Wed, 16 May 2018 07:17:06 +1000
> "Tobin C. Harding" <me@xxxxxxxx> wrote:
> 
> > > > -void get_random_bytes_arch(void *buf, int nbytes)
> > > > +int __must_check get_random_bytes_arch(void *buf, int nbytes)
> > > >  {
> > > >  	char *p = buf;
> > > > +	int left = nbytes;  
> > > 
> > > Just a nit, but I know some kernel devs prefer "upside-down-xmas-tree"
> > > style of declarations. Which would make the above:
> > > 
> > > 	int left = nbytes;
> > > 	char *p = buf;  
> > 
> > Super specific coding style and rigorous code cleanliness is a big part
> > of why I love kernel dev.  Thanks for pointing this one out. 
> 
> It's a relatively new form, but I like it. It makes the code look "less
> messy" ;-)  Some devs don't care, others do. This file already breaks
> it, so it really is up to you. Like I said, it's "just a nit", not
> really important.
> 
> > 
> > While we are on these code lines, whats the typical kernel variable name
> > for a loop counter that is going to be counted down? 'left',
> > 'remaining', 'to_go', 'still'???
> 
> "left" looks good to me.
> 
> > 
> > > >  
> > > > -	trace_get_random_bytes_arch(nbytes, _RET_IP_);
> > > > -	while (nbytes) {
> > > > +	trace_get_random_bytes_arch(left, _RET_IP_);  
> > > 
> > > Nothing to do with this patch series, but I wonder if we should move
> > > the trace event below, and record how much was done.  
> > 
> > I don't fully understand trace events, I just left this line in tact
> > and hoped for the best :(
> 
> Your patch is fine. This could be something to add after your series.
> 
> > 
> > /me adds 'trace events' to list of things to learn more about
> 
> Just look at /sys/kernel/debug/tracing/events
> 
> Or read Documentation/trace/ftrace.{rst,txt}.

Awesome, cheers.