Message-ID: <56CC9489.7060706@virtuousgeek.org>
Date:	Tue, 23 Feb 2016 09:19:05 -0800
From:	Jesse Barnes <jbarnes@...tuousgeek.org>
To:	Simon Guinot <simon.guinot@...uanux.org>
Cc:	Alan Cox <alan@...ux.intel.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Vincent Pelletier <plr.vincent@...il.com>,
	Giel van Schijndel <me@...tis.eu>,
	linux-gpio@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Vincent Donnefort <vdonnefort@...il.com>,
	Yoann Sculo <yoann@...lo.fr>,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH] kernel/resource.c: fix muxed resource handling in
 __request_region()

On 02/23/2016 08:19 AM, Simon Guinot wrote:
> On Mon, Feb 22, 2016 at 12:46:09PM -0800, Jesse Barnes wrote:
>> On 02/22/2016 05:49 AM, Alan Cox wrote:
>>>> we have some good alternatives in the form of bus and platform
>>>> drivers that
>>>> can manage the appropriate serialization and keep things from
>>>> stomping
>>>> on one another.
>>>
>>> It's not used much, especially nowdays. The use case is basically multi
>>> I/O chips on the ISA/LPC bus with magic shared config register ports.
>>>
>>> We have sufficiently few of those we could give muxed the boot and
>>> special case them if preferred.
>>
>> Ah that's right, now I remember the context.  So where should we go from here then?  Just leave the ugly fix in or hack on old stuff and hope not to break it?
> 
> Hi Jesse,
> 
> The fix is not ugly but only incomplete. And I have to say that the
> whole IORESOURCE_MUXED thing is not shiny either :)

Yeah definitely, I'm not casting stones at you, this whole problem is an
ugly one. :)

As Alan said, muxed is really intended for a pretty limited set of
cases.  The "right" solution is a lot more work than its worth, so we
have this instead, which is fine.  But obviously it's been a little
trickier to put in place than we hoped. :)

> The main problem in __request_region() is that we are dropping the
> spinlock of the resource list while holding a reference on a resource,
> waiting for a muxed resource to become available.
> 
> From here, I can see two bugs:
> 
> 1 - At wake-up, the next __request_resource() iteration will not detect
>     a remaining conflict. To work properly, __request_resource() needs
>     to be called with a parent of the conflicting resource. Instead we
>     are passing the conflicting resource itself...
> 2 - At wake-up the resource pointer we are holding could have been
>     freed. Since we are just about to use this pointer to insert a new
>     resource in the linked list, it is broken...
> 
> My patch fixes 1 and makes things better for 2.
> 
> But I think Linus is right. If at wake-up we use the original parent
> resource to check again for a conflict, then we are safe.
> 
> If you want, I can propose a such patch.
> 
> Note that IORESOURCE_MUXED is mostly used by Super-I/Os drivers. A
> Super-I/O is a legacy I/O controller embedded on x86 motherboards. It is
> used to connect the low-bandwidth devices such as parallel ports, serial
> ports, keyboard controllers, hardware monitoring controllers, GPIO
> controllers, etc. While each logical device have its own memory region,
> a shared memory region is used for some configuration purpose.
> IORESOURCE_MUXED is a convenient way to deal with that. For some code
> examples you can look at the superio_* functions in the IT87 drivers:
> gpio/gpio-it87.c, hwmon/it87.c and watchdog/it87_wdt.c.
> 
> I am not aware of any other users for IORESOURCE_MUXED.
> 
> Let me know how you want to go and if you need my help.

I'd be happy if you proposed a patch.  It would also be nice if we could
somehow more formally limit the MUXED usage to these super I/O devices,
just so other users don't get into trouble thinking it's more general
than it is.

Thanks,
Jesse

Powered by blists - more mailing lists