DM9000 issue with mem resource management

DM9000 issue with mem resource management

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

Hi everybody,

I ran into a resource-related bug in the DM9000 driver.

When the platform device has only 2 resources, dm9000_probe() doesn't set 
db->irq_res, which results in a segfault when the pointer gets dereferenced 
in dm9000_open().

I tried to fix the issue, and found out that the resource management code is 
quite broken.

If I understand things correctly, specifying 3 resources makes the DM9000 
driver ioremap() the memory, while specifying 2 resources implies that the 
platform code already ioremap()ed the memory. Is that right ?

If so, why does dm9000_probe() call request_mem_region() on ioremap()ed 
memory ?

Wouldn't it also be simpler to use release_mem_region() in 
dm9000_release_board() instead of release_resource() + kfree() ?

I'd be grateful if someone could confirm my assumptions. I'll then submit a 
patch to fix those issues.

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: DM9000 issue with mem resource management

[Ben Dooks]

On Thu, Jun 05, 2008 at 05:42:58PM +0200, Laurent Pinchart wrote:
> Hi everybody,
> 
> I ran into a resource-related bug in the DM9000 driver.
> 
> When the platform device has only 2 resources, dm9000_probe() doesn't set 
> db->irq_res, which results in a segfault when the pointer gets dereferenced 
> in dm9000_open().
> 
> I tried to fix the issue, and found out that the resource management code is 
> quite broken.

Personally, I'm thinking about just removing the case for 2, and making it
three resources only. The following in-kernel machines use the following
resources:

arch/arm/mach-at91/board-sam9261ek.c			3
arch/arm/mach-pxa/cm-x270.c				3
arch/arm/mach-pxa/em-x270.c				3
arch/arm/mach-pxa/trizeps4.c				3
arch/arm/mach-pxa/colibri.c				3
arch/arm/mach-s3c2410/mach-bast.c			3
arch/arm/mach-s3c2410/mach-vr1000.c			3
arch/blackfin/mach-bf527/boards/ezkit.c			2
arch/blackfin/mach-bf533/boards/H8606.c			2
arch/blackfin/mach-bf533/boards/ip0x.c			3
arch/blackfin/mach-bf537/boards/generic_board.c		2
arch/blackfin/mach-bf537/boards/stamp.c			2

As you can see, the #3 outweigh the #2. Unless anyone else objects, I
will add a patch to reduce this to the case where the driver expects
3 resources, and ask the users of #2 to submit changes for their
bots.
 
> If I understand things correctly, specifying 3 resources makes the DM9000 
> driver ioremap() the memory, while specifying 2 resources implies that the 
> platform code already ioremap()ed the memory. Is that right ?
> 
> If so, why does dm9000_probe() call request_mem_region() on ioremap()ed 
> memory ?

Hmm, dunno. I really have no idea why this happened. I don't think this
was my fault!
 
> Wouldn't it also be simpler to use release_mem_region() in 
> dm9000_release_board() instead of release_resource() + kfree() ?

If you already have the resource, this is a reasonably simple way of
ensuring you're disposing of the right resource.
 
> I'd be grateful if someone could confirm my assumptions. I'll then submit a 
> patch to fix those issues.

My recommendation is that we remove the 2 case completely, so I'd not bother
trying to fix this.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: DM9000 issue with mem resource management

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]

Hi Ben,

