Re: + fix-ide-dma-resource-managment.patch added to -mm tree

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>The patch titled

>>     Fix IDE DMA resource managment

>>has been added to the -mm tree.  Its filename is

>>     fix-ide-dma-resource-managment.patch

>>See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
>>out what to do about this

>>From: Sergei Shtylylov <sshtylyov@ru.mvista.com>

>>Resolve several IDE DMA resource management issues:

>>- release DMA engine for memory mapped DMA as well

> NAK - this won't work, see siimage.c how these resources were
> really reserved in the first place [ request_mem_region() ]

    Oops, looks like I missed that missing ide_setup_dma() call in that driver...
    Is there any point at all in handling mmio == 2 case in ide-dma.c then? 
The only function that should handle this seems ide_release_dma() but in case 
of MMIO there's _no_ provision for the proper driver cleanup (assuming it 
allocates everything it needs by itself)... and, if the driver allocates the 
engine of the different size that PDC_BYTES * PRD_ENTRIES (the case with 
sgiioc3.c), the engine release doesn't look consitent...

> hwif->mmio == 2 means that host driver is responsible
> for reserving/releasing resources (this is _preferred_ way
> of handling resources)

    Hm, didn't know that...

> please fix siimage.c and core code instead
> (please see libata-core.c and ata_host_stop() etc.)

>>- release the same number of DMA I/O ports that was requested by a driver

> NAK, it should be always 8 - the only driver which does something different
> is trm290.c and now looking at it, it seems broken since early 2.5.x

    Why then the argument to ide_setup_dma() is needed at all?

> please fix trm290.c to use ->mmio == 2 instead

    It's a _really_ old chipset (pre SFF-8038i) and has _no_ memory mapped 
regs AFAIK...

>>- release the channel's secondary DMA I/O ports for real

> NAK, looking and the code - secondary DMA base is only used
> by siimage.c and sgiioc4.c (both use ->mmio == 2, for siimage ->dma_base2
> is unused, for sgiioc4 it is used for some weird DMA transfer ending stuff)

    Yes, I have sgioc4.c patch that deals with this (almost) ready...

> so all of ->dma_base2 handling code in ide-dma.c is just a dead code
> and should be removed

    OK...

>>- claim extra DMA I/O ports regardless of what IDE channels are present/enabled
> 
> NAK, this change makes extra DMA I/O ports being reserved twice

    No. Please review the patch carefully. This was tested OK on both channels 
and secondary only channel.

>>- merge the duplicate code into ide_dma_iobase()

> NAK, some of this code is irrelevant for MMIO (ie. ->dma_extra)

    But it was duplicated anyway...

> Please rework your changes and split them on separate patches
> (IDE code is quite difficult - making changes in small steps really
> helps).

    Er... I'll try to.

> Thanks,
> Bartlomiej

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Sergei Shtylylov wrote:

> Bartlomiej Zolnierkiewicz wrote:

[skipped]

>>> Resolve several IDE DMA resource management issues:

>>> - release DMA engine for memory mapped DMA as well

>> NAK - this won't work, see siimage.c how these resources were
>> really reserved in the first place [ request_mem_region() ]

    NB: it's reserved this way only when available -- this driver can work 
with both I/O and memory mapped regs...

>    Oops, looks like I missed that missing ide_setup_dma() call in that 
> driver...

    No, I've just forgotten that it's called implicitly from setup-pci.c since 
init_dma() "method" in not defined in that driver. So, looks like you're wrong 
here -- it should work currently BUT the DMA engine is not released since 
(mmio == 2) check and return precedes call to ide_release_dma_engine() in 
ide_release_dma().
    And as I said, there's no provision to release any driver-specific 
resources on a call to ide_release_dma()...

[skipped]

>> hwif->mmio == 2 means that host driver is responsible
>> for reserving/releasing resources (this is _preferred_ way
>> of handling resources)

>    Hm, didn't know that...

    ... and this was hard to guess from the current code. ;-)

>> please fix trm290.c to use ->mmio == 2 instead

