linux-kernel - Re: [PATCH v2 1/3] units: complement the set of Hz units

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 1 Aug 2022 13:01:40 +0000
From:   Dmitry Rokosov <DDRokosov@...rdevices.ru>
To:     Jonathan Cameron <jic23@...nel.org>
CC:     Andy Shevchenko <andy.shevchenko@...il.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
        "wsa@...nel.org" <wsa@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "Michael.Hennerich@...log.com" <Michael.Hennerich@...log.com>,
        "jbhayana@...gle.com" <jbhayana@...gle.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        kernel <kernel@...rdevices.ru>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] units: complement the set of Hz units

Hello Jonathan,

Thank you for the review. Please find my comments below.

On Sun, Jul 31, 2022 at 12:41:55PM +0100, Jonathan Cameron wrote:
> On Fri, 29 Jul 2022 20:02:42 +0200
> Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> 
> > On Fri, Jul 29, 2022 at 7:23 PM Dmitry Rokosov <DDRokosov@...rdevices.ru> wrote:
> > >
> > > Currently, Hz units do not have milli, micro and nano Hz coefficients.
> > > Some drivers (IIO especially) use their analogues to calculate
> > > appropriate Hz values. This patch includes them to units.h definitions,
> > > so they can be used from different kernel places.  
> > 
> > ...
> > 
> > > +#define NHZ_PER_HZ             1000000000UL
> > > +#define UHZ_PER_HZ             1000000UL
> > > +#define MILLIHZ_PER_HZ         1000UL  
> > 
> > Oh, but then a bit of consistency?
> > 
> > MICRO
> > NANO
> Tricky given existing items, but I agree we shouldn't make
> it worse.

Okay, agree. I'll change them to MILLI/MICRO/NANO in the next version.

> 
> However, I'm not 100% sold on why we need these conversions relative to HZ.
> What's wrong with using MILLI / NANO etc as already defined?  I guess
> there is a 'documentation' like effect of making it clear these are frequency
> unit conversions, but I don't think it makes sense to add it for all the other
> types of unit, so why is Hz special?

Yes, you are totally right, it has some 'documenation' effect from
a physics theory perspective. Kernel already has some units for HZ, so I
think it's a bad idea when sometimes we have to use *HZ for KILO and
MEGA units, but sometimes we have to use abstract MILLI/MICRO/NANO
coefficients. In other words, I suppose the right way is to choose one
option from the following list:
   - remove all *HZ units and use abstract units instead
   - complement *HZ units and use them

In my opinion, the second one is better, because *HZ units are good
opposite to *SEC units (from time64.h).

> 
> I'm not sure why we have the existing ones for HZ with the
> exception of KHZ_PER_MEGAHZ.
> 
> Jonathan
> 
> > 
> 

-- 
Thank you,
Dmitry