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

[Jakub Sitnicki]

On Fri, Dec 12, 2014 at 08:47 AM CET, Jakub Sitnicki wrote:
> Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> writes:
>
>> On Monday, December 08, 2014 10:01:57 PM Jakub Sitnicki wrote:
>>> 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?
>>
>
> Thanks for taking a look at this. I think we're good here. If you please
> bear with me for a moment:
>
> release_resource() removes an element from the list of resource parent's
> children (and makes it an orphan):
>
> 	p = &old->parent->child;
> 	for (;;) {
> 		tmp = *p;
> 		if (!tmp)
> 			break;
> 		if (tmp == old) {
> 			*p = tmp->sibling;
> 			old->parent = NULL;
> 			return 0;
> 		}
> 		p = &tmp->sibling;
> 	}
>
> release_region() does the same but with additional checks, and also
> frees the resource:
>
> 	p = &parent->child;
>         /* ... */
> 	for (;;) {
> 		struct resource *res = *p;
>
> 		if (!res)
> 			break;
> 		if (res->start <= start && res->end >= end) {
>                         /* ... */
> 			*p = res->sibling;
>                         /* ... */
> 			free_resource(res);
> 			return;
> 		}
> 		p = &res->sibling;
> 	}
>
> When making the change I've based on other code in the kernel which also
> make use of request_region().
>
> To quote one example, drivers/net/ethernet/8390/ne2k-pci.c cleans up its
> I/O port region when initialization fails like so:
>
> static int ne2k_pci_init_one(struct pci_dev *pdev,
> 			     const struct pci_device_id *ent)
> {
>         /* ... */
>
> 	if (request_region (ioaddr, NE_IO_EXTENT, DRV_NAME) == NULL) {
> 		dev_err(&pdev->dev, "I/O resource 0x%x @ 0x%lx busy\n",
> 			NE_IO_EXTENT, ioaddr);
> 		return -EBUSY;
> 	}
>
>         /* ... */
>
> 	dev = alloc_ei_netdev();
> 	if (!dev) {
> 		dev_err(&pdev->dev, "cannot allocate ethernet device\n");
> 		goto err_out_free_res;
> 	}
>
>         /* ... */
>
> err_out_free_res:
> 	release_region (ioaddr, NE_IO_EXTENT);
> 	return -ENODEV;
> }

Just wondering, do you have any further thoughts on this?

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