>    It's a _really_ old chipset (pre SFF-8038i) and has _no_ memory 
> mapped regs AFAIK...

    The only thing that can be done is not to call ide_setup_dma() from it...

>>> - release the channel's secondary DMA I/O ports for real

>> NAK, looking and the code - secondary DMA base is only used
>> by siimage.c and sgiioc4.c (both use ->mmio == 2, for siimage ->dma_base2
>> is unused, for sgiioc4 it is used for some weird DMA transfer ending 
>> stuff)

>    Yes, I have sgioc4.c patch that deals with this (almost) ready...

    ... and for siimage, this seems a no-brainer. :-)

[skipped]

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Bartlomiej Zolnierkiewicz]

On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Sergei Shtylylov wrote:
>
> > Bartlomiej Zolnierkiewicz wrote:
>
> [skipped]
>
> >>> Resolve several IDE DMA resource management issues:
>
> >>> - release DMA engine for memory mapped DMA as well
>
> >> NAK - this won't work, see siimage.c how these resources were
> >> really reserved in the first place [ request_mem_region() ]
>
>     NB: it's reserved this way only when available -- this driver can work
> with both I/O and memory mapped regs...
>
> >    Oops, looks like I missed that missing ide_setup_dma() call in that
> > driver...
>
>     No, I've just forgotten that it's called implicitly from setup-pci.c since
> init_dma() "method" in not defined in that driver. So, looks like you're wrong
> here -- it should work currently BUT the DMA engine is not released since
> (mmio == 2) check and return precedes call to ide_release_dma_engine() in
> ide_release_dma().

please take a look at siimage.c:init_iops_siimage():

	if (pci_get_drvdata(dev) == NULL)
		return;
	init_mmio_iops_siimage(hwif);

and siimage.c:init_mmio_iops_siimage():

	hwif->mmio			= 2;

so ->mmio == 2 IFF MMIO is used

* for ->mmio == 0 current code seem to release resources properly
* for ->mmio == 2 you need to release the region that was reserved
  originally (for siimage it is one big fat mem region) and not standard
  DMA I/O regions, therefore this change is wrong

>     And as I said, there's no provision to release any driver-specific
> resources on a call to ide_release_dma()...

That's why asked you to take look at libata-core.c:ata_host_stop()
etc. and to add any needed core code... :-)

> [skipped]
>
> >> hwif->mmio == 2 means that host driver is responsible
> >> for reserving/releasing resources (this is _preferred_ way
> >> of handling resources)
>
> >    Hm, didn't know that...
>
>     ... and this was hard to guess from the current code. ;-)
>
> >> please fix trm290.c to use ->mmio == 2 instead
>
> >    It's a _really_ old chipset (pre SFF-8038i) and has _no_ memory
> > mapped regs AFAIK...
>
>     The only thing that can be done is not to call ide_setup_dma() from it...

Exactly and make trm290.c to handle resources itself...

> >>> - release the channel's secondary DMA I/O ports for real
>
> >> NAK, looking and the code - secondary DMA base is only used
> >> by siimage.c and sgiioc4.c (both use ->mmio == 2, for siimage ->dma_base2
> >> is unused, for sgiioc4 it is used for some weird DMA transfer ending
> >> stuff)
>
> >    Yes, I have sgioc4.c patch that deals with this (almost) ready...
>
>     ... and for siimage, this seems a no-brainer. :-)

Yes, you may want to fix it... :-)

Thanks,
Bartlomiej

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>>>Resolve several IDE DMA resource management issues:

>>>>>- release DMA engine for memory mapped DMA as well

>>>>NAK - this won't work, see siimage.c how these resources were
>>>>really reserved in the first place [ request_mem_region() ]

>>    NB: it's reserved this way only when available -- this driver can work
>>with both I/O and memory mapped regs...

>>>   Oops, looks like I missed that missing ide_setup_dma() call in that
>>>driver...

>>    No, I've just forgotten that it's called implicitly from setup-pci.c since
>>init_dma() "method" in not defined in that driver. So, looks like you're wrong
>>here -- it should work currently BUT the DMA engine is not released since
>>(mmio == 2) check and return precedes call to ide_release_dma_engine() in
>>ide_release_dma().

