Re: [PATCH 1/2] hwspinlock: reverse logic for used channels

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 2877 bytes --]


> > @@ -42,8 +42,8 @@
> >   * used as the ID's of the hwspinlock instances).
> >   *
> >   * The radix tree API supports tagging items in the tree, which this
> > - * framework uses to mark unused hwspinlock instances (see the
> > - * HWSPINLOCK_UNUSED tag above). As a result, the process of querying the
> > + * framework uses to mark used hwspinlock instances (see the
> > + * HWSPINLOCK_USED tag above). As a result, the process of querying the
> >   * tree, looking for an unused hwspinlock instance, is now reduced to a
> >   * single radix tree API call.
> >   */
> 
> [Severity: Low]
> Is the documentation internally contradictory here? Because the tag is now
> exclusively applied to USED locks, it seems impossible to query for an
> UNUSED lock with a single tag-based API call.

Can reword to "getting the hwspinlock state". The core doesn't offer to
look for just some unused hwlock. Only specific locks are looked for and
their state is queried.

> [Severity: High]
> This isn't a bug introduced by this patch, but does

Ignored.

> > @@ -482,9 +482,9 @@ static struct hwspinlock *hwspin_lock_unregister_single(unsigned int id)
> >  
> >  	mutex_lock(&hwspinlock_tree_lock);
> >  
> > -	/* make sure the hwspinlock is not in use (tag is set) */
> > -	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_UNUSED);
> > -	if (ret == 0) {
> > +	/* make sure the hwspinlock is not in use */
> > +	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_USED);
> > +	if (ret) {
> >  		pr_err("hwspinlock %d still in use (or not present)\n", id);
> >  		goto out;
> >  	}
> 
> [Severity: Low]
> Is the "(or not present)" portion of this error message unreachable?

Can reword.

> > @@ -798,8 +798,8 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
> >  	pm_runtime_put(dev);
> >  
> >  	/* mark this hwspinlock as available */
> > -	tmp = radix_tree_tag_set(&hwspinlock_tree, hwlock_to_id(hwlock),
> > -							HWSPINLOCK_UNUSED);
> > +	tmp = radix_tree_tag_clear(&hwspinlock_tree, hwlock_to_id(hwlock),
> > +				   HWSPINLOCK_USED);
> >  
> >  	/* sanity check (this shouldn't happen) */
> >  	WARN_ON(tmp != hwlock);
> 
> [Severity: Medium]
> Does hwspin_lock_free() now incorrectly return 1 instead of 0 on success?

The quoted code is the wrong block, but the issue is correct. Will fix.

> [Severity: Critical]
> This is a pre-existing issue, but does accessing hwlock->bank->num_locks

Ignored.

> [Severity: High]
> This is a pre-existing issue, but does hwspin_lock_unregister() fail to
> rollback on an error?

...

> [Severity: Critical]
> This is a pre-existing issue, but does devm_hwspin_lock_unreg() ignore the
> -EBUSY return value from hwspin_lock_unregister()?

Unregistering has _many_ problems. This will be addressed in a seperate
patch series.

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]