Re: [PATCH v3 0/3] iio: adc: xilinx-ams: refactor alarm handling to table-driven design

Re: [PATCH v3 0/3] iio: adc: xilinx-ams: refactor alarm handling to table-driven design

[Guilherme Ivo Bozi]

> This series looks fine to me.
>
> Given the AMD Xilinx folk are fairly active reviewers I'm just
> waiting on them taking a look.
Hi, just checking in on this series since it’s been some time.
Any updates from AMD Xilinx review or anything else needed from
my side?

--
Thanks,
Guilherme Ivo

Re: [PATCH v3 0/3] iio: adc: xilinx-ams: refactor alarm handling to table-driven design

[Michal Simek]

Hi Ivo,

On 5/12/26 13:00, Guilherme Ivo Bozi wrote:
>> This series looks fine to me.
>>
>> Given the AMD Xilinx folk are fairly active reviewers I'm just
>> waiting on them taking a look.
> Hi, just checking in on this series since it’s been some time.
> Any updates from AMD Xilinx review or anything else needed from
> my side?

Checking internally who can test this. Please give me some time.

Thanks,
Michal

RE: [PATCH v3 0/3] iio: adc: xilinx-ams: refactor alarm handling to table-driven design

[Erim, Salih]

Hi Guilherme,

> 
> > This series looks fine to me.
> >
> > Given the AMD Xilinx folk are fairly active reviewers I'm just waiting
> > on them taking a look.
> Hi, just checking in on this series since it’s been some time.
> Any updates from AMD Xilinx review or anything else needed from my side?

No, not needed, I am reviewing and will test it.

> 
> --
> Thanks,
> Guilherme Ivo

Salih.

Re: [PATCH v3 0/3] iio: adc: xilinx-ams: refactor alarm handling to table-driven design

[Salih Erim]

Hi Guilherme,

Thanks for working on this. The refactoring approach looks good overall,
but all three patches have indentation issues and patch 3 has one
semantic issue. Please see my comments on each patch.

On 4/14/2026 11:40 PM, Guilherme Ivo Bozi wrote:
> This series addresses significant code duplication in alarm handling
> logic across the Xilinx AMS IIO driver.
> 
> To address this, the series introduces a centralized table-driven
> mapping (alarm_map) that replaces multiple switch statements spread
> across the driver.
> 
> This improves:
> - maintainability (single source of truth for mappings)
> - readability (removes repeated switch logic)
> - extensibility (new alarms require only table updates)
> 
> No functional changes are intended.
> 
> Series overview:
> - Patch 1: fix out-of-bounds channel lookup
> - Patch 2: convert mutex handling to guard(mutex)
> - Patch 3: introduce table-driven alarm mapping
> 
> v1 -> v2:
> - Fixed Fixes tag format
> - Replaced AMS_ALARM_INVALID with AMS_ALARM_NONE
> - Changed alarm_map base_offset type
> 
> v2 -> v3:
> - Replace 'i >= num_channels' with 'i == num_channels'
> - Add missing trailing comma in alarm_map array initializer
> 
> Guilherme Ivo Bozi (3):
>    iio: adc: xilinx-ams: fix out-of-bounds channel lookup in event
>      handling
>    iio: adc: xilinx-ams: use guard(mutex) for automatic locking
>    iio: adc: xilinx-ams: refactor alarm mapping to table-driven approach
> 
>   drivers/iio/adc/xilinx-ams.c | 190 +++++++++++++----------------------
>   1 file changed, 71 insertions(+), 119 deletions(-)
> 
> --
> 2.47.3
> 

Salih

Re: [PATCH v3 1/3] iio: adc: xilinx-ams: fix out-of-bounds channel lookup in event handling

[Salih Erim]

Hi Guilherme,

Replies are inline.