> please take a look at siimage.c:init_iops_siimage():
> 
> 	if (pci_get_drvdata(dev) == NULL)
> 		return;

    So what? It just leaves the default I/O mapped iops intact and the driver 
init. continues...

> 	init_mmio_iops_siimage(hwif);

> and siimage.c:init_mmio_iops_siimage():

> 	hwif->mmio			= 2;

> so ->mmio == 2 IFF MMIO is used

    Yes, but ide_setup_dma() is called in _both_ cases with the current IDE 
core. And therefore, ide_release_dma() should've undone what ide_setup_dma() 
did... As I understand now, both the current behavior and what I'm proposing 
are wrong.

> * for ->mmio == 0 current code seem to release resources properly
> * for ->mmio == 2 you need to release the region that was reserved
>   originally (for siimage it is one big fat mem region) and not standard
>   DMA I/O regions, therefore this change is wrong

    Yeah, I saw that prefectly clear before posting the 1st patch but really 
didn't have time for such a drastic change as adding the cleanup hook to a lot 
of drivers (and not much hope to get it accepted :-)

[skipped]

> Thanks,
> Bartlomiej

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Bartlomiej Zolnierkiewicz]

On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
> >>>>>Resolve several IDE DMA resource management issues:
>
> >>>>>- release DMA engine for memory mapped DMA as well
>
> >>>>NAK - this won't work, see siimage.c how these resources were
> >>>>really reserved in the first place [ request_mem_region() ]
>
> >>    NB: it's reserved this way only when available -- this driver can work
> >>with both I/O and memory mapped regs...
>
> >>>   Oops, looks like I missed that missing ide_setup_dma() call in that
> >>>driver...
>
> >>    No, I've just forgotten that it's called implicitly from setup-pci.c since
> >>init_dma() "method" in not defined in that driver. So, looks like you're wrong
> >>here -- it should work currently BUT the DMA engine is not released since
> >>(mmio == 2) check and return precedes call to ide_release_dma_engine() in
> >>ide_release_dma().
>
> > please take a look at siimage.c:init_iops_siimage():
> >
> >       if (pci_get_drvdata(dev) == NULL)
> >               return;
>
>     So what? It just leaves the default I/O mapped iops intact and the driver
> init. continues...

->mmio stays 0 and the driver continues in the standard I/O mode...

> >       init_mmio_iops_siimage(hwif);
>
> > and siimage.c:init_mmio_iops_siimage():
>
> >       hwif->mmio                      = 2;
>
> > so ->mmio == 2 IFF MMIO is used
>
>     Yes, but ide_setup_dma() is called in _both_ cases with the current IDE
> core. And therefore, ide_release_dma() should've undone what ide_setup_dma()

and it does it for ->mmio == 0 case

> did... As I understand now, both the current behavior and what I'm proposing
> are wrong.

yes (->mmio == 2 case)

> > * for ->mmio == 0 current code seem to release resources properly
> > * for ->mmio == 2 you need to release the region that was reserved
> >   originally (for siimage it is one big fat mem region) and not standard
> >   DMA I/O regions, therefore this change is wrong
>
>     Yeah, I saw that prefectly clear before posting the 1st patch but really
> didn't have time for such a drastic change as adding the cleanup hook to a lot
> of drivers (and not much hope to get it accepted :-)

You don't have to add it to every host driver, just to drivers that need it...
It will be accepted if it is correct, currently proposed change isn't...

Bartlomiej

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

> On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:

>>>>>>>Resolve several IDE DMA resource management issues:

>>>>>>>- release DMA engine for memory mapped DMA as well

>>>>>>NAK - this won't work, see siimage.c how these resources were
>>>>>>really reserved in the first place [ request_mem_region() ]

>>>>   NB: it's reserved this way only when available -- this driver can work
>>>>with both I/O and memory mapped regs...

>>>>>  Oops, looks like I missed that missing ide_setup_dma() call in that
>>>>>driver...

