Re: [PATCH 2/2] sata_mv: remove iounmap in mv_platform_remove

2008-02-16 Thread Martin Michlmayr 
* Mark Lord [EMAIL PROTECTED] [2008-02-13 14:31]:
  struct device *dev = pdev-dev;
  struct ata_host *host = dev_get_drvdata(dev);
 -struct mv_host_priv *hpriv = host-private_data;
 -void __iomem *base = hpriv-base;
  ata_host_detach(host);
 -iounmap(base);
  return 0;
  }
 ..

 Where does the iounmap() now get done instead of that place?

You commented on the 2nd patch but can you please review/ack the 1st
one so this can go in.  Thanks.
-- 
Martin Michlmayr
http://www.cyrius.com/
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] sata_mv: remove iounmap in mv_platform_remove

2008-02-14 Thread saeed bishara 
   this will fix crash bug when doing rmmod to the driver, this is because the
   port_stop function get called later and it could access the device's 
 registers.
  

  Where does the iounmap() now get done instead of that place?

nowhere, the /proc/iomem still shows that sata_mv uses io mempry after
the rrmod, but when I re-load the driver, I can see it reuses the same
io address range. so it doesn't waste io mem  resources.

saeed
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] sata_mv: remove iounmap in mv_platform_remove

2008-02-14 Thread Mark Lord 

saeed bishara wrote:

  this will fix crash bug when doing rmmod to the driver, this is because the
  port_stop function get called later and it could access the device's 
registers.
 

 Where does the iounmap() now get done instead of that place?


nowhere, the /proc/iomem still shows that sata_mv uses io mempry after
the rrmod, but when I re-load the driver, I can see it reuses the same
io address range. so it doesn't waste io mem  resources.

..

Mmm.. sounds like a bug to me.  Possibly two bugs:

1. the ioremap() should fail if the range is already mapped, and
2. we should free the resources on module unload.

I suppose this would be mostly automatic if the code simply
were to use devm_ioremap() instead of ioremap().

Cheers
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] sata_mv: remove iounmap in mv_platform_remove

2008-02-14 Thread saeed bishara 

  Mmm.. sounds like a bug to me.  Possibly two bugs:

  1. the ioremap() should fail if the range is already mapped, and
  2. we should free the resources on module unload.

  I suppose this would be mostly automatic if the code simply
  were to use devm_ioremap() instead of ioremap().
my understanding for the /proc/iomem was totaly wrong..
the ioremap() actually allocated different address each time, I don't
know if should fail if the range is already mapped, but maybe the name
(remap) hints that it is shouldn't.
anyway, I replaced ioremap with devm_ioremap and it looks fine. I'll
send the new patch in different email.

saeed
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] sata_mv: remove iounmap in mv_platform_remove

2008-02-14 Thread Mark Lord 

saeed bishara wrote:

 Mmm.. sounds like a bug to me.  Possibly two bugs:

 1. the ioremap() should fail if the range is already mapped, and
 2. we should free the resources on module unload.

 I suppose this would be mostly automatic if the code simply
 were to use devm_ioremap() instead of ioremap().

my understanding for the /proc/iomem was totaly wrong..
the ioremap() actually allocated different address each time,

..

Good.  That makes a lot more sense.


I don't know if should fail if the range is already mapped,
but maybe the name (remap) hints that it is shouldn't.
anyway, I replaced ioremap with devm_ioremap and it looks fine. I'll
send the new patch in different email.

..

I think that the code should first call devm_request_region()
to reserve the ioports -- this will then fail if the ports are in-use.
After success there, it should then call devm_ioremap(),
which gives a virtual address back (the remap part) for use within the kernel.

Cheers
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] sata_mv: remove iounmap in mv_platform_remove

2008-02-13 Thread Saeed Bishara 
this will fix crash bug when doing rmmod to the driver, this is because the
port_stop function get called later and it could access the device's registers.

Signed-off-by: Saeed Bishara [EMAIL PROTECTED]
---
 drivers/ata/sata_mv.c |3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 9c9a5b0..c61e666 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2983,11 +2983,8 @@ static int __devexit mv_platform_remove(struct 
platform_device *pdev)
 {
struct device *dev = pdev-dev;
struct ata_host *host = dev_get_drvdata(dev);
-   struct mv_host_priv *hpriv = host-private_data;
-   void __iomem *base = hpriv-base;
 
ata_host_detach(host);
-   iounmap(base);
return 0;
 }
 
-- 
1.5.2.5

-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] sata_mv: remove iounmap in mv_platform_remove

2008-02-13 Thread Mark Lord 

Saeed Bishara wrote:

this will fix crash bug when doing rmmod to the driver, this is because the
port_stop function get called later and it could access the device's registers.

Signed-off-by: Saeed Bishara [EMAIL PROTECTED]
---
 drivers/ata/sata_mv.c |3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 9c9a5b0..c61e666 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2983,11 +2983,8 @@ static int __devexit mv_platform_remove(struct 
platform_device *pdev)
 {
struct device *dev = pdev-dev;
struct ata_host *host = dev_get_drvdata(dev);
-   struct mv_host_priv *hpriv = host-private_data;
-   void __iomem *base = hpriv-base;
 
 	ata_host_detach(host);

-   iounmap(base);
return 0;
 }

..

Where does the iounmap() now get done instead of that place?


--
Mark Lord
Real-Time Remedies Inc.
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html