On Thursday 05 June 2008 19:40, Ben Dooks wrote:
> On Thu, Jun 05, 2008 at 05:42:58PM +0200, Laurent Pinchart wrote:
> > Hi everybody,
> > 
> > I ran into a resource-related bug in the DM9000 driver.
> > 
> > When the platform device has only 2 resources, dm9000_probe() doesn't set 
> > db->irq_res, which results in a segfault when the pointer gets
> > dereferenced in dm9000_open().
> > 
> > I tried to fix the issue, and found out that the resource management code
> > is quite broken.
> 
> Personally, I'm thinking about just removing the case for 2, and making it
> three resources only. The following in-kernel machines use the following
> resources:
> 
> arch/arm/mach-at91/board-sam9261ek.c			3
> arch/arm/mach-pxa/cm-x270.c				3
> arch/arm/mach-pxa/em-x270.c				3
> arch/arm/mach-pxa/trizeps4.c				3
> arch/arm/mach-pxa/colibri.c				3
> arch/arm/mach-s3c2410/mach-bast.c			3
> arch/arm/mach-s3c2410/mach-vr1000.c			3
> arch/blackfin/mach-bf527/boards/ezkit.c			2
> arch/blackfin/mach-bf533/boards/H8606.c			2
> arch/blackfin/mach-bf533/boards/ip0x.c			3
> arch/blackfin/mach-bf537/boards/generic_board.c		2
> arch/blackfin/mach-bf537/boards/stamp.c			2
> 
> As you can see, the #3 outweigh the #2. Unless anyone else objects, I
> will add a patch to reduce this to the case where the driver expects
> 3 resources, and ask the users of #2 to submit changes for their
> bots.
>  
> > If I understand things correctly, specifying 3 resources makes the DM9000 
> > driver ioremap() the memory, while specifying 2 resources implies that the 
> > platform code already ioremap()ed the memory. Is that right ?
> > 
> > If so, why does dm9000_probe() call request_mem_region() on ioremap()ed 
> > memory ?
> 
> Hmm, dunno. I really have no idea why this happened. I don't think this
> was my fault!
>  
> > Wouldn't it also be simpler to use release_mem_region() in 
> > dm9000_release_board() instead of release_resource() + kfree() ?
> 
> If you already have the resource, this is a reasonably simple way of
> ensuring you're disposing of the right resource.
>  
> > I'd be grateful if someone could confirm my assumptions. I'll then submit
> > a patch to fix those issues.
> 
> My recommendation is that we remove the 2 case completely, so I'd not bother
> trying to fix this.

I'm not too sure on that one.

I have DM9000 chips on a custom bus. The bus code remaps the whole address 
range (32 bytes per card x 32 slots) in one go and initializes the resource 
with a subset of the remapped range.

Converting that to per-slot ioremaps would create many more mappings. Is that 
an issue (performance-wise, or in term of a maximum number of mappings if 
there is a limit) ? I will still need a global mapping, as the bus code needs 
to access configuration registers on the individual cards, so I won't be able 
to request_mem_region both the global mapping and the individual mappings. 
I'm not sure if that's an issue.

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: DM9000 issue with mem resource management

[Ben Dooks, g]

On Fri, Jun 06, 2008 at 11:01:42AM +0200, Laurent Pinchart wrote:
> Hi Ben,
> 
> On Thursday 05 June 2008 19:40, Ben Dooks wrote:
> > On Thu, Jun 05, 2008 at 05:42:58PM +0200, Laurent Pinchart wrote:
> > > Hi everybody,
> > > 
> > > I ran into a resource-related bug in the DM9000 driver.
> > > 
> > > When the platform device has only 2 resources, dm9000_probe() doesn't set 
> > > db->irq_res, which results in a segfault when the pointer gets
> > > dereferenced in dm9000_open().
> > > 
> > > I tried to fix the issue, and found out that the resource management code
> > > is quite broken.
> > 
> > Personally, I'm thinking about just removing the case for 2, and making it
> > three resources only. The following in-kernel machines use the following
> > resources:
> > 
> > arch/arm/mach-at91/board-sam9261ek.c			3
> > arch/arm/mach-pxa/cm-x270.c				3
> > arch/arm/mach-pxa/em-x270.c				3
> > arch/arm/mach-pxa/trizeps4.c				3
> > arch/arm/mach-pxa/colibri.c				3
> > arch/arm/mach-s3c2410/mach-bast.c			3
> > arch/arm/mach-s3c2410/mach-vr1000.c			3
> > arch/blackfin/mach-bf527/boards/ezkit.c			2
> > arch/blackfin/mach-bf533/boards/H8606.c			2
> > arch/blackfin/mach-bf533/boards/ip0x.c			3
> > arch/blackfin/mach-bf537/boards/generic_board.c		2
> > arch/blackfin/mach-bf537/boards/stamp.c			2
> > 
> > As you can see, the #3 outweigh the #2. Unless anyone else objects, I
> > will add a patch to reduce this to the case where the driver expects
> > 3 resources, and ask the users of #2 to submit changes for their
> > bots.
> >  
> > > If I understand things correctly, specifying 3 resources makes the DM9000 
> > > driver ioremap() the memory, while specifying 2 resources implies that the 
> > > platform code already ioremap()ed the memory. Is that right ?
> > > 
> > > If so, why does dm9000_probe() call request_mem_region() on ioremap()ed 
> > > memory ?
> > 
> > Hmm, dunno. I really have no idea why this happened. I don't think this
> > was my fault!
> >  
> > > Wouldn't it also be simpler to use release_mem_region() in 
> > > dm9000_release_board() instead of release_resource() + kfree() ?
> > 
> > If you already have the resource, this is a reasonably simple way of
> > ensuring you're disposing of the right resource.
> >  
> > > I'd be grateful if someone could confirm my assumptions. I'll then submit
> > > a patch to fix those issues.
> > 
> > My recommendation is that we remove the 2 case completely, so I'd not bother
> > trying to fix this.
> 
> I'm not too sure on that one.

