| Message ID | 20241107011015.2472600-2-quzicheng@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix phase range check in AD9832 and AD9834 drivers | expand |
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 >
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);
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(-)