mfd: max8925: request resource region

If resource region isn't requested, component devices will fail to request their resources.

Signed-off-by Haojian Zhuang <[email protected]>

Quoted 4 lines · Haojian Zhuang

If resource region isn't requested, component devices will fail to request their resources.

Signed-off-by Haojian Zhuang <[email protected]>

This looks wrong.

This driver looks broken. ?IO regions are for PC IO stuff, not for I2C buses. ?There is no resource reservation system in the kernel for registers on I2C devices.

Quoted 11 lines · Russell King - ARM Linux

Quoted 4 lines · Haojian Zhuang

If resource region isn't requested, component devices will fail to request their resources.

Signed-off-by Haojian Zhuang <[email protected]>

This looks wrong.

This driver looks broken. ?IO regions are for PC IO stuff, not for I2C buses. ?There is no resource reservation system in the kernel for registers on I2C devices.

There's some components in PMIC devices. I used IO resources to identify these components,
and these IO resources are register offset in PMIC. But if I choose MEM resources, it may
conflict with real memory resources. So I choose IO resources. Do you have any other implementation
instead?

Quoted 4 lines · Haojian Zhuang

If resource region isn't requested, component devices will fail to request their resources.

Signed-off-by Haojian Zhuang <[email protected]>

This looks wrong.

This driver looks broken. IO regions are for PC IO stuff, not for I2C buses. There is no resource reservation system in the kernel for registers on I2C devices.

Quoted 11 lines · Russell King - ARM Linux

Quoted 4 lines · Haojian Zhuang

If resource region isn't requested, component devices will fail to request their resources.

Signed-off-by Haojian Zhuang <[email protected]>

This looks wrong.

This driver looks broken. IO regions are for PC IO stuff, not for I2C buses. There is no resource reservation system in the kernel for registers on I2C devices.

They've commonly been used for this, it's a fairly sane way for an MFD to communicate with its subdevices. The fix in this series isn't good, though - providing a parent resource with a suitable range for all the device resources should do the job much more sensibly. This isn't great but the infrastructure seems to do the right thing with it for now and it only requires a bit of reinterpretation of what IORESOURCE_IO means.

Quoted 6 lines · Mark Brown

They've commonly been used for this, it's a fairly sane way for an MFD to communicate with its subdevices. The fix in this series isn't good, though - providing a parent resource with a suitable range for all the device resources should do the job much more sensibly. This isn't great but the infrastructure seems to do the right thing with it for now and it only requires a bit of reinterpretation of what IORESOURCE_IO means.

It's an abuse. And it won't work unless PCMCIA, PCI or ISA is enabled.

This abuse must stop, and it must stop right now. And I really don't care about "it's commonly been used for XYZ" because it's a totally fucked idea.

What if you have two devices both claiming IO regions at 0? Hint: it fails.

It's buggered beyond belief and it needs to die right now, no questions about it. And anyone who supports this idea needs to be...

Quoted 2 lines

What if you have two devices both claiming IO regions at 0? Hint: it fails.

Don't think I've got any examples with regions beginning at 0 but other regions seem to not run into any problems with overlap. Note that all these drivers do with the regions is use them to look up the base register.

Quoted 7 lines · Mark Brown

Quoted 2 lines

What if you have two devices both claiming IO regions at 0? ?Hint: it fails.

Don't think I've got any examples with regions beginning at 0 but other regions seem to not run into any problems with overlap. ?Note that all these drivers do with the regions is use them to look up the base register.

ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI port number for relatively large values, but either ISA or PCMCIA or an arbitrary MFD for relatively small values" is absolutely crazy.

FWIW, there are resources you define in include/linux/mfd/wm831x/core.h that have values between 0x4000 and 0x8000, which is exactly the range occupied by PCI devices on my thinkpad. It's only a matter of time until someone puts conflicting devices into one machine.

? ? ? ?Arnd

Even we append a new IORESOURCE type. We still meet similar issue. So it means that we can't use any IORESOURCE for this kind of i2c usage. Is it right?