Well, having thought about it some more, the 2 address case is also an
abuse of the IORESOURCE mechanism with platform devices, as the core
driver code uses the .start and .end to register with the relevant
iomem reservation code. I'm going more strongly on removing the #2 case
unless anyone can come up with a really compelling reason to keep it. 
 
> I have DM9000 chips on a custom bus. The bus code remaps the whole address 
> range (32 bytes per card x 32 slots) in one go and initializes the resource 
> with a subset of the remapped range.
>
> Converting that to per-slot ioremaps would create many more mappings. Is that 
> an issue (performance-wise, or in term of a maximum number of mappings if 
> there is a limit) ? I will still need a global mapping, as the bus code needs 
> to access configuration registers on the individual cards, so I won't be able 
> to request_mem_region both the global mapping and the individual mappings. 
> I'm not sure if that's an issue.

That sounds like a problem with the bus code mapping too much of the memory
and thus causing a problem for any subsequent drivers. Having an extra
mapping in is only a minor problem compared with the other travesty the
2 region case is currently causing.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: DM9000 issue with mem resource management

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 4521 bytes --]

Hi Ben,

On Saturday 07 June 2008 23:35, Ben Dooks, g@trinity.fluff.org wrote:
> On Fri, Jun 06, 2008 at 11:01:42AM +0200, Laurent Pinchart wrote:
> > Hi Ben,
> > 
> > On Thursday 05 June 2008 19:40, Ben Dooks wrote:
> > > On Thu, Jun 05, 2008 at 05:42:58PM +0200, Laurent Pinchart wrote:
> > > > Hi everybody,
> > > > 
> > > > I ran into a resource-related bug in the DM9000 driver.
> > > > 
> > > > When the platform device has only 2 resources, dm9000_probe() doesn't
> > > > set db->irq_res, which results in a segfault when the pointer gets
> > > > dereferenced in dm9000_open().
> > > > 
> > > > I tried to fix the issue, and found out that the resource management
> > > > code is quite broken.
> > > 
> > > Personally, I'm thinking about just removing the case for 2, and making
> > > it three resources only. The following in-kernel machines use the
> > > following resources:
> > > 
> > > arch/arm/mach-at91/board-sam9261ek.c			3
> > > arch/arm/mach-pxa/cm-x270.c				3
> > > arch/arm/mach-pxa/em-x270.c				3
> > > arch/arm/mach-pxa/trizeps4.c				3
> > > arch/arm/mach-pxa/colibri.c				3
> > > arch/arm/mach-s3c2410/mach-bast.c			3
> > > arch/arm/mach-s3c2410/mach-vr1000.c			3
> > > arch/blackfin/mach-bf527/boards/ezkit.c			2
> > > arch/blackfin/mach-bf533/boards/H8606.c			2
> > > arch/blackfin/mach-bf533/boards/ip0x.c			3
> > > arch/blackfin/mach-bf537/boards/generic_board.c		2
> > > arch/blackfin/mach-bf537/boards/stamp.c			2
> > > 
> > > As you can see, the #3 outweigh the #2. Unless anyone else objects, I
> > > will add a patch to reduce this to the case where the driver expects
> > > 3 resources, and ask the users of #2 to submit changes for their
> > > bots.
> > >  
> > > > If I understand things correctly, specifying 3 resources makes the
> > > > DM9000 driver ioremap() the memory, while specifying 2 resources
> > > > implies that the platform code already ioremap()ed the memory. Is that
> > > > right ? 
> > > > 
> > > > If so, why does dm9000_probe() call request_mem_region() on
> > > > ioremap()ed memory ?
> > > 
> > > Hmm, dunno. I really have no idea why this happened. I don't think this
> > > was my fault!
> > >  
> > > > Wouldn't it also be simpler to use release_mem_region() in 
> > > > dm9000_release_board() instead of release_resource() + kfree() ?
> > > 
> > > If you already have the resource, this is a reasonably simple way of
> > > ensuring you're disposing of the right resource.
> > >  
> > > > I'd be grateful if someone could confirm my assumptions. I'll then
> > > > submit a patch to fix those issues.
> > > 
> > > My recommendation is that we remove the 2 case completely, so I'd not
> > > bother trying to fix this.
> > 
> > I'm not too sure on that one.
> 
> Well, having thought about it some more, the 2 address case is also an
> abuse of the IORESOURCE mechanism with platform devices, as the core
> driver code uses the .start and .end to register with the relevant
> iomem reservation code.

