Re: [PATCH] PNP: Switch from __check_region() to __request_region()

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Monday, December 08, 2014 10:01:57 PM Jakub Sitnicki wrote:
> PNP core is the last user of the __check_region() which has been
> deprecated for almost 12 years (since v2.5.54). Replace it with a combo
> of __request_region() followed by __release_region().
> 
> pnp_check_port() and pnp_check_mem() remain racy after this change.
> 
> Signed-off-by: Jakub Sitnicki <jsitnicki@gmail.com>
> ---
> 
> There was a previous attempt at making this change 3 years ago but the
> author has never addressed the review comments:
> 
>   https://lkml.org/lkml/2011/8/12/216
> 
> The end goal here is to get rid of __check_region() which lands in
> every kernel image because of the PNP core.
> 
> It has been previously pointed out that replacing __check_region()
> with request_region() obscures the fact that pnp_check_port() is racy:
> 
>   https://lkml.org/lkml/2011/8/11/466
> 
> Because of that I've also considered just moving __check_region() to
> PNP core. However, that would require making free_resource() an
> exported symbol in kernel/resource.c.
> 
> On the other hand, a switch to request/release_region() makes
> pnp_check_port() and pnp_check_mem() follow the same pattern as found
> in pnp_check_irq() and pnp_check_dma():
> 
> 	if (!dev->active) {
> 		if (request_<resource type>(...))
> 			return 0;
> 		free_<resource type>(...);
> 	}
>   
> Admittedly, I was not able to exercise the touched code paths on a
> commodity x86_64 laptop or under QEMU / VirtualBox (which lack ISA PnP
> support, AFAIK).
> 
> To my understanding, the correct way to test pnp_check_port() or
> pnp_check_mem() would be by issuing either:
> 
>   $ echo fill >/sys/bus/pnp/devices/XX:YY/resources
> or
>   $ echo auto >/sys/bus/pnp/devices/XX:YY/resources
> 
> ... but only if the device is not attached or active, which is not the
> case for ACPI PnP devices on my machines. If anyone can provide hints
> on steps to test this, I will be glad to do so.
> 
>  drivers/pnp/resource.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pnp/resource.c b/drivers/pnp/resource.c
> index 782e822..f980ff7 100644
> --- a/drivers/pnp/resource.c
> +++ b/drivers/pnp/resource.c
> @@ -179,8 +179,9 @@ int pnp_check_port(struct pnp_dev *dev, struct resource *res)
>  	/* check if the resource is already in use, skip if the
>  	 * device is active because it itself may be in use */
>  	if (!dev->active) {
> -		if (__check_region(&ioport_resource, *port, length(port, end)))
> +		if (!request_region(*port, length(port, end), "pnp"))
>  			return 0;
> +		release_region(*port, length(port, end));

Shouldn't we also release the resource returned by request_region() if it is
not NULL?

>  	}
>  
>  	/* check if the resource is reserved */
> @@ -241,8 +242,9 @@ int pnp_check_mem(struct pnp_dev *dev, struct resource *res)
>  	/* check if the resource is already in use, skip if the
>  	 * device is active because it itself may be in use */
>  	if (!dev->active) {
> -		if (check_mem_region(*addr, length(addr, end)))
> +		if (!request_mem_region(*addr, length(addr, end), "pnp"))
>  			return 0;
> +		release_mem_region(*addr, length(addr, end));
>  	}
>  
>  	/* check if the resource is reserved */
>