Re: [PATCH 04/10] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Fabrice,

fabrice.gasnier@foss.st.com wrote on Tue, 1 Feb 2022 09:41:03 +0100:

> On 1/28/22 4:04 PM, Miquel Raynal wrote:
> > Hi Jonathan,
> > 
> > jic23@kernel.org wrote on Sat, 15 Jan 2022 16:06:19 +0000:
> >   
> >> On Thu, 16 Dec 2021 09:22:35 +0100
> >> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >>  
> >>> Hi Alexandru,
> >>>
> >>> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:
> >>>     
> >>>> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> >>>> <miquel.raynal@bootlin.com> wrote:      
> >>>>>
> >>>>> This is an internal variable of the core, let's use the
> >>>>> iio_buffer_enabled() helper which is exported for the following purpose:
> >>>>> telling if the current mode is a buffered mode, which is precisely what
> >>>>> this driver looks for.
> >>>>>
> >>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>> ---
> >>>>>  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
> >>>>>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>>> index 1cfefb3b5e56..a3b8827d3bbf 100644
> >>>>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>>> @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> >>>>>          * In continuous mode, use fast mode configuration,
> >>>>>          * if it provides a better resolution.
> >>>>>          */
> >>>>> -       if (adc->nconv == 1 && !trig &&
> >>>>> -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> >>>>> +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {        
> >>>>
> >>>> This may become tricky if other modes get added later.
> >>>> STM does a relatively good job in updating and re-using their drivers
> >>>> (even if some of them do look quirky sometimes).    
> >>
> >> Their hardware is crazy/complicated so tends to push the limits!
> >>  
> >>>>
> >>>> So, the question here would be: is "iio_buffer_enabled(indio_dev)"
> >>>> going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
> >>>> INDIO_BUFFER_HARDWARE get added?      
> >>>
> >>> I would argue, is this a real problem? Today iio_buffer_enabled() seem
> >>> to handle well what this driver is expecting. If tomorrow someone adds
> >>> another mode, that is his/her responsibility to state "okay, this
> >>> section is not common to all buffer styles *anymore*, so we need to do
> >>> a more fine grained check against ->currentmodes than
> >>> iio_buffer_enabled() does". In that case using the ->currentmodes
> >>> getter would be the right way to go, but only at that particular
> >>> moment, not today.    
> >>
> >> It should be isolated to this driver, so I think it is fine to use
> >> the broader check today, but I'll leave this to the st folks as
> >> it's their driver and I don't feel that strongly about it.  
> 
> Hi Miquel, Alexandru, Jonathan, all,
> 
> First, sorry for the delay.
> 
> Indeed, I don't expect any functional changes here by using
> iio_buffer_enabled(indio_dev).
> So it should be fine to use it. You're right, the driver looks for
> buffer mode in both places where this gets used.
> 
> Just an additional statement is: the driver also checks for no trigger,
> and single channel in both places (to select desired mode in the dfsdm).
> As I see, only INDIO_BUFFER_SOFTWARE is expected then.

Ok, thanks for the validation, do not hesitate to drop a Reviewed-by to
the next version of this series if you agree with the changes made here.

> For my own understanding (I'm just asking), why not using the
> currentmodes getter routine ?
> 
> I've had a look at the whole series [1], It seems used elsewhere. I may
> miss something... It would be 100% equivalent to current code to use:
> iio_get_internal_mode(indio_dev) & INDIO_BUFFER_SOFTWARE ?
> 
> This would be safe in case new modes gets introduced later ?
> (another note: unless these new modes gets set by default in the 'modes'
> field, this should have no impact here as well anyway ?)

I would argue that this is more a conceptual change. IMHO:
- currentmode is a variable that should have been kept internal
- checking against its value directly is kind of a hack and should be
  avoided when possible because we want the core to have full freedom
  over the way it manages these flags
- if you want to verify if buffers are enabled, then the core offers
  you a dedicated helper that does exactly this, and will do it better
  than if hardcoded by individual writers, generally

And it's not "used elsewhere" anymore thanks to this series :) only two
drivers _really_ need to check the actual current mode to do specific
actions, but that's all.

I hope it clarifies a bit.

Thanks,
Miquèl