linux-kernel - Re: [PATCH v2 1/3] iio: magnetometer: ak8975: Fix reading for ak099xx sensors

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: <45915CD6-A9BB-4071-ABCC-8DE76F7066C3@mainlining.org>
Date: Tue, 06 Aug 2024 19:54:56 +0200
From: Barnabás Czémán <barnabas.czeman@...nlining.org>
To: Jonathan Cameron <jic23@...nel.org>
CC: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Jonathan Albrieux <jonathan.albrieux@...il.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux@...nlining.org
Subject: Re: [PATCH v2 1/3] iio: magnetometer: ak8975: Fix reading for ak099xx sensors



On August 6, 2024 6:19:25 PM GMT+02:00, Jonathan Cameron <jic23@...nel.org> wrote:
>On Tue, 06 Aug 2024 08:10:18 +0200
>Barnabás Czémán <barnabas.czeman@...nlining.org> wrote:
>
>Hi Barnabás,
>
>Welcome to IIO.
>
>> ST2 register read should be placed after read measurment data,
>> because it will get correct values after it.
>
>What is the user visible result of this? Do we detect errors when none
>are there?  Do we have a datasheet reference for the status being
>update on the read command, not after the trigger?

Second read will fail. In the datasheet ST2 comes after measurment data read. Here is some explanation from datasheet.

"When ST2 register is read, AK09918 judges that data reading is finished. Stored measurement data is
protected during data reading and data is not updated. By reading ST2 register, this protection is
released. It is required to read ST2 register after data reading."

So if ST2 is read before measurment it will stuck at protected mode.
>>
>Needs a Fixes tag to let us know how far to backport the fix.
I think it is broken since 09912 was added but i cannot verify i have only devices with 09918.
>
>A few comments inline.
> 
>> Signed-off-by: Barnabás Czémán <barnabas.czeman@...nlining.org>
>> ---
>>  drivers/iio/magnetometer/ak8975.c | 31 +++++++++++++++----------------
>>  1 file changed, 15 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
>> index dd466c5fa621..925d76062b3e 100644
>> --- a/drivers/iio/magnetometer/ak8975.c
>> +++ b/drivers/iio/magnetometer/ak8975.c
>> @@ -692,22 +692,7 @@ static int ak8975_start_read_axis(struct ak8975_data *data,
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	/* This will be executed only for non-interrupt based waiting case */
>> -	if (ret & data->def->ctrl_masks[ST1_DRDY]) {
>> -		ret = i2c_smbus_read_byte_data(client,
>> -					       data->def->ctrl_regs[ST2]);
>> -		if (ret < 0) {
>> -			dev_err(&client->dev, "Error in reading ST2\n");
>> -			return ret;
>> -		}
>> -		if (ret & (data->def->ctrl_masks[ST2_DERR] |
>> -			   data->def->ctrl_masks[ST2_HOFL])) {
>> -			dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
>> -			return -EINVAL;
>> -		}
>> -	}
>> -
>This completely removes the check from the _fill_buffer() path
>
>> -	return 0;
>> +	return !(ret & data->def->ctrl_masks[ST1_DRDY]);
>returning a positive value here is unusual enough you should add a comment for
>the function + use that return value.
>
>>  }
>>  
>>  /* Retrieve raw flux value for one of the x, y, or z axis.  */
>> @@ -731,6 +716,20 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>>  	ret = i2c_smbus_read_i2c_block_data_or_emulated(
>>  			client, def->data_regs[index],
>>  			sizeof(rval), (u8*)&rval);
>No longer gated on ret & data->def->ctrl_masks[ST1_DRDY] which seems unintentional.
>
>Still need a check on ret here.
>
>> +	ret = i2c_smbus_read_byte_data(client,
>> +				       data->def->ctrl_regs[ST2]);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "Error in reading ST2\n");
>> +		goto exit;
>> +	}
>> +
>> +	if (ret & (data->def->ctrl_masks[ST2_DERR] |
>> +		   data->def->ctrl_masks[ST2_HOFL])) {
>> +		dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
>> +		ret = -EINVAL;
>> +		goto exit;
>> +	}
>> +
>>  	if (ret < 0)
>>  		goto exit;
>
>And this one ends up redundant I think which suggests to me the
>code is inserted a few lines early.
>
>>  
>> 
>