Can you explain why you need this kind of arbitration to start with? Can't you just ensure that each client of the max8925 only sees a fixed set of nonconflicting registers, and provide a higher-level abstractions for the registers that are indeed shared between clients?

Quoted 14 lines · Mark Brown

Quoted 4 lines · Arnd Bergmann

Can you explain why you need this kind of arbitration to start with? Can't you just ensure that each client of the max8925 only sees a fixed set of nonconflicting registers, and provide a higher-level abstractions for the registers that are indeed shared between clients?

This is nothing to do with arbitration or sharing. It's for the case where you have a set of IP blocks on the chip (and possibly over a series of different chips) all with the same register map within the IP block - you need a way to tell the function driver for the IP block where it is in the chip register map. A similar thing happens (without issue) for the interrupts within the chip.

Quoted 4 lines

You'd never expect any collisions to need arbitrating, it's purely about telling the function driver where to find the IP without having to open code this. Anything which is actually shared would be handled in the MFD core for the device normally, or with some other API like genirq.

The patch that we are discussing adds a call to 'request_region' -- that is the arbitration interface and that is what is causing the conflicts.

Using a 'struct resource' of type IORESOURCE_IO to pass information about non-PIO registers to child devices is inconsistent, ugly and confusing, but I agree it doesn't actually result in bugs. The problems start when you use those resources in combination with request_region, request_resource and ioport_map.

Quoted 4 lines · Mark Brown

You'd never expect any collisions to need arbitrating, it's purely about telling the function driver where to find the IP without having to open code this. Anything which is actually shared would be handled in the MFD core for the device normally, or with some other API like genirq.

The patch that we are discussing adds a call to 'request_region' -- that is the arbitration interface and that is what is causing the conflicts.

Using a 'struct resource' of type IORESOURCE_IO to pass information about non-PIO registers to child devices is inconsistent, ugly and confusing, but I agree it doesn't actually result in bugs. The problems

start when you use those resources in combination with request_region, request_resource and ioport_map.

Quoted 3 lines

Using a 'struct resource' of type IORESOURCE_IO to pass information about non-PIO registers to child devices is inconsistent, ugly and confusing, but I agree it doesn't actually result in bugs. The problems

I actually don't find it at all confusing, but I think that's mostly because at a high level I look at _IO as being about a register space and this is just a different register space.

The part that confuses me most about the abuse of IORESOURCE_IO is that these have no parent resource and are not registered or listed in /proc/ioports.

Quoted 4 lines · Mark Brown

Don't think I've got any examples with regions beginning at 0 but other regions seem to not run into any problems with overlap. Note that all these drivers do with the regions is use them to look up the base register.

ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI port number for relatively large values, but either ISA or PCMCIA or an arbitrary MFD for relatively small values" is absolutely crazy.

FWIW, there are resources you define in include/linux/mfd/wm831x/core.h that have values between 0x4000 and 0x8000, which is exactly the range occupied by PCI devices on my thinkpad. It's only a matter of time until someone puts conflicting devices into one machine.

There are two distinct problems here:

* conflicts in request_resource() between stuff that is in fundamentally different.
* defining the __io() address space to have a zero offset, which causes NULL pointer dereferences in legacy ISA drivers.

My feeling is that the resource model is just hasn't moved along with the times (predates and doesn't really map to the device model) and is used in a consistent way (request_resource, allocate_resource and request_region operate on the same types, but in different ways). It's not clear to me whether it makes sense to continue this path for new kinds of resources.

My understanding is also that the uses in MFD (e.g. max8925 and wm831x) are only interested in the aspect of passing information to child devices rather than arbitrating between conflicting accesses. If that's the case, a separate mechanism that doesn't use a global numbering scheme might be more appropriate.

Quoted 6 lines · Arnd Bergmann

There are two distinct problems here:

* conflicts in request_resource() between stuff that is in fundamentally different.
* defining the __io() address space to have a zero offset, which causes NULL pointer dereferences in legacy ISA drivers.

Right, though in the MFD case neither of these things should be relevant as the resources should never actually be used in this way.

