linux-kernel - Re: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230426101200.7czyp6nlg44tweyb@intel.intel>
Date:   Wed, 26 Apr 2023 12:12:00 +0200
From:   Andi Shyti <andi.shyti@...nel.org>
To:     Matti Vaittinen <mazziesaccount@...il.com>
Cc:     Andi Shyti <andi.shyti@...nel.org>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Paul Gazzillo <paul@...zz.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor

Hi Matti,

> Thanks for the review! It's nice to see you're still keeping an eye on ROHM
> / Kionix senor drivers ;)

yeah... this is fun... if I just had a bit more time :)

> > > +static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
> > > +			    struct iio_chan_spec const *chan, int *val, int *val2)
> > > +{
> > > +	int ret, int_time;
> > > +
> > > +	ret = bu27008_chan_cfg(data, chan);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = bu27008_meas_set(data, BU27008_MEAS_EN);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	int_time = bu27008_get_int_time(data);
> > > +	if (int_time < 0)
> > > +		int_time = 400000;
> > > +
> > > +	msleep((int_time + 500) / 1000);
> > 
> > What is this 500 doing? Is it making a real difference? it's
> > 0.5ms.
> 
> Thanks for the question, having extra pairs of eyes helps spotting
> brainfarts :)
> 
> The 500 here is half of the value of the divider - idea was to do rounding
> correctly upwards to prevent premature wake-up. Well, this is incorrect
> because we should always round up the sleep time, not just 'mathematically
> correctly' (Eg, not only upwards when value >= 0.5 but upwards always when
> the division is not even).
> 
> After this being said, integration times for this device are full milli
> seconds so they can all be divided by 1000 uS.
> 
> Nevertheless, it's good to note that the sensor is definitely not being
> clocked by the same clock as CPU and I assume the timing for it will be
> drifting quite a bit from the CPU clock. This means some sensors will for
> sure complete the measurement later than this wake-up. In order to tackle
> this we have the valid-bit polling in bu27008_chan_read_data(). So, at the
> end of the day, this rounding correction is lkely to be just some
> unnecessary noise.

I understand the logic of the waiting, but msleep is not the
right function as waiting with msleep is always very approximate,
that's why it's recommended to use it for a large waiting period,
where the error is smaller.

If int_time is 1ms, waiting 1.5 or 2 or 1, is the same thing,
most probably you will end up waiting more.

> > What's the minimum int_time? Can we set a minimum, as well, just
> > for the sake of the msleep?
> 
> Can you please elaborate what you mean by this? The minimum integration time
> for bu27008 is 55 mS and this is set in the time tables for the gts-helpers.
> The bu27008_get_int_time() should never return valid time smaller than that.

Witha minimum i mean a minimum value for the msleep to start
working decently. E.g. what if int_time is lower than 1ms? Can we
have msleep(0)?

[...]

> > > +static int bu27008_chip_init(struct bu27008_data *data)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = regmap_update_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
> > > +			   BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
> > > +	if (ret)
> > > +		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
> > > +
> > > +	/*
> > > +	 * The data-sheet does not tell how long performing the IC reset takes.
> > > +	 * However, the data-sheet says the minimum time it takes the IC to be
> > > +	 * able to take inputs after power is applied, is 100 uS. I'd assume
> > > +	 * > 1 mS is enough.
> > > +	 */
> > > +	msleep(1);
> > 
> > please use usleep_range().
> 
> I prefer to not require setting up hrtimers as we have no real requirements
> for the duration of this sleep. I know the msleep() is likely to exceed the
> 1 mS, potentially a lot if there is things to do - but we don't really care
> at this point. The main thing is to give the HW time to reset while allowing
> other things to be scheduled.

For the reason above, msleep(1) is quite a meaningless
instruction. If you need to wait around 1ms, then usleep_range is
the function to be used.

Refer, also, to the Documentation/timers/timers-howto.rst

> > > +
> > > +	return ret;
> > > +}

[...]

> > > +static irqreturn_t bu27008_trigger_handler(int irq, void *p)
> > 
> > Do we really need to be in atomic context here? Can this be
> > handled from a thread?
> 
> As far as I understand, this is handled from a process context.

Sorry... I misread it... I thought you used request_irq() for
this and request_threaded_irq() for bu27008_irq_thread_handler().

Ignore :)

Andi