On 4/14/2026 11:40 PM, Guilherme Ivo Bozi wrote:
> ams_event_to_channel() may return a pointer past the end of
> dev->channels when no matching scan_index is found. This can lead
> to invalid memory access in ams_handle_event().
> 
> Add a bounds check in ams_event_to_channel() and return NULL when
> no channel is found. Also guard the caller to safely handle this
> case.
> 
> Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> Signed-off-by: Guilherme Ivo Bozi <guilherme.bozi@usp.br>
> ---
>   drivers/iio/adc/xilinx-ams.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index 124470c92529..6191cd1b29a5 100644
> --- a/drivers/iio/adc/xilinx-ams.c
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -871,6 +871,9 @@ static const struct iio_chan_spec *ams_event_to_channel(struct iio_dev *dev,
>                  if (dev->channels[i].scan_index == scan_index)
>                          break;
> 
> +       if (i == dev->num_channels)
> +               return NULL;
> +
The added lines use spaces for indentation instead of tabs.

Salih

Re: [PATCH v3 2/3] iio: adc: xilinx-ams: use guard(mutex) for automatic locking

[Salih Erim]

Hi,

On 4/14/2026 11:40 PM, Guilherme Ivo Bozi wrote:
> Replace open-coded mutex_lock()/mutex_unlock() pairs with
> guard(mutex) to simplify locking and ensure proper unlock on
> all control flow paths.
> 
> This removes explicit unlock handling, reduces boilerplate,
> and avoids potential mistakes in error paths while keeping
> the behavior unchanged.
> 
> Signed-off-by: Guilherme Ivo Bozi <guilherme.bozi@usp.br>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> ---
>   drivers/iio/adc/xilinx-ams.c | 24 ++++++++----------------
>   1 file changed, 8 insertions(+), 16 deletions(-)

Same indentation issue -- spaces instead of tabs on all new lines.

Salih

Re: [PATCH v3 3/3] iio: adc: xilinx-ams: refactor alarm mapping to table-driven approach

[Salih Erim]

Hi,

On 4/14/2026 11:40 PM, Guilherme Ivo Bozi wrote:
> Replace multiple open-coded switch statements that map between
> scan_index, alarm bits, and register offsets with a centralized
> table-driven approach.
> 
> Introduce a struct-based alarm_map to describe the relationship
> between scan indices and alarm offsets, and add a helper to
> translate scan_index to event IDs. This removes duplicated logic
> across ams_get_alarm_offset(), ams_event_to_channel(), and
> ams_get_alarm_mask().
> 
> The new approach improves maintainability, reduces code size,
> and makes it easier to extend or modify alarm mappings in the
> future, while preserving existing behavior.
> 
> Signed-off-by: Guilherme Ivo Bozi <guilherme.bozi@usp.br>
> ---
>   drivers/iio/adc/xilinx-ams.c | 161 +++++++++++++----------------------
>   1 file changed, 58 insertions(+), 103 deletions(-)

Two issues:

1) Same tabs-vs-spaces indentation problem as patches 1 and 2.

2) In ams_event_to_channel():

> +       if (event < 0 || event >= ARRAY_SIZE(alarm_map))
> +               return NULL;

The 'event' parameter is u32, so 'event < 0' is always false.
Either drop the < 0 check or change the parameter type to int
if you intend to handle negative values.

Salih

Re: [PATCH v3 1/3] iio: adc: xilinx-ams: fix out-of-bounds channel lookup in event handling

[Guilherme Ivo Bozi]

Hi Salih,

Replies are inline.

On Tue, May 12, 2026 at 11:22 AM Salih Erim <salih.erim@amd.com> wrote:
>
> Hi Guilherme,
>
> Replies are inline.
>
> On 4/14/2026 11:40 PM, Guilherme Ivo Bozi wrote:
> > ams_event_to_channel() may return a pointer past the end of
> > dev->channels when no matching scan_index is found. This can lead
> > to invalid memory access in ams_handle_event().
> >
> > Add a bounds check in ams_event_to_channel() and return NULL when
> > no channel is found. Also guard the caller to safely handle this
> > case.
> >
> > Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> > Signed-off-by: Guilherme Ivo Bozi <guilherme.bozi@usp.br>
> > ---
> >   drivers/iio/adc/xilinx-ams.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> > index 124470c92529..6191cd1b29a5 100644
> > --- a/drivers/iio/adc/xilinx-ams.c
> > +++ b/drivers/iio/adc/xilinx-ams.c
> > @@ -871,6 +871,9 @@ static const struct iio_chan_spec *ams_event_to_channel(struct iio_dev *dev,
> >                  if (dev->channels[i].scan_index == scan_index)
> >                          break;
> >
> > +       if (i == dev->num_channels)
> > +               return NULL;
> > +
> The added lines use spaces for indentation instead of tabs.
I checked both locally and the raw mbox from lore.kernel.org, and the
indentation uses TAB characters consistently (^I in the diff).

To verify, I inspected the relevant hunk using cat -A:

^I^Iif (dev->channels[i].scan_index == scan_index)
^I^I^Ibreak;

+^Iif (i == dev->num_channels)
+^I^Ireturn NULL;

^Ireturn &dev->channels[i];

I could not observe any indentation issues locally or from the raw mbox.

>
> Salih
>

--
Guilherme Ivo

Re: [PATCH v3 3/3] iio: adc: xilinx-ams: refactor alarm mapping to table-driven approach

[Guilherme Ivo Bozi]

Hi Salih,

On Tue, May 12, 2026 at 11:34 AM Salih Erim <salih.erim@amd.com> wrote:
>
> Hi,
>
> On 4/14/2026 11:40 PM, Guilherme Ivo Bozi wrote:
> > Replace multiple open-coded switch statements that map between
> > scan_index, alarm bits, and register offsets with a centralized
> > table-driven approach.
> >
> > Introduce a struct-based alarm_map to describe the relationship
> > between scan indices and alarm offsets, and add a helper to
> > translate scan_index to event IDs. This removes duplicated logic
> > across ams_get_alarm_offset(), ams_event_to_channel(), and
> > ams_get_alarm_mask().
> >
> > The new approach improves maintainability, reduces code size,
> > and makes it easier to extend or modify alarm mappings in the
> > future, while preserving existing behavior.
> >
> > Signed-off-by: Guilherme Ivo Bozi <guilherme.bozi@usp.br>
> > ---
> >   drivers/iio/adc/xilinx-ams.c | 161 +++++++++++++----------------------
> >   1 file changed, 58 insertions(+), 103 deletions(-)
>
> Two issues:
>
> 1) Same tabs-vs-spaces indentation problem as patches 1 and 2.
>
> 2) In ams_event_to_channel():
>
> > +       if (event < 0 || event >= ARRAY_SIZE(alarm_map))
> > +               return NULL;
>
> The 'event' parameter is u32, so 'event < 0' is always false.
> Either drop the < 0 check or change the parameter type to int
> if you intend to handle negative values.
You are correct regarding the u32 type -- the event < 0 check is
unnecessary. I will remove it in the next revision.

