[PATCH 0/2] Correct Phase Range Check for AD9832 and AD9834 Drivers

[PATCH 0/2] Correct Phase Range Check for AD9832 and AD9834 Drivers

[Zicheng Qu]

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

[PATCH 1/2] staging: iio: ad9834: Correct phase range check

[Zicheng Qu]

The phase register for the AD9834 is 12 bits, which means the valid
phase values range from 0 to 4095 (2^12 - 1). The current check uses a
greater-than condition with BIT(12), which equals 4096. This allows an
invalid phase value of 4096 to pass.

This patch corrects the check to use greater-than-or-equal-to, ensuring
that only phase values within the valid range are accepted.

Fixes: f1d05b5f68cb ("Staging: iio: Prefer using the BIT macro")
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

[PATCH 2/2] staging: iio: ad9832: Correct phase range check

[Zicheng Qu]

The phase register for the AD9832 is 12 bits, which means the valid
phase values range from 0 to 4095 (2^12 - 1). The current check uses a
greater-than condition with BIT(12), which equals 4096. This allows an
invalid phase value of 4096 to pass.

This patch corrects the check to use greater-than-or-equal-to, ensuring
that only phase values within the valid range are accepted.

Fixes: f1d05b5f68cb ("Staging: iio: Prefer using the BIT macro")
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

Re: [PATCH 1/2] staging: iio: ad9834: Correct phase range check

[Dan Carpenter]

On Tue, Nov 05, 2024 at 02:03:58PM +0000, Zicheng Qu wrote:
> The phase register for the AD9834 is 12 bits, which means the valid
> phase values range from 0 to 4095 (2^12 - 1). The current check uses a
> greater-than condition with BIT(12), which equals 4096. This allows an
> invalid phase value of 4096 to pass.
> 

I tried to figure out how this looks like to the user but I didn't
figure it out right away.  Please add this information to the commit
message.

> This patch corrects the check to use greater-than-or-equal-to, ensuring
> that only phase values within the valid range are accepted.
> 
> Fixes: f1d05b5f68cb ("Staging: iio: Prefer using the BIT macro")

The Fixes tag is wrong.  It should be:

Fixes: 12b9d5bf76bf ("Staging: IIO: DDS: AD9833 / AD9834 driver")

regards,
dan carpenter

Re: [PATCH 2/2] staging: iio: ad9832: Correct phase range check

[Dan Carpenter]

On Tue, Nov 05, 2024 at 02:03:59PM +0000, Zicheng Qu wrote:
> The phase register for the AD9832 is 12 bits, which means the valid
> phase values range from 0 to 4095 (2^12 - 1). The current check uses a
> greater-than condition with BIT(12), which equals 4096. This allows an
> invalid phase value of 4096 to pass.
> 
> This patch corrects the check to use greater-than-or-equal-to, ensuring
> that only phase values within the valid range are accepted.
> 
> Fixes: f1d05b5f68cb ("Staging: iio: Prefer using the BIT macro")

Same comments.

regards,
dan carpenter

[PATCH v2 0/2] Fix phase range check in AD9832 and AD9834 drivers

[Zicheng Qu]

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

[PATCH v2 1/2] staging: iio: ad9834: Correct phase range check

[Zicheng Qu]

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

[PATCH v2 2/2] staging: iio: ad9832: Correct phase range check

[Zicheng Qu]

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

Re: [PATCH v2 1/2] staging: iio: ad9834: Correct phase range check

[Dan Carpenter]

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

Re: [PATCH v2 0/2] Fix phase range check in AD9832 and AD9834 drivers

[Jonathan Cameron]

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

Re: [PATCH v2 1/2] staging: iio: ad9834: Correct phase range check

[Jonathan Cameron]

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
>