Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <69d7d2ab-4bfb-dac3-c6c9-88ab8e481c60@gmail.com>
Date:   Tue, 13 Jun 2023 14:06:19 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Content-Language: en-US, en-GB
To:     Mehdi Djait <mehdi.djait.k@gmail.com>, jic23@kernel.org
Cc:     krzysztof.kozlowski+dt@linaro.org,
        andriy.shevchenko@linux.intel.com, robh+dt@kernel.org,
        lars@metafoo.de, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
References: <cover.1686651032.git.mehdi.djait.k@gmail.com>
 <5e827d018b5d0caf2b5d157e2c24d78c20fe855f.1686651032.git.mehdi.djait.k@gmail.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH v6 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211
 accelerometer
In-Reply-To: <5e827d018b5d0caf2b5d157e2c24d78c20fe855f.1686651032.git.mehdi.djait.k@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi again Mehdi,

Just one thing still crossed to my mind...

On 6/13/23 13:22, Mehdi Djait wrote:
> Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
> ranges from ±2G to ±16G, digital output through I²C/SPI.
> Add support for basic accelerometer features such as reading acceleration
> via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
> 
> Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> ---


> +static int kx132_get_fifo_bytes(struct kx022a_data *data)
> +{
> +	__le16 buf_status;
> +	int ret, fifo_bytes;
> +
> +	ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1,
> +			       &buf_status, sizeof(buf_status));
> +	if (ret) {
> +		dev_err(data->dev, "Error reading buffer status\n");
> +		return ret;
> +	}
> +
> +	fifo_bytes = le16_to_cpu(buf_status);
> +	fifo_bytes &= data->chip_info->buf_smp_lvl_mask;

Does this mask guarantee the fifo_bytes can't be bigger than the 
allocated buffer no matter what is read from I2C?

I am asking this because it's not uncommon to see some noise in I2C, or 
read 0xff 0xff, ... if IC is for example resetting. Hence, I would sleep 
a tad better if I knew the fifo_bytes read from IC is not 100% trusted - 
but is verified against known maximum number of samples.

This does not matter with the KX022A because the size of max number of 
samples exceeds the value indicated by 0xff (the 8bit SMP_LVL register).

Please correct me if I am wrong but it seems to me we use 10bits of data 
from smp_lvl register on KX132. Hence, the maximum value if all bits are 
set is 1023. This equals to 1023/6bytes samples - which is roughly 170. 
I think the FIFO size was defined to be maximum of 86 samples on KX132 - 
which means that if we have some bogus value from I2C, we may overwrite 
allocated buffer. Hence we might want to ensure the fifo_bytes is never 
bigger than the fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES - no matter 
what we read from I2C.

> +
> +	return fifo_bytes;
> +}
> +

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~