>>>>   No, I've just forgotten that it's called implicitly from setup-pci.c since
>>>>init_dma() "method" in not defined in that driver. So, looks like you're wrong
>>>>here -- it should work currently BUT the DMA engine is not released since
>>>>(mmio == 2) check and return precedes call to ide_release_dma_engine() in
>>>>ide_release_dma().

>>>please take a look at siimage.c:init_iops_siimage():

>>>      if (pci_get_drvdata(dev) == NULL)
>>>              return;

>>    So what? It just leaves the default I/O mapped iops intact and the driver
>>init. continues...

> ->mmio stays 0 and the driver continues in the standard I/O mode...

    I don't really see the difference in regard to our case. Am I missing 
something?

>>>      init_mmio_iops_siimage(hwif);

>>>and siimage.c:init_mmio_iops_siimage():

>>>      hwif->mmio                      = 2;

>>>so ->mmio == 2 IFF MMIO is used

>>    Yes, but ide_setup_dma() is called in _both_ cases with the current IDE
>>core. And therefore, ide_release_dma() should've undone what ide_setup_dma()

> and it does it for ->mmio == 0 case

    But not for mmio == 2. Why?

>>did... As I understand now, both the current behavior and what I'm proposing
>>are wrong.

    Not really, at least in this part. :-)

> yes (->mmio == 2 case)

    We've just concluded that allocating and freeing the DMA engine for that 
case is still perfectly valid, so where was I wrong?

[skipped]

> Bartlomiej

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Sergei Shtylylov wrote:

>> Bartlomiej Zolnierkiewicz wrote:

[skipped]

>>>> - release the channel's secondary DMA I/O ports for real

>>> NAK, looking and the code - secondary DMA base is only used
>>> by siimage.c and sgiioc4.c (both use ->mmio == 2, for siimage 
>>> ->dma_base2
>>> is unused, for sgiioc4 it is used for some weird DMA transfer ending 
>>> stuff)

    Hm, concerning the other field of ide_hwif_t, dma_master: I currently see 
no users to it but pdc202xx_old.c. The way it's computed makes it dependent on 
what channels are enabled (though all channels are forced present in that 
driver by the most recent patch) which makes it invalid if the primary channel 
happens to be disabled. I'd like to replace it with extra_base fields which 
would point to the extra DMA ports regardless of the channels present/enabled, 
is this OK?

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Bartlomiej Zolnierkiewicz]

On 3/28/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Sergei Shtylylov wrote:
>
> >> Bartlomiej Zolnierkiewicz wrote:
>
> [skipped]
>
> >>>> - release the channel's secondary DMA I/O ports for real
>
> >>> NAK, looking and the code - secondary DMA base is only used
> >>> by siimage.c and sgiioc4.c (both use ->mmio == 2, for siimage
> >>> ->dma_base2
> >>> is unused, for sgiioc4 it is used for some weird DMA transfer ending
> >>> stuff)
>
>     Hm, concerning the other field of ide_hwif_t, dma_master: I currently see
> no users to it but pdc202xx_old.c. The way it's computed makes it dependent on
> what channels are enabled (though all channels are forced present in that
> driver by the most recent patch) which makes it invalid if the primary channel
> happens to be disabled. I'd like to replace it with extra_base fields which
> would point to the extra DMA ports regardless of the channels present/enabled,
> is this OK?

Isn't it better to just use pci_resource_start(dev, 4) in pdc202xx_old.c?

Bartlomiej

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>>Bartlomiej Zolnierkiewicz wrote:

>>[skipped]

>>>>>>- release the channel's secondary DMA I/O ports for real

>>>>>NAK, looking and the code - secondary DMA base is only used
>>>>>by siimage.c and sgiioc4.c (both use ->mmio == 2, for siimage
>>>>>->dma_base2
>>>>>is unused, for sgiioc4 it is used for some weird DMA transfer ending
>>>>>stuff)

