Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Simon Guinot]

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 :)

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.

Simon

Attachment: signature.asc
Description: PGP signature