linux-kernel - Re: [PATCH 08/10] irqchip/gic-v3-its: Decouple its initialization from gic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Nov 2018 08:09:28 +0000
From:   "Richter, Robert" <Robert.Richter@...ium.com>
To:     Julien Thierry <julien.thierry@....com>
CC:     Marc Zyngier <marc.zyngier@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Stuart Yoder <stuyoder@...il.com>,
        Laurentiu Tudor <laurentiu.tudor@....com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Will Deacon <will.deacon@....com>,
        Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>
Subject: Re: [PATCH 08/10] irqchip/gic-v3-its: Decouple its initialization
 from gic

On 08.11.18 11:26:02, Julien Thierry wrote:
> On 07/11/18 22:03, Robert Richter wrote:

> >-static int __init its_init(void)
> >+int __init its_init(void)
> >  {
> >      struct its_node *its;
> >      bool has_v4 = false;
> >      int err;
> >
> >+     if (list_empty(&its_probed))
> >+             return 0;
> >+
> >+     raw_spin_lock(&its_lock);
> >+redo:
> >+     list_for_each_entry(its, &its_probed, entry) {
> >+             list_del_init(&its->entry);
> >+
> >+             raw_spin_unlock(&its_lock);
> >+
> >+             /* Needs to be called in non-atomic context */
> >+             err = its_init_one(its);
> >+             if (err)
> >+                     its_free(its);
> >+
> >+             raw_spin_lock(&its_lock);
> >+
> >+             if (!err)
> >+                     list_add_tail(&its->entry, &its_nodes);
> >+
> >+             goto redo;
> 
> Again, you're starting a loop only to work on the first element and then
> restarting the loop.
> 
> Just do a while (!list_empty()), and without gotos...

When writing the code I have looked into an alternative to
list_for_each_entry() and did not find a better way that works with
proper locking. list_first_entry() requires the list being non-empty.
If you set the lock after list_empty() it is not granted the list is
actual empty. See also my other comment in an earlier mail.

-Robert

> 
> >+     }
> >+
> >+     raw_spin_unlock(&its_lock);
> >+
> >      if (list_empty(&its_nodes)) {
> >              pr_warn("ITS: No ITS available, not enabling LPIs\n");
> >              return -ENXIO;
> >      }
> >
> >      err = allocate_lpi_tables();
> >-     if (err)
> >+     if (err) {
> >+             pr_warn("ITS: Failed to initialize (%d), not enabling LPIs\n",
> >+                     err);
> >              return err;
> >+     }