Re: [PATCH v2 2/4] printk: deprecate boot_delay in favour of printk_delay

[Andrew Murray]

Hi Petr,

>
> I would change this to sizeof(unsigned int) as well.
>
> >                 .mode           = 0644,
> > -               .proc_handler   = proc_dointvec_minmax,
> > +               .proc_handler   = proc_douintvec_minmax,
> >                 .extra1         = SYSCTL_ZERO,
> >                 .extra2         = (void *)&ten_thousand,
> >         },

OK.


>
> Otherwise, it looks good to me.
>
> > This makes the cast explict, verifies the range before casting
>
> Yup.
>
> > (I guess a range of negative values could result in a postive value
> > within the 10,000 range) and also uses the correct proc_handler.
>
> IMHO, this should not happen. I have never seen int smaller than 4
> bytes. If I count it correctly, positive numbers should be up to 32767.
>
>
> > This has the side effect of not showing the following pr_debug for the
> > lpj calculation when a delay is not set. Though loops_per_msec is only
> > used during boot, so I don't think there is any loss here.
>
> I agree that it should not harm. But please mention this in
> the commit message.

Yes, sure.


>
> >         pr_debug("printk_delay: %u, preset_lpj: %ld, lpj: %lu, "
> >                 "HZ: %d, loops_per_msec: %llu\n",
> >                 printk_delay_msec, preset_lpj, lpj, HZ, loops_per_msec);
> >
> > Can I keep your Reviewed-By with these changes?
>
> Feel free to keep it. That said, please put at least the change
> of the ctl_table into a separate patch. I would rather put both new
> changes into separate file. It is never a good idea to hide
> such changes in a complex patch which mostly reshuffles an existing
> code.

Yes, that's a fair point.

Thanks,

Andrew Murray


>
> Best Regards,
> Petr