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

From: Richter, Robert
Date: Fri Nov 09 2018 - 03:09:33 EST


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;
> >+ }