Hi all,

I am submitting two patches to address an issue in the AD9832 and AD9834
IIO drivers. The current implementation allows an invalid phase value of
4096 to pass due to incorrect range checking. The phase registers for
both devices are 12 bits, meaning valid values should range from 0 to
4095.

These patches modify the condition to use a greater-than-or-equal-to
check, ensuring only valid phase values are accepted.

Zicheng Qu (2):
  staging: iio: ad9834: Correct phase range check
  staging: iio: ad9832: Correct phase range check

 drivers/staging/iio/frequency/ad9832.c | 2 +-
 drivers/staging/iio/frequency/ad9834.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.34.1

Hi,

The previous link is: https://lore.kernel.org/linux-iio/20241105140359.2465656-1-quzicheng@huawei.com/T/#u

This patch (v2) addresses an issue in both the AD9832 and AD9834 drivers
where the phase range check allows an invalid value of 4096. The
condition has been corrected to ensure only valid 12-bit values (0-4095)
are accepted.

These changes ensure compliance with datasheet specifications and
prevent potential issues with incorrect phase settings.

Zicheng Qu (2):
  staging: iio: ad9834: Correct phase range check
  staging: iio: ad9832: Correct phase range check

 drivers/staging/iio/frequency/ad9832.c | 2 +-
 drivers/staging/iio/frequency/ad9834.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Thank you for reviewing this patch.

Best regards,

Zicheng
-- 
2.34.1

On Thu, 7 Nov 2024 01:10:13 +0000
Zicheng Qu <quzicheng@huawei.com> wrote:

> Hi,
> 
> The previous link is: https://lore.kernel.org/linux-iio/20241105140359.2465656-1-quzicheng@huawei.com/T/#u
> 
> This patch (v2) addresses an issue in both the AD9832 and AD9834 drivers
> where the phase range check allows an invalid value of 4096. The
> condition has been corrected to ensure only valid 12-bit values (0-4095)
> are accepted.
> 
> These changes ensure compliance with datasheet specifications and
> prevent potential issues with incorrect phase settings.
> 
> Zicheng Qu (2):
>   staging: iio: ad9834: Correct phase range check
>   staging: iio: ad9832: Correct phase range check
> 
>  drivers/staging/iio/frequency/ad9832.c | 2 +-
>  drivers/staging/iio/frequency/ad9834.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Thank you for reviewing this patch.
> 
For future reference, for IIO at least (and most of the kernel) don't
reply to previous version when posting a new version.  That tends to
make it likely your patch set will be lost way up the pile of email
in a maintainers in box.

Many maintainers start at latest and work backwards until they run out
of time :)

Jonathan

> Best regards,
> 
> Zicheng

User Perspective:
When a user sets the phase value, the ad9834_write_phase() is called.
The phase register has a 12-bit resolution, so the valid range is 0 to
4095. If the phase offset value of 4096 is input, it effectively exactly
equals 0 in the lower 12 bits, meaning no offset.

Reasons for the Change:
1) Original Condition (phase > BIT(AD9834_PHASE_BITS)):
This condition allows a phase value equal to 2^12, which is 4096.
However, this value exceeds the valid 12-bit range, as the maximum valid
phase value should be 4095.
2) Modified Condition (phase >= BIT(AD9834_PHASE_BITS)):
Ensures that the phase value is within the valid range, preventing
invalid datafrom being written.

Impact on Subsequent Logic: st->data = cpu_to_be16(addr | phase):
If the phase value is 2^12, i.e., 4096 (0001 0000 0000 0000), and addr
is AD9834_REG_PHASE0 (1100 0000 0000 0000), then addr | phase results in
1101 0000 0000 0000, occupying DB12. According to the section of WRITING
TO A PHASE REGISTER in the datasheet, the MSB 12 PHASE0 bits should be
DB11. The original condition leads to incorrect DB12 usage, which
contradicts the datasheet and could pose potential issues for future
updates if DB12 is used in such related cases.

