resource_len() utility function

PATCH In review David Brownell 3 participants 9 messages Oct 25, 2007

Subsystems (7)

USB Arm i.MX and Layerscape SPI Arm Samsung Exynos +3 more subsystems

lore

Share & tools

lore

David Brownell

author

2007-10-25 01:21 UTC #0 lore raw

PATCH 1/1 resource_len() utility function

Add a new resource_len() function, so drivers can start using this instead of driver-private code for a common idiom. The call can be useful with at least:

- request_region(), release_region()
- request_mem_region(), release_mem_region()
- ioremap()

Candidate drivers include those using platform or PNP busses, and maybe some others. PCI already has a similar function.

This patch also updates a representative set of drivers in two subsystems to use this call (SPI, and USB peripheral/gadget).

Signed-off-by David Brownell <[email protected]>

---
Despite comments that we have "needed" this for at least five years, this is pure cleanup. IMO it's appropriate for 2.6.25 and to simmer with the rest of the MM soup for a while.

 drivers/spi/atmel_spi.c           |    2 +-
 drivers/spi/omap2_mcspi.c         |    7 +++----
 drivers/spi/spi_imx.c             |    5 ++---
 drivers/spi/spi_mpc83xx.c         |    2 +-
 drivers/spi/spi_s3c24xx.c         |    4 ++--
 drivers/spi/spi_txx9.c            |    2 +-
 drivers/spi/xilinx_spi.c          |    5 ++---
 drivers/usb/gadget/at91_udc.c     |   12 +++++-------
 drivers/usb/gadget/fsl_usb2_udc.c |    9 ++++-----
 drivers/usb/gadget/omap_udc.c     |    6 +++---
 include/linux/ioport.h            |    5 +++++
 11 files changed, 29 insertions(+), 30 deletions(-)

include/linux/ioport.h +5 -0

@@ -22,6 +22,11 @@ struct resource {
 	struct resource *parent, *sibling, *child;
 };
 
