Messages in this thread | | | From | "Richter, Robert" <> | Subject | Re: [PATCH 08/10] irqchip/gic-v3-its: Decouple its initialization from gic | Date | Fri, 9 Nov 2018 08:09:28 +0000 |
| |
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; > >+ }
| |