Re: [PATCH] opp: Invalidate current opp when draining the opp list

[Beata Michalska <beata.michalska@arm.com>]

On Tue, Mar 09, 2021 at 10:01:21AM +0530, Viresh Kumar wrote:
> On 08-03-21, 18:14, Beata Michalska wrote:
> > > -bool _opp_remove_all_static(struct opp_table *opp_table)
> > > +/*
> > > + * Can't remove the OPP from under the lock, debugfs removal needs to happen
> > > + * lock less to avoid circular dependency issues. This must be called without
> > > + * the opp_table->lock held.
> > > + */
> > > +static int _opp_drain_list(struct opp_table *opp_table, bool dynamic)
> > >  {
> > > -	struct dev_pm_opp *opp;
> > > +	struct dev_pm_opp *opp, *current_opp = NULL;
> > > +	int count = 0;
> > > +
> > > +	while ((opp = _opp_get_next(opp_table, dynamic))) {
> > > +		if (opp_table->current_opp == opp) {
> > > +			/*
> > > +			 * Reached at current OPP twice, no other OPPs left. The
> > > +			 * last reference to current_opp is dropped from
> > > +			 * _opp_table_kref_release().
> > > +			 */
> > > +			if (current_opp)
> > > +				break;
> > > +
> > > +			current_opp = opp;
> > > +		}
> > Having a quick look at the code ...
> > Shouldn't the current_opp be moved at the end of the list ?
> > Otherwise there is a risk of leaving unreferenced opps (and opp_table).
> 
> How exactly ? Note that it is expected that the OPP table isn't being
> used by anyone anymore at this point and all the users went away.
>
With the current version, once the _opp_get_next returns opp
that is the current_opp, the while loop will break, leaving all
the opps that are on the list after current_opp: _opp_get_next
is not actually getting the *next* entry from the list. If it
reaches the current_opp, the _next_ one will still be the same opp
as it will not be removed from the list at this point
( _opp_get_next is a simple for_each_entry and until the first entry
gets removed from the list it will always return the same one).
If the opp_table->current_opp gets pushed at the end of the list,
it should be safe to assume that if it gets reached for the second time,
it's the last one on the list).

The dev_pm_opp_remove_all_dynamic depends on the number of dynamic
opps removed from the list to drop the according number of references
on the opp_table. If the count does not correspond to number of calls
to dev_pm_opp_add, the final refcount on opp_table might not drop to
the point when the table will actually get released. And if the while
loop above drops earlier - this might be the case.

Also, I am not sure how probable is the case, but if the current_opp
has more references that the initial one and one from current_opp,
it might not get released properly either.

Or am I missing smth ?

> > Might be also worth adding warning (?)
> > 
> >     WARN_ONCE(!list_is_singular())
> 
> It is allowed for the list to contain both static and dynamic OPPs,
> and so the list may have more OPPs here.
> 

True, jumped to the idea too early and the _opp_table_kref_release
already warns on list not being empty.

---
BR.
B.
> -- 
> viresh