Quoted 6 lines

My feeling is that the resource model is just hasn't moved along with the times (predates and doesn't really map to the device model) and is used in a consistent way (request_resource, allocate_resource and request_region operate on the same types, but in different ways). It's not clear to me whether it makes sense to continue this path for new kinds of resources.

This is true. On the other hand there's some infrastructure around resources which is pretty helpful, though now I look at it most of this is actually specific to platform devices rather than being a platform device wrapper for a generic thing. Things like platform_get_resource() are pretty good to use, and in the context of platform devices the whole resource conflict thing is normally pretty much irrelevant.

Quoted 5 lines

My understanding is also that the uses in MFD (e.g. max8925 and wm831x) are only interested in the aspect of passing information to child devices rather than arbitrating between conflicting accesses. If that's the case, a separate mechanism that doesn't use a global numbering scheme might be more appropriate.

Given what I'm saying about platform devices above perhaps we should be factoring some of the platform device stuff up to struct device level. Another option would be to work on separating the management of the number spaces and the interfaces for getting the numbers back out to make it easier to add more number spaces.

Quoted 5 lines · Mark Brown

Given what I'm saying about platform devices above perhaps we should be factoring some of the platform device stuff up to struct device level. Another option would be to work on separating the management of the number spaces and the interfaces for getting the numbers back out to make it easier to add more number spaces.

Isn't that what devres is for? We should be able to just attach arbitrary data to a device with this, e.g. a struct regmap to use for doing I/O that a driver can use. Maybe we should add some wrappers around that to make it more obvious to use.

Quoted 8 lines · Arnd Bergmann

Quoted 5 lines · Mark Brown

Given what I'm saying about platform devices above perhaps we should be factoring some of the platform device stuff up to struct device level. Another option would be to work on separating the management of the number spaces and the interfaces for getting the numbers back out to make it easier to add more number spaces.

Quoted 4 lines

Isn't that what devres is for? We should be able to just attach arbitrary data to a device with this, e.g. a struct regmap to use for doing I/O that a driver can use. Maybe we should add some wrappers around that to make it more obvious to use.

Not as far as I understand it, it's more about making error handling and unregistration code less error prone by automating the freeing of things when a device goes away or probe() fails. The managed versions wrap the unmanaged allocators but don't generally otherwise change the interface except by adding a struct device. Unless I'm missing something, of course.

Quoted 10 lines · Mark Brown

Quoted 9 lines · Arnd Bergmann

Quoted 2 lines · Mark Brown

Given what I'm saying about platform devices above perhaps we should be factoring some of the platform device stuff up to struct device level.

Isn't that what devres is for? We should be able to just attach arbitrary data to a device with this, e.g. a struct regmap to use for doing I/O that a driver can use. Maybe we should add some wrappers around that to make it more obvious to use.

Quoted 3 lines

Not as far as I understand it, it's more about making error handling and unregistration code less error prone by automating the freeing of things when a device goes away or probe() fails. The managed versions

Yes, it also does that, but have a look at how drivers/pci/pci.c uses devres to attach auxiliary attributes to a device. We can do the same thing for other devices to attach any data.

Yes, it also does that, but have a look at how drivers/pci/pci.c uses devres to attach auxiliary attributes to a device. We can do the same thing for other devices to attach any data.

Quoted 3 lines · Arnd Bergmann

Yes, it also does that, but have a look at how drivers/pci/pci.c uses devres to attach auxiliary attributes to a device. We can do the same thing for other devices to attach any data.

Actually, thinking about this further devres is definitely not what we want here: when the device is unbound it'll free all the resources it has for the device which is definitely not what we want for things like this. We need the data to persist for as long as the device exists.

How about a IORESOURCE_REGMAP type then with the following semantics:

Each struct regmap gets an embedded resource that gets a unique range in the IORESOURCE_REGMAP space using allocate_resource, and each device using that can have its own sub-resources registered to that.

Then we add a helper function that pulls out the regmap from the resource using something like container_of(res->parent, struct regmap, resource) and calculates the register number by subtracting the parent->start from res->start.

