On Monday, December 22, 2014 11:34:48 PM Rafael J. Wysocki wrote:
> On Monday, December 22, 2014 12:19:44 PM Jakub Sitnicki wrote:
> > 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?
>
> I'll queue it up for 3.20 later in January.
Done now.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
