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

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

[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 <saeed@marvell.com>
> ---
>  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.
mlord@pobox.com

[PATCH 1/2] sata_mv: use hpriv->base instead of the host->iomap

[Saeed Bishara]

this fixes crash bug as the iomap table is not valid for integrated controllers.

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 drivers/ata/sata_mv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 04b5717..9c9a5b0 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -870,7 +870,7 @@ static void mv_start_dma(struct ata_port *ap, void __iomem *port_mmio,
 		struct mv_host_priv *hpriv = ap->host->private_data;
 		int hard_port = mv_hardport_from_port(ap->port_no);
 		void __iomem *hc_mmio = mv_hc_base_from_port(
-				ap->host->iomap[MV_PRIMARY_BAR], hard_port);
+					mv_host_base(ap->host), hard_port);
 		u32 hc_irq_cause, ipending;
 
 		/* clear EDMA event indicators, if any */
-- 
1.5.2.5

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

[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 <saeed@marvell.com>
---
 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

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

[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

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

[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

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

[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

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

[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

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

[Martin Michlmayr]

* Mark Lord <mlord@pobox.com> [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/

Re: [PATCH 1/2] sata_mv: use hpriv->base instead of the host->iomap

[Mark Lord]

Saeed Bishara wrote:
> this fixes crash bug as the iomap table is not valid for integrated controllers.
> 
> Signed-off-by: Saeed Bishara <saeed@marvell.com>
> ---
>  drivers/ata/sata_mv.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 04b5717..9c9a5b0 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -870,7 +870,7 @@ static void mv_start_dma(struct ata_port *ap, void __iomem *port_mmio,
>  		struct mv_host_priv *hpriv = ap->host->private_data;
>  		int hard_port = mv_hardport_from_port(ap->port_no);
>  		void __iomem *hc_mmio = mv_hc_base_from_port(
> -				ap->host->iomap[MV_PRIMARY_BAR], hard_port);
> +					mv_host_base(ap->host), hard_port);
>  		u32 hc_irq_cause, ipending;
>  
>  		/* clear EDMA event indicators, if any */
..

This one looks fine.

Acked-by: Mark Lord <mlord@pobox.com>

Cheers

Re: [PATCH 1/2] sata_mv: use hpriv->base instead of the host->iomap

[Jeff Garzik]

Saeed Bishara wrote:
> this fixes crash bug as the iomap table is not valid for integrated controllers.
> 
> Signed-off-by: Saeed Bishara <saeed@marvell.com>
> ---
>  drivers/ata/sata_mv.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 04b5717..9c9a5b0 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -870,7 +870,7 @@ static void mv_start_dma(struct ata_port *ap, void __iomem *port_mmio,
>  		struct mv_host_priv *hpriv = ap->host->private_data;
>  		int hard_port = mv_hardport_from_port(ap->port_no);
>  		void __iomem *hc_mmio = mv_hc_base_from_port(
> -				ap->host->iomap[MV_PRIMARY_BAR], hard_port);
> +					mv_host_base(ap->host), hard_port);
>  		u32 hc_irq_cause, ipending;

applied