Agreed.

> I'm going more strongly on removing the #2 case unless anyone can come up
> with a really compelling reason to keep it.

Ok. I'll submit a patch.

BTW, why do the some boards declare the data resource as more than 4 bytes 
long ?

> > I have DM9000 chips on a custom bus. The bus code remaps the whole address 
> > range (32 bytes per card x 32 slots) in one go and initializes the
> > resource with a subset of the remapped range.
> >
> > Converting that to per-slot ioremaps would create many more mappings. Is
> > that an issue (performance-wise, or in term of a maximum number of
> > mappings if there is a limit) ? I will still need a global mapping, as the
> > bus code needs to access configuration registers on the individual cards,
> > so I won't be able to request_mem_region both the global mapping and the
> > individual mappings.
> > I'm not sure if that's an issue. 
> 
> That sounds like a problem with the bus code mapping too much of the memory
> and thus causing a problem for any subsequent drivers.

That's right. It had been done that way to save mappings. Without a global 
mapping I'll need 64 mappings instead of 1.

> Having an extra mapping in is only a minor problem compared with the other
> travesty the 2 region case is currently causing.

What about 63 extra mappings ?

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: DM9000 issue with mem resource management

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 1945 bytes --]

Hi Ben,

On Thursday 05 June 2008 19:40, Ben Dooks wrote:
> On Thu, Jun 05, 2008 at 05:42:58PM +0200, Laurent Pinchart wrote:
> > Hi everybody,
> > 
> > I ran into a resource-related bug in the DM9000 driver.
> > 
> > When the platform device has only 2 resources, dm9000_probe() doesn't set 
> > db->irq_res, which results in a segfault when the pointer gets
> > dereferenced in dm9000_open().
> > 
> > I tried to fix the issue, and found out that the resource management code
> > is quite broken.
> 
> Personally, I'm thinking about just removing the case for 2, and making it
> three resources only. The following in-kernel machines use the following
> resources:
> 
> arch/arm/mach-at91/board-sam9261ek.c			3
> arch/arm/mach-pxa/cm-x270.c				3
> arch/arm/mach-pxa/em-x270.c				3
> arch/arm/mach-pxa/trizeps4.c				3
> arch/arm/mach-pxa/colibri.c				3
> arch/arm/mach-s3c2410/mach-bast.c			3
> arch/arm/mach-s3c2410/mach-vr1000.c			3
> arch/blackfin/mach-bf527/boards/ezkit.c			2
> arch/blackfin/mach-bf533/boards/H8606.c			2
> arch/blackfin/mach-bf533/boards/ip0x.c			3
> arch/blackfin/mach-bf537/boards/generic_board.c		2
> arch/blackfin/mach-bf537/boards/stamp.c			2
> 
> As you can see, the #3 outweigh the #2. Unless anyone else objects, I
> will add a patch to reduce this to the case where the driver expects
> 3 resources, and ask the users of #2 to submit changes for their
> bots.

I've just had a look at the #2 boards and there's something I don't get. My 
understanding is that the 2 resources case is used to pass ioremap()ed memory 
to the driver, while the 3 resources case is used to pass physical addresses. 
This is what the driver currently does. Does it work by sheer luck due to a 
global 1-to-1 mapping on arm ?

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]