Fixes: 12b9d5bf76bf ("Staging: IIO: DDS: AD9833 / AD9834 driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
---
 drivers/staging/iio/frequency/ad9834.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
index 47e7d7e6d920..6e99e008c5f4 100644
--- a/drivers/staging/iio/frequency/ad9834.c
+++ b/drivers/staging/iio/frequency/ad9834.c
@@ -131,7 +131,7 @@ static int ad9834_write_frequency(struct ad9834_state *st,
 static int ad9834_write_phase(struct ad9834_state *st,
 			      unsigned long addr, unsigned long phase)
 {
-	if (phase > BIT(AD9834_PHASE_BITS))
+	if (phase >= BIT(AD9834_PHASE_BITS))
 		return -EINVAL;
 	st->data = cpu_to_be16(addr | phase);
 
-- 
2.34.1

On Thu, Nov 07, 2024 at 01:10:14AM +0000, Zicheng Qu wrote:
> User Perspective:
> When a user sets the phase value, the ad9834_write_phase() is called.
> The phase register has a 12-bit resolution, so the valid range is 0 to
> 4095. If the phase offset value of 4096 is input, it effectively exactly
> equals 0 in the lower 12 bits, meaning no offset.
> 
> Reasons for the Change:
> 1) Original Condition (phase > BIT(AD9834_PHASE_BITS)):
> This condition allows a phase value equal to 2^12, which is 4096.
> However, this value exceeds the valid 12-bit range, as the maximum valid
> phase value should be 4095.
> 2) Modified Condition (phase >= BIT(AD9834_PHASE_BITS)):
> Ensures that the phase value is within the valid range, preventing
> invalid datafrom being written.
> 
> Impact on Subsequent Logic: st->data = cpu_to_be16(addr | phase):
> If the phase value is 2^12, i.e., 4096 (0001 0000 0000 0000), and addr
> is AD9834_REG_PHASE0 (1100 0000 0000 0000), then addr | phase results in
> 1101 0000 0000 0000, occupying DB12. According to the section of WRITING
> TO A PHASE REGISTER in the datasheet, the MSB 12 PHASE0 bits should be
> DB11. The original condition leads to incorrect DB12 usage, which
> contradicts the datasheet and could pose potential issues for future
> updates if DB12 is used in such related cases.
> 
> Fixes: 12b9d5bf76bf ("Staging: IIO: DDS: AD9833 / AD9834 driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
> ---

Fair enough.  Thanks.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

On Thu, 7 Nov 2024 13:32:42 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Thu, Nov 07, 2024 at 01:10:14AM +0000, Zicheng Qu wrote:
> > User Perspective:
> > When a user sets the phase value, the ad9834_write_phase() is called.
> > The phase register has a 12-bit resolution, so the valid range is 0 to
> > 4095. If the phase offset value of 4096 is input, it effectively exactly
> > equals 0 in the lower 12 bits, meaning no offset.
> > 
> > Reasons for the Change:
> > 1) Original Condition (phase > BIT(AD9834_PHASE_BITS)):
> > This condition allows a phase value equal to 2^12, which is 4096.
> > However, this value exceeds the valid 12-bit range, as the maximum valid
> > phase value should be 4095.
> > 2) Modified Condition (phase >= BIT(AD9834_PHASE_BITS)):
> > Ensures that the phase value is within the valid range, preventing
> > invalid datafrom being written.
> > 
> > Impact on Subsequent Logic: st->data = cpu_to_be16(addr | phase):
> > If the phase value is 2^12, i.e., 4096 (0001 0000 0000 0000), and addr
> > is AD9834_REG_PHASE0 (1100 0000 0000 0000), then addr | phase results in
> > 1101 0000 0000 0000, occupying DB12. According to the section of WRITING
> > TO A PHASE REGISTER in the datasheet, the MSB 12 PHASE0 bits should be
> > DB11. The original condition leads to incorrect DB12 usage, which
> > contradicts the datasheet and could pose potential issues for future
> > updates if DB12 is used in such related cases.
> > 
> > Fixes: 12b9d5bf76bf ("Staging: IIO: DDS: AD9833 / AD9834 driver")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
> > ---  
> 
> Fair enough.  Thanks.
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
Applied both patches to the fixes-togreg branch of iio.git.

Note they probably won't go upstream now until after rc1.

Thanks,

Jonathan

> 
> regards,
> dan carpenter
>

User Perspective:
When a user sets the phase value, the ad9832_write_phase() is called.
The phase register has a 12-bit resolution, so the valid range is 0 to
4095. If the phase offset value of 4096 is input, it effectively exactly
equals 0 in the lower 12 bits, meaning no offset.

Reasons for the Change:
1) Original Condition (phase > BIT(AD9832_PHASE_BITS)):
This condition allows a phase value equal to 2^12, which is 4096.
However, this value exceeds the valid 12-bit range, as the maximum valid
phase value should be 4095.
2) Modified Condition (phase >= BIT(AD9832_PHASE_BITS)):
Ensures that the phase value is within the valid range, preventing
invalid datafrom being written.

Impact on Subsequent Logic: st->data = cpu_to_be16(addr | phase):
If the phase value is 2^12, i.e., 4096 (0001 0000 0000 0000), and addr
is AD9832_REG_PHASE0 (1100 0000 0000 0000), then addr | phase results in
1101 0000 0000 0000, occupying DB12. According to the section of WRITING
TO A PHASE REGISTER in the datasheet, the MSB 12 PHASE0 bits should be
DB11. The original condition leads to incorrect DB12 usage, which
contradicts the datasheet and could pose potential issues for future
updates if DB12 is used in such related cases.

Fixes: ea707584bac1 ("Staging: IIO: DDS: AD9832 / AD9835 driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
---
 drivers/staging/iio/frequency/ad9832.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index 492612e8f8ba..140ee4f9c137 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -158,7 +158,7 @@ static int ad9832_write_frequency(struct ad9832_state *st,
 static int ad9832_write_phase(struct ad9832_state *st,
 			      unsigned long addr, unsigned long phase)
 {
-	if (phase > BIT(AD9832_PHASE_BITS))
+	if (phase >= BIT(AD9832_PHASE_BITS))
 		return -EINVAL;
 
 	st->phase_data[0] = cpu_to_be16((AD9832_CMD_PHA8BITSW << CMD_SHIFT) |
-- 
2.34.1