devres and requesting resources

devres and requesting resources

[Jeff Garzik]

Something I just noticed, which I missed during the initial devres review...

The vast majority of my ATA drivers originally called 
pci_request_regions(), to reserve all regions attached to a device.

In converting to pcim_iomap_regions(), we no longer reserve /all/ 
regions, only the ones requested.

This is actually a bug:  it was intentional to call 
pci_request_regions(), because that ensures that no other software will 
use our resources -- even if we are not actively using the resource in 
question.

Or IOW, I wanted to ensure that there would be no device sharing... 
which this devres conversion accidentally enabled.

The simple fix is obviously to replace pci_request_region() call with 
pci_request_regions() in lib/devres.c, but I wonder if that will break 
any existing driver?

	Jeff

Re: devres and requesting resources

[Alan Cox]

> In converting to pcim_iomap_regions(), we no longer reserve /all/ 
> regions, only the ones requested.
> 
> This is actually a bug:  it was intentional to call 
> pci_request_regions(), because that ensures that no other software will 
> use our resources -- even if we are not actively using the resource in 
> question.

That would be a bug. We should only reserve the regions we are using. It
is possible that the other regions can be used correctly by another driver
(consider the CS5520 or the MPIIX where the IDE is only part of one
function of the PCI device).

I believe the current code is in fact correct.

> Or IOW, I wanted to ensure that there would be no device sharing... 
> which this devres conversion accidentally enabled.

Why ?

> The simple fix is obviously to replace pci_request_region() call with 
> pci_request_regions() in lib/devres.c, but I wonder if that will break 
> any existing driver?

It would make devres mostly useless especially outside of libata.

Alan

Re: devres and requesting resources

[Jeff Garzik]

Alan Cox wrote:
>> In converting to pcim_iomap_regions(), we no longer reserve /all/ 
>> regions, only the ones requested.
>>
>> This is actually a bug:  it was intentional to call 
>> pci_request_regions(), because that ensures that no other software will 
>> use our resources -- even if we are not actively using the resource in 
>> question.
> 
> That would be a bug. We should only reserve the regions we are using. It
> is possible that the other regions can be used correctly by another driver
> (consider the CS5520 or the MPIIX where the IDE is only part of one
> function of the PCI device).

Only rare PCI devices are shareable among multiple drivers.

sata_* at least intentionally used pci_request_regions() because it is 
obvious from the hardware spec that multiple regions accessed by 
multiple drivers is highly unlikely, without the driver being 
specifically coded to support such sharing.  Such sharing code is far 
beyond simple resource reservation, to avoid stepping on toes when there 
is a single MMIO region and set of interrupt clearing registers.

So reading your email it sounds like there are valid cases for both 
configurations.

Its a design choice either way, not a bug either way.

	Jeff

Re: devres and requesting resources

[Tejun Heo]

Jeff Garzik wrote:
> Only rare PCI devices are shareable among multiple drivers.
> 
> sata_* at least intentionally used pci_request_regions() because it is
> obvious from the hardware spec that multiple regions accessed by
> multiple drivers is highly unlikely, without the driver being
> specifically coded to support such sharing.  Such sharing code is far
> beyond simple resource reservation, to avoid stepping on toes when there
> is a single MMIO region and set of interrupt clearing registers.
> 
> So reading your email it sounds like there are valid cases for both
> configurations.
> 
> Its a design choice either way, not a bug either way.

I was thinking like Alan when I was writing pcim_request_regions() but I
agree reserving all the regions does avoid certain problems.  Especially
true for controller which duals as native SATA and compatible SFF
controller like ICH AHCIs.  ata_generic or ide generic might attach to a
controller which is already being driven by ahci under certain
configurations.

Please note that pcim_request_regions() is a helper to do
pci_request_region() calls and pcim_iomap() in one go.  Drivers which
have different requirements can just open code pci_request_regions() and
pcim_iomap().  pcim_request_regions() should provide sensible default
behavior for common cases.

I think the best solution is to allow duplicate request regions for
managed devices which is okay as we know we're holding the resource and
let drivers which need to reserve all regions call pci_request_regions()
before calling pcim_request_regions().

How does it sound?

Thanks.

-- 
tejun

Re: devres and requesting resources

[Alan Cox]

> controller like ICH AHCIs.  ata_generic or ide generic might attach to a
> controller which is already being driven by ahci under certain
> configurations.

ata_generic will not nor ata_legacy both are much too smart for that. The
legacy old IDE driver might but that probably isn't going to be fixed by
devres and is trivial to fix in that driver (just steal the code from
pata_legacy).

> have different requirements can just open code pci_request_regions() and
> pcim_iomap().  pcim_request_regions() should provide sensible default
> behavior for common cases.

Which is arguably the current behaviour. Changing the behaviour and not
the name is a really bad idea and will cause problems in future so don't
do that.
>
> I think the best solution is to allow duplicate request regions for
> managed devices which is okay as we know we're holding the resource and
> let drivers which need to reserve all regions call pci_request_regions()
> before calling pcim_request_regions().