>>    Hm, concerning the other field of ide_hwif_t, dma_master: I currently see
>>no users to it but pdc202xx_old.c. The way it's computed makes it dependent on
>>what channels are enabled (though all channels are forced present in that
>>driver by the most recent patch) which makes it invalid if the primary channel
>>happens to be disabled. I'd like to replace it with extra_base fields which
>>would point to the extra DMA ports regardless of the channels present/enabled,
>>is this OK?

> Isn't it better to just use pci_resource_start(dev, 4) in pdc202xx_old.c?

    Not only better but is actually correct -- extra_base would belong only to 
a sole channel in a pair... :-< I'm still not sure this field is worth 
introducing... I probably won't.

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Sergei Shtylylov wrote:

> Bartlomiej Zolnierkiewicz wrote:

>>> [skipped]

>>>>>>> - release the channel's secondary DMA I/O ports for real

>>>>>> NAK, looking and the code - secondary DMA base is only used
>>>>>> by siimage.c and sgiioc4.c (both use ->mmio == 2, for siimage
>>>>>> ->dma_base2
>>>>>> is unused, for sgiioc4 it is used for some weird DMA transfer ending
>>>>>> stuff)

>>>    Hm, concerning the other field of ide_hwif_t, dma_master: I 
>>> currently see
>>> no users to it but pdc202xx_old.c. The way it's computed makes it 
>>> dependent on
>>> what channels are enabled (though all channels are forced present in 
>>> that
>>> driver by the most recent patch) which makes it invalid if the 
>>> primary channel
>>> happens to be disabled. I'd like to replace it with extra_base fields 
>>> which
>>> would point to the extra DMA ports regardless of the channels 
>>> present/enabled,
>>> is this OK?

>> Isn't it better to just use pci_resource_start(dev, 4) in pdc202xx_old.c?

>    Not only better but is actually correct -- extra_base would belong 
> only to a sole channel in a pair... :-< I'm still not sure this field is 
> worth introducing... I probably won't.

    OTOH, it's handy in ide-dma.c itself. And hwif->extra_base is surely more 
efficient than pci_resource_start(hwif->dev, 4). So, I'm going to introduce 
it, just changing the initial code somewhat...

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Alan Cox]

On Maw, 2006-03-28 at 20:21 +0400, Sergei Shtylylov wrote:
>     OTOH, it's handy in ide-dma.c itself. And hwif->extra_base is surely more 
> efficient than pci_resource_start(hwif->dev, 4).

Its just more memory use so I doubt it. pci_resource_foo is just
derefencing internal structures not going to the bus.

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Alan Cox wrote:

>>    OTOH, it's handy in ide-dma.c itself. And hwif->extra_base is surely more 
>>efficient than pci_resource_start(hwif->dev, 4).
> 
> 
> Its just more memory use so I doubt it.

    dma_master and dma_base2 fields are taking up the memory anyway. I'm going 
to remove them both.

> pci_resource_foo is just
> derefencing internal structures not going to the bus.

    Note the double dereference vs single one.

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Bartlomiej Zolnierkiewicz]

Hi,

On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
> >>The patch titled
>
> >>     Fix IDE DMA resource managment
>
> >>has been added to the -mm tree.  Its filename is
>
> >>     fix-ide-dma-resource-managment.patch
>
> >>See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> >>out what to do about this
>
> >>From: Sergei Shtylylov <sshtylyov@ru.mvista.com>
>
> >>Resolve several IDE DMA resource management issues:
>
> >>- release DMA engine for memory mapped DMA as well
>
> > NAK - this won't work, see siimage.c how these resources were
> > really reserved in the first place [ request_mem_region() ]
>
>     Oops, looks like I missed that missing ide_setup_dma() call in that driver...
>     Is there any point at all in handling mmio == 2 case in ide-dma.c then?
> The only function that should handle this seems ide_release_dma() but in case
> of MMIO there's _no_ provision for the proper driver cleanup (assuming it
> allocates everything it needs by itself)... and, if the driver allocates the
> engine of the different size that PDC_BYTES * PRD_ENTRIES (the case with
> sgiioc3.c), the engine release doesn't look consitent...
>
> > hwif->mmio == 2 means that host driver is responsible
> > for reserving/releasing resources (this is _preferred_ way
> > of handling resources)
>
>     Hm, didn't know that...

