[Mesa-dev] [PATCH 2/2] panfrost: Implement custom block/instruction iterators

[Boris Brezillon]

On Tue, 13 Aug 2019 12:59:03 -0700
Alyssa Rosenzweig <alyssa.rosenzweig at collabora.com> wrote:

> Could you explain a little bit more why this is neded? What do these
> helpers do that the generic list.h helpers can't?

The _safe attribute in generic helpers is just about handling the case
where the current item is removed from the list (or a new one is
inserted and you want to skip the new entry in your iteration). Some
users of the mir_foreach_xxx() iterators break this rule: they remove
entries beyond the current one. That's okay if the removed entry is
after the one stored in the temporary 'storage' param (also called
next/prev sometimes), but when one actually removes the next/prev
entry, the safe iterator ends up manipulating an element that's been
removed from the list (->{prev,next} might be invalid) or even worse,
something that's been released (use-after-free).

> Would it be possible
> to extend the shared list.h helpers (might this contain functionality
> interesting to other drivers as well)?

Hm, not without making them more complex I'm afraid. And since
most users are happy with the current implementation I'm not entirely
sure there's a need for such a feature.

> 
> What about the new next_ins argument -- should that always be required
> to specify?

AFAICT, it's only really needed in midgard_pair_load_store() to update
next_ins if the instruction we removed is next_ins. This being said, a
few functions were calling mir_next_op() manually and having next_ins
directly accessible saves us this extra call.

> Maybe we want a dedicated variant _with_next() to save the
> empty argument in the usual case (when we don't actually need it).

We can, but I also find it useful to expose this implementation detail
to users, so that they have a rough idea of what's happening behind the
scene (temporary storage pointing to the next instruction to allow
safe removal of the current item).