+static inline resource_size_t __pure resource_len(const struct resource *r)
+{
+	return r->end + 1 - r->start;
+}
+
 struct resource_list {
 	struct resource_list *next;
 	struct resource *res;

drivers/usb/gadget/at91_udc.c +5 -7

@@ -1656,9 +1656,7 @@ static int __init at91udc_probe(struct p
 	if (!res)
 		return -ENXIO;
 
-	if (!request_mem_region(res->start,
-			res->end - res->start + 1,
-			driver_name)) {
+	if (!request_mem_region(res->start, resource_len(res), driver_name)) {
 		DBG("someone's using UDC memory\n");
 		return -EBUSY;
 	}

@@ -1670,9 +1668,9 @@ static int __init at91udc_probe(struct p
 	udc->pdev = pdev;
 	udc->enabled = 0;
 
-	udc->udp_baseaddr = ioremap(res->start, res->end - res->start + 1);
+	udc->udp_baseaddr = ioremap(res->start, resource_len(res));
 	if (!udc->udp_baseaddr) {
-		release_mem_region(res->start, res->end - res->start + 1);
+		release_mem_region(res->start, resource_len(res));
 		return -ENOMEM;
 	}

@@ -1735,7 +1733,7 @@ static int __init at91udc_probe(struct p
 fail1:
 	device_unregister(&udc->gadget.dev);
 fail0:
-	release_mem_region(res->start, res->end - res->start + 1);
+	release_mem_region(res->start, resource_len(res));
 	DBG("%s probe failed, %d\n", driver_name, retval);
 	return retval;
 }

@@ -1761,7 +1759,7 @@ static int __exit at91udc_remove(struct 
 
 	iounmap(udc->udp_baseaddr);
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(res->start, res->end - res->start + 1);
+	release_mem_region(res->start, resource_len(res));
 
 	clk_put(udc->iclk);
 	clk_put(udc->fclk);

drivers/usb/gadget/fsl_usb2_udc.c +4 -5

@@ -2268,14 +2268,13 @@ static int __init fsl_udc_probe(struct p
 		return -ENXIO;
 	}
 
-	if (!request_mem_region(res->start, res->end - res->start + 1,
-				driver_name)) {
+	if (!request_mem_region(res->start, resource_len(res), driver_name)) {
 		ERR("request mem region for %s failed \n", pdev->name);
 		kfree(udc_controller);
 		return -EBUSY;
 	}
 
-	dr_regs = ioremap(res->start, res->end - res->start + 1);
+	dr_regs = ioremap(res->start, resource_len(res));
 	if (!dr_regs) {
 		ret = -ENOMEM;
 		goto err1;

@@ -2376,7 +2375,7 @@ err3:
 err2:
 	iounmap(dr_regs);
 err1:
-	release_mem_region(res->start, res->end - res->start + 1);
+	release_mem_region(res->start, resource_len(res));
 	kfree(udc_controller);
 	return ret;
 }

@@ -2405,7 +2404,7 @@ static int __exit fsl_udc_remove(struct 
 	dma_pool_destroy(udc_controller->td_pool);
 	free_irq(udc_controller->irq, udc_controller);
 	iounmap(dr_regs);
-	release_mem_region(res->start, res->end - res->start + 1);
+	release_mem_region(res->start, resource_len(res));
 
 	device_unregister(&udc_controller->gadget.dev);
 	/* free udc --wait for the release() finished */

drivers/usb/gadget/omap_udc.c +3 -3

@@ -2758,7 +2758,7 @@ static int __init omap_udc_probe(struct 
 
 	/* NOTE:  "knows" the order of the resources! */
 	if (!request_mem_region(pdev->resource[0].start,
-			pdev->resource[0].end - pdev->resource[0].start + 1,
+			resource_len(&pdev->resource[0]),
 			driver_name)) {
 		DBG("request_mem_region failed\n");
 		return -EBUSY;

@@ -2962,7 +2962,7 @@ cleanup0:
 	}
 
 	release_mem_region(pdev->resource[0].start,
-			pdev->resource[0].end - pdev->resource[0].start + 1);
+			resource_len(&pdev->resource[0]));
 
 	return status;
 }

@@ -3001,7 +3001,7 @@ static int __exit omap_udc_remove(struct
 	}
 
 	release_mem_region(pdev->resource[0].start,
-			pdev->resource[0].end - pdev->resource[0].start + 1);
+			resource_len(&pdev->resource[0]));
 
 	device_unregister(&udc->gadget.dev);
 	wait_for_completion(&done);

drivers/spi/atmel_spi.c +1 -1

@@ -657,7 +657,7 @@ static int __init atmel_spi_probe(struct
 	spin_lock_init(&as->lock);
 	INIT_LIST_HEAD(&as->queue);
 	as->pdev = pdev;
-	as->regs = ioremap(regs->start, (regs->end - regs->start) + 1);
+	as->regs = ioremap(regs->start, resource_len(regs));
 	if (!as->regs)
 		goto out_free_buffer;
 	as->irq = irq;

drivers/spi/omap2_mcspi.c +3 -4

@@ -964,8 +964,7 @@ static int __init omap2_mcspi_probe(stru
 		status = -ENODEV;
 		goto err1;
 	}
-	if (!request_mem_region(r->start, (r->end - r->start) + 1,
-			pdev->dev.bus_id)) {
+	if (!request_mem_region(r->start, resource_len(r), pdev->dev.bus_id)) {
 		status = -EBUSY;
 		goto err1;
 	}

@@ -1020,7 +1019,7 @@ err3:
 err2:
 	clk_put(mcspi->ick);
 err1a:
-	release_mem_region(r->start, (r->end - r->start) + 1);
+	release_mem_region(r->start, resource_len(r));
 err1:
 	spi_master_put(master);
 	return status;

@@ -1041,7 +1040,7 @@ static int __exit omap2_mcspi_remove(str
 	clk_put(mcspi->ick);
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(r->start, (r->end - r->start) + 1);
+	release_mem_region(r->start, resource_len(r));
 
 	spi_unregister_master(master);
 	kfree(dma_channels);

drivers/spi/spi_imx.c +2 -3

@@ -1487,15 +1487,14 @@ static int __init spi_imx_probe(struct p
 		status = -ENODEV;
 		goto err_no_iores;
 	}
-	drv_data->ioarea = request_mem_region(res->start,
-						res->end - res->start + 1,
+	drv_data->ioarea = request_mem_region(res->start, resource_len(res),
 						pdev->name);
 	if (drv_data->ioarea == NULL) {
 		dev_err(&pdev->dev, "probe - cannot reserve region\n");
 		status = -ENXIO;
 		goto err_no_iores;
 	}
-	drv_data->regs = ioremap(res->start, res->end - res->start + 1);
+	drv_data->regs = ioremap(res->start, resource_len(res));
 	if (drv_data->regs == NULL) {
 		dev_err(&pdev->dev, "probe - cannot map IO\n");
 		status = -ENXIO;

drivers/spi/spi_mpc83xx.c +1 -1

@@ -452,7 +452,7 @@ static int __init mpc83xx_spi_probe(stru
 	mpc83xx_spi->bitbang.master->setup = mpc83xx_spi_setup;
 	init_completion(&mpc83xx_spi->done);
 
-	mpc83xx_spi->base = ioremap(r->start, r->end - r->start + 1);
+	mpc83xx_spi->base = ioremap(r->start, resource_len(r));
 	if (mpc83xx_spi->base == NULL) {
 		ret = -ENOMEM;
 		goto put_master;

drivers/spi/spi_s3c24xx.c +2 -2

@@ -284,7 +284,7 @@ static int __init s3c24xx_spi_probe(stru
 		goto err_no_iores;
 	}
 
-	hw->ioarea = request_mem_region(res->start, (res->end - res->start)+1,
+	hw->ioarea = request_mem_region(res->start, resource_len(res),
 					pdev->name);
 
 	if (hw->ioarea == NULL) {

@@ -293,7 +293,7 @@ static int __init s3c24xx_spi_probe(stru
 		goto err_no_iores;
 	}
 
-	hw->regs = ioremap(res->start, (res->end - res->start)+1);
+	hw->regs = ioremap(res->start, resource_len(res));
 	if (hw->regs == NULL) {
 		dev_err(&pdev->dev, "Cannot map IO\n");
 		err = -ENXIO;

drivers/spi/spi_txx9.c +1 -1

@@ -382,7 +382,7 @@ static int __init txx9spi_probe(struct p
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	if (!res)
 		goto exit;
-	c->membase = ioremap(res->start, res->end - res->start + 1);
+	c->membase = ioremap(res->start, resource_len(res));
 	if (!c->membase)
 		goto exit;

drivers/spi/xilinx_spi.c +2 -3

@@ -341,13 +341,12 @@ static int __init xilinx_spi_probe(struc
 	xspi->bitbang.master->setup = xilinx_spi_setup;
 	init_completion(&xspi->done);
 
-	if (!request_mem_region(r->start,
-			r->end - r->start + 1, XILINX_SPI_NAME)) {
+	if (!request_mem_region(r->start, resource_len(r), XILINX_SPI_NAME)) {
 		ret = -ENXIO;
 		goto put_master;
 	}
 
-	xspi->regs = ioremap(r->start, r->end - r->start + 1);
+	xspi->regs = ioremap(r->start, resource_len(r));
 	if (xspi->regs == NULL) {
 		ret = -ENOMEM;
 		goto put_master;

Alan Cox

2007-10-25 01:41 UTC #1 lore raw

Quoted 8 lines · On Wed, 24 Oct 2007 18:20:52 -0700 David Brownell <[email protected]> wrote:

Add a new resource_len() function, so drivers can start using this instead of driver-private code for a common idiom. The call can be useful with at least:
- request_region(), release_region()
- request_mem_region(), release_mem_region()
- ioremap()

Quoted 5 lines

Candidate drivers include those using platform or PNP busses, and maybe some others. PCI already has a similar function.
This patch also updates a representative set of drivers in two subsystems to use this call (SPI, and USB peripheral/gadget).

PCI also increasingly is using functions that allow the user to choose to map a resource as a resource (eg pci_iomap). So is it better to have functions request_mem_resource(res) free_mem_resource(res) and similar instead or as well ?

Jeff Garzik

2007-10-25 02:08 UTC #2 lore raw

Quoted 14 lines · Alan Cox

Quoted 11 lines · On Wed, 24 Oct 2007 18:20:52 -0700 David Brownell <[email protected]> wrote:
Add a new resource_len() function, so drivers can start using this instead of driver-private code for a common idiom. The call can be useful with at least:
- request_region(), release_region()
- request_mem_region(), release_mem_region()
- ioremap()
Candidate drivers include those using platform or PNP busses, and maybe some others. PCI already has a similar function.

Quoted 7 lines

Quoted 2 lines
This patch also updates a representative set of drivers in two subsystems to use this call (SPI, and USB peripheral/gadget).
PCI also increasingly is using functions that allow the user to choose to map a resource as a resource (eg pci_iomap). So is it better to have functions request_mem_resource(res) free_mem_resource(res) and similar instead or as well ?

With regards to resource reservation... IMO we should mimic struct pci_dev and add struct resource[] to struct device.

Just like we have pci_request_regions(), we should also be able to easily to a dev_request_regions(). the implementation should be very close to pci_request_region() and pci_release_region().

Then a dev_iomap() analogue to pci_iomap() should be pretty straightforward to create.

Jeff

David Brownell

author

2007-10-25 02:47 UTC #3 lore raw

Quoted 2 lines · Jeff Garzik

With regards to resource reservation... IMO we should mimic struct pci_dev and add struct resource[] to struct device.

One minor difficulty: PCI has a limit on the number of BARs, but other busses don't. It'd be better as a "struct resource *".

Quoted 3 lines

Just like we have pci_request_regions(), we should also be able to easily to a dev_request_regions(). the implementation should be very close to pci_request_region() and pci_release_region().

Wouldn't it be nicer to have PCI use those dev_*() calls? Ideally pci_request_region() wraps dev_request_region()...

Quoted 2 lines

Then a dev_iomap() analogue to pci_iomap() should be pretty straightforward to create.

Another minor nit: addressing the various resource types. The platform bus code has multiple lookup schemes.

Calls like resource_iomap() might be more flexible, so that lookup schemes can stay flexible.

- Dave

Jeff Garzik

2007-10-25 03:47 UTC #4 lore raw

Quoted 11 lines · David Brownell

Quoted 1 line · Jeff Garzik
With regards to resource reservation... IMO we should mimic struct pci_dev and add struct resource[] to struct device.
One minor difficulty: PCI has a limit on the number of BARs, but other busses don't. It'd be better as a "struct resource *".
Quoted 1 line
Just like we have pci_request_regions(), we should also be able to easily to a dev_request_regions(). the implementation should be very close to pci_request_region() and pci_release_region().
Wouldn't it be nicer to have PCI use those dev_*() calls? Ideally pci_request_region() wraps dev_request_region()...

Absolutely...

Quoted 7 lines

Quoted 1 line
Then a dev_iomap() analogue to pci_iomap() should be pretty straightforward to create.
Another minor nit: addressing the various resource types. The platform bus code has multiple lookup schemes.
Calls like resource_iomap() might be more flexible, so that lookup schemes can stay flexible.

I figured that, in the absence of a true, defined BAR concept, the struct device version would simply index into the discussed array of struct resource. That means any ordering or layout of resources should work, presuming (the usual case) that both driver and platform agree on the resource layout.

Jeff

David Brownell

author

2007-10-25 04:38 UTC #5 lore raw

Quoted 6 lines · Jeff Garzik

Quoted 6 lines
Quoted 2 lines
Then a dev_iomap() analogue to pci_iomap() should be pretty straightforward to create.
Another minor nit: addressing the various resource types. The platform bus code has multiple lookup schemes.

Quoted 8 lines

Quoted 2 lines
Calls like resource_iomap() might be more flexible, so that lookup schemes can stay flexible.
I figured that, in the absence of a true, defined BAR concept, the struct device version would simply index into the discussed array of struct resource. That means any ordering or layout of resources should work, presuming (the usual case) that both driver and platform agree on the resource layout.

So you'd suggest having search utilities (as with platform_bus) returning resource indices not resources?

Thing is, BARs are usually well defined, but when folk glue resources together they use whatever order is convenient on that particular platform. And different platforms can have different numbers and types of resources, etc.

- Dave

Alan Cox

2007-10-25 12:27 UTC #6 lore raw

Quoted 2 lines

So you'd suggest having search utilities (as with platform_bus) returning resource indices not resources?

That seems a bad idea to me

Quoted 4 lines

Thing is, BARs are usually well defined, but when folk glue resources together they use whatever order is convenient on that particular platform. And different platforms can have different numbers and types of resources, etc.

Far better I think that pci_ functions that take BAR values end up as wrappers of the form

	pci_iomap(pdev, bar)
		return dev_iomap_resource(&pdev->resource[bar]);

David Brownell

author

2007-10-25 16:31 UTC #7 lore raw

Quoted 4 lines · Alan Cox

Quoted 2 lines
So you'd suggest having search utilities (as with platform_bus) returning resource indices not resources?
That seems a bad idea to me

I'm assuming you mean they should continue to work like they do today: return the resource. Your pseudocode below shows an iomap utility taking a resource.

Quoted 10 lines

Quoted 4 lines
Thing is, BARs are usually well defined, but when folk glue resources together they use whatever order is convenient on that particular platform. And different platforms can have different numbers and types of resources, etc.
Far better I think that pci_ functions that take BAR values end up as wrappers of the form
	pci_iomap(pdev, bar)
		return dev_iomap_resource(&pdev->resource[bar]);

Sure, for PCI ... where the meaning of BARs is a fixed part of the hardware spec, and it's not uncommon to skip a few.

But as I noted, that notion doesn't apply cleanly outside PCI; indexes aren't necessarily portable between systems. So any such interface should discourage their use.

One issue with a dev_iomap() is that only memory resources really need mapping ... but *all* of them need claiming. (Modulo the detail that the iomap morphs i/o addresses too.)

The $SUBJECT function is a (minor) simplification for both the mapping and claiming steps.

I think I'd rather see a dev_resource_claim() which combines the request_{,mem_}region() semantics with mapping ... that way drivers could save code, not just unify the two types of register addressing.

- Dave

David Brownell

author

2007-10-25 02:39 UTC #8 lore raw

In reply to Alan Cox

Quoted 14 lines · Alan Cox

Quoted 11 lines · On Wed, 24 Oct 2007 18:20:52 -0700 David Brownell <[email protected]> wrote:
Add a new resource_len() function, so drivers can start using this instead of driver-private code for a common idiom. The call can be useful with at least:
- request_region(), release_region()
- request_mem_region(), release_mem_region()
- ioremap()
Candidate drivers include those using platform or PNP busses, and maybe some others. PCI already has a similar function.

Quoted 7 lines

Quoted 2 lines
This patch also updates a representative set of drivers in two subsystems to use this call (SPI, and USB peripheral/gadget).
PCI also increasingly is using functions that allow the user to choose to map a resource as a resource (eg pci_iomap). So is it better to have functions request_mem_resource(res) free_mem_resource(res) and similar instead or as well ?

This was intended to be a minor band-aid. ;)

We already have request_resource(), which does something different than the request_*region() calls. I think calls with those names would complicate an already-too-strange interface, adding oddball siblings to request_resource().

I'd hope that when those resource calls were defined they made sense ... but to me, they don't do so today. Consider that the *typical* caller is given a "struct resource", and then to claim the specified address space it must convert it into a "start + length" representation before getting back a *NEW* "struct resource" ... with identical contents, other than the value of one all-but-undocumented flag bit. Then, if it's I/O space the address is usable already; but for memory space, it still needs an ioremap()...

Oh, and PCI has its own resource structs ("BAR") that don't look or act the same as other resources.

So while I like the notion of starting to abolish that conversion step, this wasn't an attempt to fix all the bizarre behaviors of the resource API.

I could imagine a call taking a resource and returning a "void __iomem *" to use for IO, which implicitly claims the region (in either memory or i/o space) and does any ioremap needed for memory space. With a sibling call to undo all that. If that's the answer, someone else should develop the patch and update drivers...

- Dave