not the best documentation but this should give some hints:

from <linux/ide.h>:
	int		mmio;		/* hosts iomio (0) or custom (2) select */

> > please fix siimage.c and core code instead
> > (please see libata-core.c and ata_host_stop() etc.)
>
> >>- release the same number of DMA I/O ports that was requested by a driver
>
> > NAK, it should be always 8 - the only driver which does something different
> > is trm290.c and now looking at it, it seems broken since early 2.5.x
>
>     Why then the argument to ide_setup_dma() is needed at all?

It is not needed and can be removed... :-)

> > please fix trm290.c to use ->mmio == 2 instead
>
>     It's a _really_ old chipset (pre SFF-8038i) and has _no_ memory mapped
> regs AFAIK...

->mmio == 2 means _only_ that host driver is responsible
for reserving/releasing resources - it doesn't mean that host driver
is using MMIO (a bit confusing but this is what the current code does)

> >>- release the channel's secondary DMA I/O ports for real
>
> > NAK, looking and the code - secondary DMA base is only used
> > by siimage.c and sgiioc4.c (both use ->mmio == 2, for siimage ->dma_base2
> > is unused, for sgiioc4 it is used for some weird DMA transfer ending stuff)
>
>     Yes, I have sgioc4.c patch that deals with this (almost) ready...

:-)

> > so all of ->dma_base2 handling code in ide-dma.c is just a dead code
> > and should be removed
>
>     OK...
>
> >>- claim extra DMA I/O ports regardless of what IDE channels are present/enabled
> >
> > NAK, this change makes extra DMA I/O ports being reserved twice
>
>     No. Please review the patch carefully. This was tested OK on both channels
> and secondary only channel.

OK, you are right here.

> >>- merge the duplicate code into ide_dma_iobase()
>
> > NAK, some of this code is irrelevant for MMIO (ie. ->dma_extra)
>
>     But it was duplicated anyway...

Yes but it should be removed for MMIO case completely.

Thanks,
Bartlomiej

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Bartlomiej Zolnierkiewicz wrote:
> On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:

[skipped]

>>>please fix siimage.c and core code instead
>>>(please see libata-core.c and ata_host_stop() etc.)

>>>>- release the same number of DMA I/O ports that was requested by a driver

>>>please fix trm290.c to use ->mmio == 2 instead

>>    It's a _really_ old chipset (pre SFF-8038i) and has _no_ memory mapped
>>regs AFAIK...

> ->mmio == 2 means _only_ that host driver is responsible
> for reserving/releasing resources - it doesn't mean that host driver
> is using MMIO (a bit confusing but this is what the current code does)

    So, your point is that ide_setup_dma() should _never_ do anything for the 
(mmio == 2) case? Maybe it shouldn't even be called? I guess we surely need 
some driver cleanup hook to call ftom ide_release_dma()...

[skipped]

> Thanks,
> Bartlomiej

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Bartlomiej Zolnierkiewicz]

On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
> > On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:
>
> [skipped]
>
> >>>please fix siimage.c and core code instead
> >>>(please see libata-core.c and ata_host_stop() etc.)
>
> >>>>- release the same number of DMA I/O ports that was requested by a driver
>
> >>>please fix trm290.c to use ->mmio == 2 instead
>
> >>    It's a _really_ old chipset (pre SFF-8038i) and has _no_ memory mapped
> >>regs AFAIK...
>
> > ->mmio == 2 means _only_ that host driver is responsible
> > for reserving/releasing resources - it doesn't mean that host driver
> > is using MMIO (a bit confusing but this is what the current code does)
>
>     So, your point is that ide_setup_dma() should _never_ do anything for the
> (mmio == 2) case? Maybe it shouldn't even be called? I guess we surely need

No, my point is that for ->mmio == 2 core code may not try to manage
IO or MMIO resources, only that.  ide_setup_dma() call is still needed for
calling ide_allocate_dma_engine() and setting up ->dma_* / ->ide_dma_*
fields.