Quoted 1 line · Arnd Bergmann

How about a IORESOURCE_REGMAP type then with the following semantics:

The trick is allocating a bit for that flag, though I was thinking earlier on we might be able to get away with carving it out of the bus specific bits since I think anything using this wouldn't normally be using resources for anything else except interrupts. It does mean that the resource type doesn't do the right thing though.

Quoted 9 lines

Each struct regmap gets an embedded resource that gets a unique range in the IORESOURCE_REGMAP space using allocate_resource, and each device using that can have its own sub-resources registered to that.

Then we add a helper function that pulls out the regmap from the resource using something like container_of(res->parent, struct regmap, resource) and calculates the register number by subtracting the parent->start from res->start.

That feels complicated with the subtraction there, but modulo that this is roughly what's happening now. We would I think want this to work for non-regmap users too, it's not yet clear that every device with registers is going to fit into regmap (though it's looking more likely as things go on).

Quoted 5 lines · Mark Brown

The trick is allocating a bit for that flag, though I was thinking earlier on we might be able to get away with carving it out of the bus specific bits since I think anything using this wouldn't normally be using resources for anything else except interrupts. It does mean that the resource type doesn't do the right thing though.

Right, or we could try to kill IORESOURCE_BUS, which is hardly used anywhere and the users either get it wrong (bfin net2272), use it only in one file (acpi, broadcom-pci) or only for printing the contents (pnp).

Quoted 5 lines

That feels complicated with the subtraction there, but modulo that this is roughly what's happening now. We would I think want this to work for non-regmap users too, it's not yet clear that every device with registers is going to fit into regmap (though it's looking more likely as things go on).

I think the subtraction in some form is needed if you want to register the resources, to guarantee that they are non-conflicting. Registering them has the advantage that we can print them in a similar way to what we do for /proc/{ioports,iomem}.

It does result in the really odd situation that the number that gets stored in the resource is the register number plus an offset. If anything I'd expect that the resources would be defined so you had to add the parent base address with the child resource being the offset within the parent resource. Though to be honest the parent resource is almost always going to be numbered from zero so it's not going to make much difference most of the time.

Quoted 7 lines · Mark Brown

It does result in the really odd situation that the number that gets stored in the resource is the register number plus an offset. If anything I'd expect that the resources would be defined so you had to add the parent base address with the child resource being the offset within the parent resource. Though to be honest the parent resource is almost always going to be numbered from zero so it's not going to make much difference most of the time.

But that's not how any of the other resources work: AFAICT, each resource type makes up an address space with unique ranges assigned to each sibling and sub-ranges inside of that for its children.

That's what I'd expect, but you're saying that the values in the child resources are being stored as base+actual values (where base is some random number) rather than either actual values or as offsets within the base range either of which would seem less surprising. Presumably you'd also need to walk up the tree to subtract all the bases if there was more than one resource.

Perhaps I'm just totally failing to understand what you're saying?

I mean that each device providing resources of the new type would need to register its resources to the same root resource with non-conclicting

addresses. Having resources hanging in the air without a parent pointer is not good, although we seem to be doing that for a lot of platform devices these days, where we just pass the struct resource from platform code to a driver without ever registering it at each end.

Show Details

Quoted 2 lines · Arnd Bergmann

I mean that each device providing resources of the new type would need to register its resources to the same root resource with non-conclicting

Oh, right - I see what you mean. I was thinking of creating a per-chip parent for each tree rather than trying to make them a unified tree, though I guess it's useful for diagnostics. Multi-level resources might be slightly fun with this though I'm not sure how realistic they are.

Quoted 8 lines · Arnd Bergmann

Quoted 5 lines · Mark Brown

The trick is allocating a bit for that flag, though I was thinking earlier on we might be able to get away with carving it out of the bus specific bits since I think anything using this wouldn't normally be using resources for anything else except interrupts. It does mean that the resource type doesn't do the right thing though.

Quoted 4 lines

Right, or we could try to kill IORESOURCE_BUS, which is hardly used anywhere and the users either get it wrong (bfin net2272), use it only in one file (acpi, broadcom-pci) or only for printing the contents (pnp).

Yeah, I looked at that - PCMCIA seems to be the major sticking point and the usage there appeared totally sane, though it's possible that if I stare at it hard enough I might convince myself that it only needs seven bits and we can steal one.

Quoted 4 lines · Mark Brown

Yeah, I looked at that - PCMCIA seems to be the major sticking point and the usage there appeared totally sane, though it's possible that if I stare at it hard enough I might convince myself that it only needs seven bits and we can steal one.

Err what? PCMCIA doesn't use IORESOURCE_BUS. (Don't get it confused with IORESOURCE_BUSY which is totally different).

I think the subtraction in some form is needed if you want to register the resources, to guarantee that they are non-conflicting. Registering them has the advantage that we can print them in a similar way to what we do for /proc/{ioports,iomem}.

Instead, we should be having per-device resources trees. So, for instance, an I2C device with two functions, function 1 say owns register indices 0-0x3f and function 2 owns 0x40-0x7f. We should have:

struct i2c_blah_driver_info {
        struct resource reg_res;
};

        priv->reg_res.start = 0;
        priv->reg_res.end = 0x7f;
        priv->reg_res.flags = 0;

        subdev1->resource[0].start = 0;
        subdev1->resource[0].end = 0x3f;
        subdev1->resource[0].flags = 0;

        ret = request_resource(&priv->reg_res, &subdev1->resource[0]);
        /* check ret */

        subdev2->resource[0].start = 0x40;
        subdev2->resource[0].end = 0x7f;
        subdev2->resource[0].flags = 0;

        ret = request_resource(&priv->reg_res, &subdev2->resource[0]);
        /* check ret */

This means that we end up with lots of per-device resource trees rooted at the top-level-device driver, each effectively with their own separate name spaces.

Do we care about making them appear in procfs? I suspect at this stage that would be wrong. If we care at all, they should probably be per- top-level-device sysfs files - and we should have a function which sysfs can use along with the root resource to create that output.

Quoted 4 lines · Russell King - ARM Linux

Do we care about making them appear in procfs? I suspect at this stage that would be wrong. If we care at all, they should probably be per- top-level-device sysfs files - and we should have a function which sysfs can use along with the root resource to create that output.

If the resources have separate roots, I see no way how to make them all appear in the same procfs file. Having them represented in sysfs the way you describe sounds useful but not important.

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · Mark Brown

Don't think I've got any examples with regions beginning at 0 but other regions seem to not run into any problems with overlap. Note that all these drivers do with the regions is use them to look up the base register.

Quoted 3 lines

ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI port number for relatively large values, but either ISA or PCMCIA or an arbitrary MFD for relatively small values" is absolutely crazy.

So what you're saying is that it's nothing to do with zero and just about plain conflicts - there's nothing magic about zero (which is what Russell seemed to be suggesting)?

Quoted 3 lines · Mark Brown

So what you're saying is that it's nothing to do with zero and just about plain conflicts - there's nothing magic about zero (which is what Russell seemed to be suggesting)?

You've totally missed my point if that's what you think I was saying.

The resource system as it stands handles two types of resources:

1. MMIO resources, of type IORESOURCE_MEM, registered against the iomem_resource root resource
2. PCI/ISA IO resources, of type IORESOURCE_IO, registered against the ioport_resource root resource

The two root resources are hard coded into the BUSY-marking request/release APIs.

The problem comes if you start abusing IORESOURCE_IO - which has _always_ meant "this is a PCI/ISA IO resource", and then you have someone adding calls to request_region() etc. That will make stuff look at the ioport_resource root, and it won't work.

In any case, if the resource tree is disconnected from the main two parent resources, then the resource probably shouldn't have either IORESOURCE_IO nor IORESOURCE_MEM set in it. It's neither of those two resource types.

If resource region isn't requested, component devices will fail to request their resources.

Signed-off-by Haojian Zhuang <[email protected]>

---
 drivers/mfd/max8925-core.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)