linux-kernel - Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52027F39.5050805@wwwdotorg.org>
Date:	Wed, 07 Aug 2013 11:09:13 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	Sonic Zhang <sonic.adi@...il.com>, Axel Lin <axel.lin@...ics.com>,
	Grant Likely <grant.likely@...aro.org>,
	Steven Miao <realmz6@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	buildroot-devel@...ckfin.uclinux.org,
	adi-buildroot-devel@...ts.sourceforge.net,
	Sonic Zhang <sonic.zhang@...log.com>
Subject: Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices

On 08/07/2013 10:23 AM, Linus Walleij wrote:
> On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang <sonic.adi@...il.com> wrote:
> 
> I'd like Stephen and Axel to have a look at this as well...
> 
>> From: Sonic Zhang <sonic.zhang@...log.com>
>>
>> in pinmux_disable_setting after current device fails to request
>> the same pins.
>>
>> Signed-off-by: Sonic Zhang <sonic.zhang@...log.com>
> 
> I don't quite understand the patch. Can you provide more context?

Yes, the commit description needs to describe the problem this solves.

I'm *guessing* the issue is:

Something tries to enable a new mux setting on some pins. One of those
pins is already owned by something else. So, applying the current
setting fails. So, pinctrl core attempts to unapply the partially
applied setting. This ends up incorrectly over-writing the conflicting
ownership of the pins with NULL, and hence forgetting about it.

I think a better change would be something more along the lines of:

  for (i = 0; i < num_pins; i++)
+ 	if (this_device_owns_pin(pins[i])
  		pin_free(pctldev, pins[i], NULL);

?

Where this_device_owns_pin() might be someting like:

	desc->owning_setting == setting

(which would be a new field that needed to be assigned during
pinmux_enable_setting).

Or perhaps the strcmp() is fine.

>> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
>> index 88cc509..9ebcf3b 100644
>> --- a/drivers/pinctrl/pinmux.c
>> +++ b/drivers/pinctrl/pinmux.c
>> @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting)
>>                                  pins[i]);
>>                         continue;
>>                 }
>> +               /* And release the pins */
>> +               if (desc->mux_usecount &&
>> +                       !strcmp(desc->mux_owner, setting->dev_name))
>> +                       pin_free(pctldev, pins[i], NULL);
>> +
>>                 desc->mux_setting = NULL;
>>         }
>>
>> -       /* And release the pins */
>> -       for (i = 0; i < num_pins; i++)
>> -               pin_free(pctldev, pins[i], NULL);
>> -
> 
> For pinmux_disable_setting() to inspect desc->mux_usecount seems
> assymetric. This is something pin_free() should do, shouldn't it?
> 
> Should not this codepath be kept and a change made inside pin_free()
> for the check above instead?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/