How about

	pcim_request_all_regions()

for the behaviour Jeff wants, simple, direct, differently named and
obvious what it does.

Alan

Re: devres and requesting resources

[Tejun Heo]

Alan Cox wrote:
>> controller like ICH AHCIs.  ata_generic or ide generic might attach to a
>> controller which is already being driven by ahci under certain
>> configurations.
> 
> ata_generic will not nor ata_legacy both are much too smart for that. The
> legacy old IDE driver might but that probably isn't going to be fixed by
> devres and is trivial to fix in that driver (just steal the code from
> pata_legacy).

Yeah, I saw the behavior with ahci + ide generic combination.  I think
it was on SB600.  Heh... neat trick in pata_legacy().  Hmm... it's
probably better to do at generic device level and with a generic helper.

>> have different requirements can just open code pci_request_regions() and
>> pcim_iomap().  pcim_request_regions() should provide sensible default
>> behavior for common cases.
> 
> Which is arguably the current behaviour. Changing the behaviour and not
> the name is a really bad idea and will cause problems in future so don't
> do that.
>> I think the best solution is to allow duplicate request regions for
>> managed devices which is okay as we know we're holding the resource and
>> let drivers which need to reserve all regions call pci_request_regions()
>> before calling pcim_request_regions().
> 
> How about
> 
> 	pcim_request_all_regions()
> 
> for the behaviour Jeff wants, simple, direct, differently named and
> obvious what it does.

Sounds good to me.  Jeff?

-- 
tejun

Re: devres and requesting resources

[Jeff Garzik]

Tejun Heo wrote:
>> How about
>>
>> 	pcim_request_all_regions()
>>
>> for the behaviour Jeff wants, simple, direct, differently named and
>> obvious what it does.
> 
> Sounds good to me.  Jeff?

Fine with me...  Naming doesn't really matter to much as simply having 
the capability, in one form or another.

	Jeff

Re: devres and requesting resources

[Bartlomiej Zolnierkiewicz]

On Friday 29 February 2008, Alan Cox wrote:
> > controller like ICH AHCIs.  ata_generic or ide generic might attach to a
> > controller which is already being driven by ahci under certain
> > configurations.
> 
> ata_generic will not nor ata_legacy both are much too smart for that. The
> legacy old IDE driver might but that probably isn't going to be fixed by
> devres and is trivial to fix in that driver (just steal the code from
> pata_legacy).

Yep, nice trick.

Since it is your idea/code could you please also fix ide_generic? :)

> > have different requirements can just open code pci_request_regions() and
> > pcim_iomap().  pcim_request_regions() should provide sensible default
> > behavior for common cases.
> 
> Which is arguably the current behaviour. Changing the behaviour and not
> the name is a really bad idea and will cause problems in future so don't
> do that.
> >
> > I think the best solution is to allow duplicate request regions for
> > managed devices which is okay as we know we're holding the resource and
> > let drivers which need to reserve all regions call pci_request_regions()
> > before calling pcim_request_regions().

This will work until code outside libata (i.e. IDE) starts using devres...

[ Incidentally, just yesterday I finished moving resource management handling
  to host drivers so it might as well happen. ]

> How about
> 
> 	pcim_request_all_regions()
> 
> for the behaviour Jeff wants, simple, direct, differently named and
> obvious what it does.

Seconded.

Thanks,
Bart

Re: devres and requesting resources

[Alan Cox]

> > legacy old IDE driver might but that probably isn't going to be fixed by
> > devres and is trivial to fix in that driver (just steal the code from
> > pata_legacy).
> 
> Yep, nice trick.
> 
> Since it is your idea/code could you please also fix ide_generic? :)

In my copious free time - ask me around August ;)

Alan

Re: devres and requesting resources

[Alan Cox]

> Only rare PCI devices are shareable among multiple drivers.

Really. Let me see
	AGP & EDAC
	Serial/Parallel combo ports
	MPIIX
	CS5520
	Lots of I2C bus stuff
	VGA v 3D

There are quite a few, and some are already quite fun enough with our pci
struct model.

> sata_* at least intentionally used pci_request_regions() because it is 
> obvious from the hardware spec that multiple regions accessed by 
> multiple drivers is highly unlikely, without the driver being 
> specifically coded to support such sharing.  Such sharing code is far 
> beyond simple resource reservation, to avoid stepping on toes when there 
> is a single MMIO region and set of interrupt clearing registers.
> 
> So reading your email it sounds like there are valid cases for both 
> configurations.
> 
> Its a design choice either way, not a bug either way.

It is a flaw: devres that assumes it should grab all resources is unusable
for some other drivers - it is no longer generic and that makes it far
less useful. I've got no problem with the idea of a devres way to say
"and I want it all, mine mine mine" but that should not be the only
behaviour.

Alan