Re: [PATCH v4 1/4] ACPICA: actbl2.h: Add bit constants of the Flags field in the IOVT IOMMU structure

[Huacai Chen]

On Thu, Nov 5, 2026 at 7:22 PM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
>
>
>
> On 2026/11/5 下午6:31, Huacai Chen wrote:
> > On Thu, Nov 5, 2026 at 5:49 PM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 2026/11/5 下午5:31, Huacai Chen wrote:
> >>> On Thu, Nov 5, 2026 at 5:14 PM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2026/10/23 下午3:59, Huacai Chen wrote:
> >>>>> Bibo,
> >>>>>
> >>>>> Now I have applied this series but with some modifications, please
> >>>>> double check and test the code.
> >>>>>
> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/chenhuacai/linux-loongson.git/log/?h=loongarch-next
> >>>> In my v4 submit, it is:
> >>>> +               case ACPI_IOVT_DEVICE_ENTRY_START:
> >>>> +                       is_start = true;
> >>>> +                       start_devid = ep->device_id;
> >>>> +                       break;
> >>>> however in rc1, it is modified as:
> >>>>                    case ACPI_IOVT_DEVICE_ENTRY_START:
> >>>>                            is_start = true;
> >>>>                            entry->start_devid = ep->device_id;
> >>>>                            break;
> >>>> Here entry is not allocated and NULL pointer, it is allocated in
> >>>> ACPI_IOVT_DEVICE_ENTRY_END switch case.
> >>>>
> >>>> I really do not know what is aid to modify it and let others to test.
> >>> Sorry, it's my fault.
> >>>
> >>> In theory I should point out the modification (eliminating the local
> >>> variable start_devid) and refresh a new version. But we have no time
> >>> to do this because when the first patch is merged into the ACPI repo
> >>> it is near the merge window.
> >> Maybe our work style is different, I do not care much for the coding style.
> >>>
> >>> If you agree, I will submit such modifications to fix it.
> >> fix is welcome.
> >>>
> >>> diff --git a/drivers/acpi/loongarch/iovt.c b/drivers/acpi/loongarch/iovt.c
> >>> index 87cee4f0fc80..4cdac28a1fd0 100644
> >>> --- a/drivers/acpi/loongarch/iovt.c
> >>> +++ b/drivers/acpi/loongarch/iovt.c
> >>> @@ -115,6 +115,10 @@ static int __init iovt_add_iommu(struct
> >>> acpi_iovt_iommu *iommu)
> >>>           for (i = 0; i < iommu->device_entry_num; i++) {
> >>>                   switch (ep->type) {
> >>>                   case ACPI_IOVT_DEVICE_ENTRY_START:
> >>> +                       entry = kzalloc_obj(struct iovt_device_entry,
> >>> GFP_ATOMIC);
> >> There will be memory leak problem if there are START/START/END in the table.
> > That means a broken table? I think we don't need to consider broken
> > tables, because there may be kinds of broken pattern rather than just
> > a START/START/END.
> yes, START/END/START, START/START/END, START/SINGLE/END. what is the
> relationship between "don't need to consider" and kinds of broken pattern?
>
> what is the policy from your side with such BIOS table? let memory leak
> and continue?
The driver can only work with a good table or no table, but not with a
broken table.

Take START1/START2/END as an example of broken table: in your original
code, the END entry will be pairing with START2. But for a broken
table who can ensure they are the correct pair? If the real correct
pair is START1 and END, then (entry->start_devid, entry->end_devid)
will be a wrong device range. Such mistakes are much worse than a
memery leak.

If you want to say "I don't care about correct pairing, memory leak is
bad in any case.", OK, we can do it like this:

diff --git a/drivers/acpi/loongarch/iovt.c b/drivers/acpi/loongarch/iovt.c
index 87cee4f0fc80..ebd11e22c9be 100644
--- a/drivers/acpi/loongarch/iovt.c
+++ b/drivers/acpi/loongarch/iovt.c
@@ -115,6 +115,13 @@ static int __init iovt_add_iommu(struct
acpi_iovt_iommu *iommu)
        for (i = 0; i < iommu->device_entry_num; i++) {
                switch (ep->type) {
                case ACPI_IOVT_DEVICE_ENTRY_START:
+                       if (is_start)
+                               break;
+
+                       entry = kzalloc_obj(struct iovt_device_entry,
GFP_ATOMIC);
+                       if (!entry)
+                               return -ENOMEM;
+
                        is_start = true;
                        entry->start_devid = ep->device_id;
                        break;
@@ -122,10 +129,6 @@ static int __init iovt_add_iommu(struct
acpi_iovt_iommu *iommu)
                        if (!is_start)
                                break;

-                       entry = kzalloc_obj(struct iovt_device_entry,
GFP_ATOMIC);
-                       if (!entry)
-                               return -ENOMEM;
-
                        is_start = false;
                        entry->end_devid = ep->device_id;
                        list_add_tail(&entry->list, &np->ep_list);

Huacai

>
> >
> > Huacai
> >
> >>
> >> Regards
> >> Bibo Mao
> >>> +                       if (!entry)
> >>> +                               return -ENOMEM;
> >>> +
> >>>                           is_start = true;
> >>>                           entry->start_devid = ep->device_id;
> >>>                           break;
> >>> @@ -122,10 +126,6 @@ static int __init iovt_add_iommu(struct
> >>> acpi_iovt_iommu *iommu)
> >>>                           if (!is_start)
> >>>                                   break;
> >>>
> >>> -                       entry = kzalloc_obj(struct iovt_device_entry,
> >>> GFP_ATOMIC);
> >>> -                       if (!entry)
> >>> -                               return -ENOMEM;
> >>> -
> >>>                           is_start = false;
> >>>                           entry->end_devid = ep->device_id;
> >>>                           list_add_tail(&entry->list, &np->ep_list);
> >>>
> >>> Huacai
> >>>
> >>>>
> >>>>>
> >>>>> Huacai
> >>>>>
> >>>>> On Mon, Oct 12, 2026 at 3:16 PM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
> >>>>>>
> >>>>>> This change is already merged in acpica tree.
> >>>>>>
> >>>>>> slightly ping.
> >>>>>>
> >>>>>> On Tue, Sep 22, 2026 at 08:10:55PM +0800, Bibo Mao wrote:
> >>>>>>     > According to the IOVT specification, define macros for each bit of the
> >>>>>>     > Flags field in the IOVT IOMMU structure.
> >>>>>>     >
> >>>>>>     > Link: https://github.com/open-acpica/acpica/commit/352f750e4cc0
> >>>>>>     > Signed-off-by: Bibo Mao <maobibo@xxxxxxxxxxx>
> >>>>>>     > ---
> >>>>>>     >  include/acpi/actbl2.h | 7 +++++++
> >>>>>>     >  1 file changed, 7 insertions(+)
> >>>>>>     >
> >>>>>>     > diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> >>>>>>     > index baef525367b5..76e4a5bc79b4 100644
> >>>>>>     > --- a/include/acpi/actbl2.h
> >>>>>>     > +++ b/include/acpi/actbl2.h
> >>>>>>     > @@ -919,6 +919,13 @@ struct acpi_iovt_iommu {
> >>>>>>     >       u32 device_entry_offset;
> >>>>>>     >  };
> >>>>>>     >
> >>>>>>     > +/* Masks for Flags field above */
> >>>>>>     > +#define ACPI_IOVT_IOMMU_PCI_DEVICE           (1<<0)
> >>>>>>     > +#define ACPI_IOVT_IOMMU_PXM_VALID            (1<<1)
> >>>>>>     > +#define ACPI_IOVT_IOMMU_MAGAGE_BY_SEGMENT    (1<<2)
> >>>>>>     > +#define ACPI_IOVT_IOMMU_HW_CAP_SUPPORT               (1<<3)
> >>>>>>     > +#define ACPI_IOVT_IOMMU_MSI_INT_BYPASS               (1<<4)
> >>>>>>     > +
> >>>>>>     >  struct acpi_iovt_device_entry {
> >>>>>>     >       u8 type;
> >>>>>>     >       u8 length;
> >>>>>>     > --
> >>>>>>     > 2.39.3
> >>>>>>     >
> >>>>>>
> >>>>
> >>>>
> >>
>
>