>
> Salih

--
Guilherme Ivo

Re: [PATCH v3 1/3] iio: adc: xilinx-ams: fix out-of-bounds channel lookup in event handling

[Jonathan Cameron]

On Tue, 12 May 2026 12:40:05 -0300
Guilherme Ivo Bozi <guilherme.bozi@usp.br> wrote:

> Hi Salih,
> 
> Replies are inline.
> 
> On Tue, May 12, 2026 at 11:22 AM Salih Erim <salih.erim@amd.com> wrote:
> >
> > Hi Guilherme,
> >
> > Replies are inline.
> >
> > On 4/14/2026 11:40 PM, Guilherme Ivo Bozi wrote:  
> > > ams_event_to_channel() may return a pointer past the end of
> > > dev->channels when no matching scan_index is found. This can lead
> > > to invalid memory access in ams_handle_event().
> > >
> > > Add a bounds check in ams_event_to_channel() and return NULL when
> > > no channel is found. Also guard the caller to safely handle this
> > > case.
> > >
> > > Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> > > Signed-off-by: Guilherme Ivo Bozi <guilherme.bozi@usp.br>
> > > ---
> > >   drivers/iio/adc/xilinx-ams.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> > > index 124470c92529..6191cd1b29a5 100644
> > > --- a/drivers/iio/adc/xilinx-ams.c
> > > +++ b/drivers/iio/adc/xilinx-ams.c
> > > @@ -871,6 +871,9 @@ static const struct iio_chan_spec *ams_event_to_channel(struct iio_dev *dev,
> > >                  if (dev->channels[i].scan_index == scan_index)
> > >                          break;
> > >
> > > +       if (i == dev->num_channels)
> > > +               return NULL;
> > > +  
> > The added lines use spaces for indentation instead of tabs.  
> I checked both locally and the raw mbox from lore.kernel.org, and the
> indentation uses TAB characters consistently (^I in the diff).
> 
> To verify, I inspected the relevant hunk using cat -A:
> 
> ^I^Iif (dev->channels[i].scan_index == scan_index)
> ^I^I^Ibreak;
> 
> +^Iif (i == dev->num_channels)
> +^I^Ireturn NULL;
> 
> ^Ireturn &dev->channels[i];
> 
> I could not observe any indentation issues locally or from the raw mbox.
FWIW they look good to me as well. Salih, I'd guess you have a local issue.
b4 (on git.kernel.org) is really handy for ensuring none of those occur!

Jonathan

> 
> >
> > Salih
> >  
> 
> --
> Guilherme Ivo

Re: [PATCH v3 1/3] iio: adc: xilinx-ams: fix out-of-bounds channel lookup in event handling

[Salih Erim]

Hi Jonathan,
>> I could not observe any indentation issues locally or from the raw mbox.
> FWIW they look good to me as well. Salih, I'd guess you have a local issue.
> b4 (on git.kernel.org) is really handy for ensuring none of those occur!

You are right, apologies for the noise. The tabs were converted to
spaces by my email client during export. I will use b4 for patch
retrieval going forward.

There is still the u32 dead code issue in patch 3/3
(event < 0 check on a u32 parameter in ams_event_to_channel()),
Guilherme has acknowledged.

I am currently testing the series on ZCU102 hardware. Will follow up
with my Reviewed-by and Tested-by tags on the v4.

Salih

> 
> Jonathan
> 
>>
>>>
>>> Salih
>>>
>>
>> --
>> Guilherme Ivo
>