On Tue, 2015-10-27 at 17:16 +0100, Vitaly Kuznetsov wrote:
> Andy Shevchenko <[email protected]> writes:
[]
> > + if (!blk_size) {
> > > + WARN_ON(1);
> >
> > Hmm... Isn't it too strong? WARN_ONCE() might reduce a noise. Or
> > even
> > pr_warn_once/ratelimited().
>
> Nobody is supposed to call string_get_size() with blk_size = 0, if
> someone does that - it is a bug and that's what WARN_ON is supposed
> to
> report. I'm OK with changing it to WARN_ONCE() but I don't see a big
> difference - nobody's calling string_get_size() in a loop, one/two
> calls
> per one storage device is expected.
I'm fine with WARN_ONCE() if there is no objection.
>
> >
> > > + size = 0;
> > > + goto out;
> > > }
> >
> > What about doing it before if (!size) ?
> >
> > Like
> >
> > if (!blk_size) {
> > pr_warn_once(); /* or WARN_ONCE() ? */
> > /* Override size to follow error path */
> > size = 0;
> > }
> >
> > if (!size)
>
> To be honest I don't see a big difference but I'm fine with the
> change :-)
Maybe Rasmus can judge me.
> > > - remainder %= divisor[units];
> > > + remainder -= (remainder / divisor[units]) *
> > > divisor[units];
> >
> > I'm sorry I didn't get what the purpose of change here.
> >
> > (Yes, I was thinking about u64 on 32-bit architecture, but % and /
> > are
> > working in the similar way aren't they?)
>
> Thanks for noticing, there is no functional change here, it just made
> the code easier to understand (for me only?). I'm OK with reverting
> it
> to '%='.
For me is the opposite. remainder = remainder % divisor[units] will
work as well, but why change the code at all.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
