| Message ID | 20250206-wip-bl-ad3552r-axi-v0-iio-testing-carlos-v1-1-863a4b2af4ea@baylibre.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | iio: dac: adi-axi-dac: drop io_mode check | expand |
On Thu, 06 Feb 2025 09:36:14 +0100 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Drop mode check, producing the following robot test warning: > > smatch warnings: > drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode() > warn: always true condition '(mode >= 0) => (0-u32max >= 0)' > > The range check results not useful since these are the only > plausible modes for enum ad3552r_io_mode. > > Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup") > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> Ah. I missed this. Anyhow made the same change directly so all is well than ends well! Thanks, Jonathan > --- > drivers/iio/dac/adi-axi-dac.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c > index ac4c96c4ccf3..bcaf365feef4 100644 > --- a/drivers/iio/dac/adi-axi-dac.c > +++ b/drivers/iio/dac/adi-axi-dac.c > @@ -728,9 +728,6 @@ static int axi_dac_bus_set_io_mode(struct iio_backend *back, > struct axi_dac_state *st = iio_backend_get_priv(back); > int ival, ret; > > - if (!(mode >= AD3552R_IO_MODE_SPI && mode <= AD3552R_IO_MODE_QSPI)) > - return -EINVAL; > - > guard(mutex)(&st->lock); > > ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, > > --- > base-commit: c667ed738d7d1a01e9b946ef47cae113b0a05bee > change-id: 20250206-wip-bl-ad3552r-axi-v0-iio-testing-carlos-070ecab6a52a > > Best regards,
On Sat, 2025-02-08 at 15:45 +0000, Jonathan Cameron wrote: > On Thu, 06 Feb 2025 09:36:14 +0100 > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > Drop mode check, producing the following robot test warning: > > > > smatch warnings: > > drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode() > > warn: always true condition '(mode >= 0) => (0-u32max >= 0)' > > > > The range check results not useful since these are the only > > plausible modes for enum ad3552r_io_mode. > > > > Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup") > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > Ah. I missed this. Anyhow made the same change directly so all is well > than ends well! > Hi Angelo, Jonathan, I wanted to reply to this one when I saw it but I haven't done right away and then totally forgot. Sorry about that! I don't really agree with the "fix" in this patch. AFAIU, smatch is complaining since the enum is apparently defaulting to an unsigned type which means doing the >= 0 check is useless. But we should keep the upper bound... - Nuno Sá
On Mon, 10 Feb 2025 10:05:47 +0000 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sat, 2025-02-08 at 15:45 +0000, Jonathan Cameron wrote: > > On Thu, 06 Feb 2025 09:36:14 +0100 > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > Drop mode check, producing the following robot test warning: > > > > > > smatch warnings: > > > drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode() > > > warn: always true condition '(mode >= 0) => (0-u32max >= 0)' > > > > > > The range check results not useful since these are the only > > > plausible modes for enum ad3552r_io_mode. > > > > > > Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup") > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > Ah. I missed this. Anyhow made the same change directly so all is well > > than ends well! > > > > Hi Angelo, Jonathan, > > I wanted to reply to this one when I saw it but I haven't done right away and > then totally forgot. Sorry about that! > > I don't really agree with the "fix" in this patch. AFAIU, smatch is complaining > since the enum is apparently defaulting to an unsigned type which means doing > the >= 0 check is useless. But we should keep the upper bound... Why? It's an enum so unless we are messing around with deliberate casts the compiler should always be able to spot this. The check may be needed on a future date if we add more types to that enum. So I agree the check wasn't terrible and perhaps acted as hardening but it isn't strictly speaking doing anything today. Jonathan > > - Nuno Sá >
On Mon, 2025-02-10 at 19:13 +0000, Jonathan Cameron wrote: > On Mon, 10 Feb 2025 10:05:47 +0000 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Sat, 2025-02-08 at 15:45 +0000, Jonathan Cameron wrote: > > > On Thu, 06 Feb 2025 09:36:14 +0100 > > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > Drop mode check, producing the following robot test warning: > > > > > > > > smatch warnings: > > > > drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode() > > > > warn: always true condition '(mode >= 0) => (0-u32max >= 0)' > > > > > > > > The range check results not useful since these are the only > > > > plausible modes for enum ad3552r_io_mode. > > > > > > > > Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup") > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > Ah. I missed this. Anyhow made the same change directly so all is well > > > than ends well! > > > > > > > Hi Angelo, Jonathan, > > > > I wanted to reply to this one when I saw it but I haven't done right away > > and > > then totally forgot. Sorry about that! > > > > I don't really agree with the "fix" in this patch. AFAIU, smatch is > > complaining > > since the enum is apparently defaulting to an unsigned type which means > > doing > > the >= 0 check is useless. But we should keep the upper bound... > > Why? It's an enum so unless we are messing around with deliberate casts the > compiler should always be able to spot this. The check may be needed on a > future I do not think the compiler will catch this: diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c index c1dae58c1975..5234dd5e227d 100644 --- a/drivers/iio/dac/ad3552r-hs.c +++ b/drivers/iio/dac/ad3552r-hs.c @@ -293,7 +293,7 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) * Back bus to simple SPI, this must be executed together with above * target mode unwind, and can be done only after it. */ - st->data->bus_set_io_mode(st->back, AD3552R_IO_MODE_SPI); + st->data->bus_set_io_mode(st->back, -1); A W=1 build (clang) did not complained at all... Maybe tools like smatch will. > date if we add more types to that enum. > > So I agree the check wasn't terrible and perhaps acted as hardening but it > isn't strictly speaking doing anything today. > It's not a very super important check, I agree... and being an enum will be easier to spot a raw value being passed during a review but since we already had the check, I don't see why we should remove it completely and not keep the upper bound. - Nuno Sá
On Tue, 11 Feb 2025 09:56:31 +0000 Nuno Sá <noname.nuno@gmail.com> wrote: > On Mon, 2025-02-10 at 19:13 +0000, Jonathan Cameron wrote: > > On Mon, 10 Feb 2025 10:05:47 +0000 > > Nuno Sá <noname.nuno@gmail.com> wrote: > > > > > On Sat, 2025-02-08 at 15:45 +0000, Jonathan Cameron wrote: > > > > On Thu, 06 Feb 2025 09:36:14 +0100 > > > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > Drop mode check, producing the following robot test warning: > > > > > > > > > > smatch warnings: > > > > > drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode() > > > > > warn: always true condition '(mode >= 0) => (0-u32max >= 0)' > > > > > > > > > > The range check results not useful since these are the only > > > > > plausible modes for enum ad3552r_io_mode. > > > > > > > > > > Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup") > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > Ah. I missed this. Anyhow made the same change directly so all is well > > > > than ends well! > > > > > > > > > > Hi Angelo, Jonathan, > > > > > > I wanted to reply to this one when I saw it but I haven't done right away > > > and > > > then totally forgot. Sorry about that! > > > > > > I don't really agree with the "fix" in this patch. AFAIU, smatch is > > > complaining > > > since the enum is apparently defaulting to an unsigned type which means > > > doing > > > the >= 0 check is useless. But we should keep the upper bound... > > > > Why? It's an enum so unless we are messing around with deliberate casts the > > compiler should always be able to spot this. The check may be needed on a > > future > > I do not think the compiler will catch this: > > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c > index c1dae58c1975..5234dd5e227d 100644 > --- a/drivers/iio/dac/ad3552r-hs.c > +++ b/drivers/iio/dac/ad3552r-hs.c > @@ -293,7 +293,7 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev > *indio_dev) > * Back bus to simple SPI, this must be executed together with above > * target mode unwind, and can be done only after it. > */ > - st->data->bus_set_io_mode(st->back, AD3552R_IO_MODE_SPI); > + st->data->bus_set_io_mode(st->back, -1); > > A W=1 build (clang) did not complained at all... Maybe tools like smatch will. > > > date if we add more types to that enum. > > > > So I agree the check wasn't terrible and perhaps acted as hardening but it > > isn't strictly speaking doing anything today. > > > > It's not a very super important check, I agree... and being an enum will be > easier to spot a raw value being passed during a review but since we already had > the check, I don't see why we should remove it completely and not keep the upper > bound. ok. I'd take a patch putting the upper bound back. Enums checking is an interesting hole to fall down. Jonathan > > - Nuno Sá
diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c index ac4c96c4ccf3..bcaf365feef4 100644 --- a/drivers/iio/dac/adi-axi-dac.c +++ b/drivers/iio/dac/adi-axi-dac.c @@ -728,9 +728,6 @@ static int axi_dac_bus_set_io_mode(struct iio_backend *back, struct axi_dac_state *st = iio_backend_get_priv(back); int ival, ret; - if (!(mode >= AD3552R_IO_MODE_SPI && mode <= AD3552R_IO_MODE_QSPI)) - return -EINVAL; - guard(mutex)(&st->lock); ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,