> some driver cleanup hook to call ftom ide_release_dma()...

Yep.

Bartlomiej

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Sergei Shtylylov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:

[skipped]

>>>>>>- release the same number of DMA I/O ports that was requested by a driver

>>>>>please fix trm290.c to use ->mmio == 2 instead

>>>>   It's a _really_ old chipset (pre SFF-8038i) and has _no_ memory mapped
>>>>regs AFAIK...

>>>->mmio == 2 means _only_ that host driver is responsible
>>>for reserving/releasing resources - it doesn't mean that host driver
>>>is using MMIO (a bit confusing but this is what the current code does)

>>    So, your point is that ide_setup_dma() should _never_ do anything for the
>>(mmio == 2) case? Maybe it shouldn't even be called? I guess we surely need

> No, my point is that for ->mmio == 2 core code may not try to manage
> IO or MMIO resources, only that.

    Note that I didn't try to. :-)
    Your 1st NAK was because I moved ide_release_dma_engine() above that
(mmio == 2) check in ide_release_dma() -- which as we've now concluded is 
perfectly correct, isn't it?

> ide_setup_dma() call is still needed for calling ide_allocate_dma_engine()

    Er, looking at sgiioc4.c, I somewhat doubt it -- since that driver 
allocates DMA engine of the _different_ size all by itself...
    However, the driver cleanup function might be called just before releasing 
the engine in ide-dma.c, so this approach sounds reasonable. :-)

> and setting up ->dma_* / ->ide_dma_* fields.

    Yeah, it seems I've forgotten about them a bit... :-)

[skipped]

    Another question: ide_setup_dma() is not supposed to be called by non-PCI 
driver, yes?
It's just I know one (not in the community yet) that does... :-/

> Bartlomiej

WBR, Sergei

Re: + fix-ide-dma-resource-managment.patch added to -mm tree

[Bartlomiej Zolnierkiewicz]

On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
> >>>On 3/27/06, Sergei Shtylylov <sshtylyov@ru.mvista.com> wrote:
>
> [skipped]
>
> >>>>>>- release the same number of DMA I/O ports that was requested by a driver
>
> >>>>>please fix trm290.c to use ->mmio == 2 instead
>
> >>>>   It's a _really_ old chipset (pre SFF-8038i) and has _no_ memory mapped
> >>>>regs AFAIK...
>
> >>>->mmio == 2 means _only_ that host driver is responsible
> >>>for reserving/releasing resources - it doesn't mean that host driver
> >>>is using MMIO (a bit confusing but this is what the current code does)
>
> >>    So, your point is that ide_setup_dma() should _never_ do anything for the
> >>(mmio == 2) case? Maybe it shouldn't even be called? I guess we surely need
>
> > No, my point is that for ->mmio == 2 core code may not try to manage
> > IO or MMIO resources, only that.
>
>     Note that I didn't try to. :-)
>     Your 1st NAK was because I moved ide_release_dma_engine() above that

Yes.

> (mmio == 2) check in ide_release_dma() -- which as we've now concluded is
> perfectly correct, isn't it?

Hmm, yes... Stupid me, sorry for that.

> > ide_setup_dma() call is still needed for calling ide_allocate_dma_engine()
>
>     Er, looking at sgiioc4.c, I somewhat doubt it -- since that driver
> allocates DMA engine of the _different_ size all by itself...
>     However, the driver cleanup function might be called just before releasing
> the engine in ide-dma.c, so this approach sounds reasonable. :-)
>
> > and setting up ->dma_* / ->ide_dma_* fields.
>
>     Yeah, it seems I've forgotten about them a bit... :-)
>
> [skipped]
>
>     Another question: ide_setup_dma() is not supposed to be called by non-PCI
> driver, yes?

Yes, this was discussed some time ago on linux-ide.

> It's just I know one (not in the community yet) that does... :-/

AFAIR there was one mips specific driver,
but not in the tree => not mine problem... 8)

Bartlomiej