expand_resource

kernel/resource.c int allocate_resource(struct resource *r · line 278

Quoted 6 lines · Matthew Wilcox

kernel/resource.c +6 -0

+/*
+ * Expand an existing resource by size amount.
+ */
+int expand_resource(struct resource *res, unsigned long size,
+			   unsigned long align)
+{

Could we have the kerneldoc stuff added to this function?

My main
question though is - what is the intended purpose of "align" ? As it is implemented, it seems to have a weird behaviour:

- if we expand the "end" of the resource, we round it towards an address which is a multiple of align, but "start" may not be a multiple of align.
- if we expand "start", we round it down towards a multiple of align. However, "end" may not be a multiple of align.

This may actually be of use to PCMCIA IO space handling - we only have two windows, but we may need to expand them if we have a multi- function card if we have more than 2 areas to map. In this case, we'd need to know whether "start" was expanded or "end" was expanded since we can't change the use of the already-allocated resource.

It may make sense to do something more generic, like:

int adjust_resource(struct resource *res, unsigned long start,

		    unsigned long end);

so that the caller knows what he's requesting and knows whether that change succeeded or failed. However, it is something I'd need to look deeper into when I have more time available to look at such stuff, so please don't take the above as a well thought-out solution.

Certainly. Rather than answering your question below directly, let's see if the kerneldoc answers it (and if not, then the kerneldoc needs to be made clearer).

/**
* expand_resource - Expand an existing resource
* @res: Resource already linked into the resource tree.
* @size: Amount of additional space requested below @res.
* @align: Required alignment of new space.
*
* This function is intended to be called when we need more space
* below an existing resource than currently exists. The @size and
* @align parameters describe the requirements of the child resource,
* not the parent. Note that this call does not attempt to expand
* the parent of @res as that might require reprogramming devices.
* If a caller would like this behaviour, they must handle the recursion. */

Quoted 9 lines

My main
question though is - what is the intended purpose of "align" ? As it is implemented, it seems to have a weird behaviour:

- if we expand the "end" of the resource, we round it towards an address which is a multiple of align, but "start" may not be a multiple of align.
- if we expand "start", we round it down towards a multiple of align. However, "end" may not be a multiple of align.

Heh, this sounds almost identical to the PARISC problem. I knew other users would come out of the woodwork if I sent the patch ;-) We also need to know whether the start or end was changed to reprogram the parent device -- see the caller in drivers/parisc/ccio-dma.c:

Quoted 5 lines

It may make sense to do something more generic, like:

int adjust_resource(struct resource *res, unsigned long start,

		    unsigned long end);

Quoted 4 lines

so that the caller knows what he's requesting and knows whether that change succeeded or failed. However, it is something I'd need to look deeper into when I have more time available to look at such stuff, so please don't take the above as a well thought-out solution.

I don't like that. It puts more work into the caller, but doesn't seem to simplify the _resource function.

Quoted 5 lines · Matthew Wilcox

/**
* expand_resource - Expand an existing resource
* @res: Resource already linked into the resource tree.
* @size: Amount of additional space requested below @res.
* @align: Required alignment of new space.

Quoted 18 lines

*
* This function is intended to be called when we need more space
* below an existing resource than currently exists. The @size and
* @align parameters describe the requirements of the child resource,
* not the parent. Note that this call does not attempt to expand
* the parent of @res as that might require reprogramming devices.
* If a caller would like this behaviour, they must handle the recursion. */

Quoted 9 lines

My main
question though is - what is the intended purpose of "align" ? As it is implemented, it seems to have a weird behaviour:

- if we expand the "end" of the resource, we round it towards an address which is a multiple of align, but "start" may not be a multiple of align.
- if we expand "start", we round it down towards a multiple of align. However, "end" may not be a multiple of align.

Ok, your description covers the latter case, and seems quite reasonable given the arguments. The former case seems to be a little less clear though.

The problem with expand_resource as it currently stands is that it can expand either at the start or the end of the resource, and we don't know which it is going to do. PCMCIA is one example where this information matters - not only do we need to know the right IO range for each IO window, but we also need to know the IO address for each device.

This means that if we were to expand these window resources, we must know where the new space was allocated.

I think we can both agree that saving the old resources start, end and then comparing it after expand_resource() is fairly disgusting.

	int new = expand_resource(res, size, align);

	if (new < 0) {
		return new;
	} else if (new == res->start) {
		/* program start */
	} else {
		/* program end */
	}