[PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Linus Walleij]

This reverts commit 368e5fbdfc60732643f34f538823ed4bc8829827.

My NAS stopped to mount root when I went from some v4.10-rc1
to v4.12-rc3. Investigation of the bootlog yields this:

sata_mv f1080000.sata: can't request region for resource [mem 0xf1080000-0xf1084fff]
sata_mv: probe of f1080000.sata failed with error -16

Reverting this offending patch makes the sata_mv probe and
my SATA drive can mount root again, hooray!

Cc: stable@vger.kernel.org
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
The problem is probably caused by overlapping resources in
the device tree, for which ioremap() and ioremap_resource()
have different semantics: the former allows it, the latter
does not.

A proper fix probably includes amending the device tree to
not have overlapping resources.
---
 drivers/ata/sata_mv.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index b66bcda88320..3b2246dded74 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -4067,7 +4067,6 @@ static int mv_platform_probe(struct platform_device *pdev)
 	struct ata_host *host;
 	struct mv_host_priv *hpriv;
 	struct resource *res;
-	void __iomem *mmio;
 	int n_ports = 0, irq = 0;
 	int rc;
 	int port;
@@ -4086,9 +4085,8 @@ static int mv_platform_probe(struct platform_device *pdev)
 	 * Get the register base first
 	 */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	mmio = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(mmio))
-		return PTR_ERR(mmio);
+	if (res == NULL)
+		return -EINVAL;
 
 	/* allocate host */
 	if (pdev->dev.of_node) {
@@ -4132,7 +4130,12 @@ static int mv_platform_probe(struct platform_device *pdev)
 	hpriv->board_idx = chip_soc;
 
 	host->iomap = NULL;
-	hpriv->base = mmio - SATAHC0_REG_BASE;
+	hpriv->base = devm_ioremap(&pdev->dev, res->start,
+				   resource_size(res));
+	if (!hpriv->base)
+		return -ENOMEM;
+
+	hpriv->base -= SATAHC0_REG_BASE;
 
 	hpriv->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(hpriv->clk))
-- 
2.9.4

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andy Shevchenko]

On Tue, 2017-05-30 at 15:52 +0200, Linus Walleij wrote:
> This reverts commit 368e5fbdfc60732643f34f538823ed4bc8829827.
> 
> My NAS stopped to mount root when I went from some v4.10-rc1
> to v4.12-rc3. Investigation of the bootlog yields this:
> 
> sata_mv f1080000.sata: can't request region for resource [mem
> 0xf1080000-0xf1084fff]
> sata_mv: probe of f1080000.sata failed with error -16
> 
> Reverting this offending patch makes the sata_mv probe and
> my SATA drive can mount root again, hooray!

It's already in upstream (ata tree).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Linus Walleij]

On Tue, May 30, 2017 at 3:55 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2017-05-30 at 15:52 +0200, Linus Walleij wrote:
>> This reverts commit 368e5fbdfc60732643f34f538823ed4bc8829827.
>>
>> My NAS stopped to mount root when I went from some v4.10-rc1
>> to v4.12-rc3. Investigation of the bootlog yields this:
>>
>> sata_mv f1080000.sata: can't request region for resource [mem
>> 0xf1080000-0xf1084fff]
>> sata_mv: probe of f1080000.sata failed with error -16
>>
>> Reverting this offending patch makes the sata_mv probe and
>> my SATA drive can mount root again, hooray!
>
> It's already in upstream (ata tree).

OK!

Only two patches for the same issue? Today for the GPIO
tree I got four (4!) patches fixing the same issue :D

Yours,
Linus Walleij

[PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andrew Lunn]

This reverts commit 368e5fbdfc60732643f34f538823ed4bc8829827.

devm_ioremap_resource() enforces that there are no overlapping
resources, where as devm_ioremap() does not. The sata phy driver needs
a subset of the sata IO address space, so maps some of the sata
address space. As a result, sata_mv now fails to probe, reporting it
cannot get its resources, and so we don't have any SATA disks.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/ata/sata_mv.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index b66bcda88320..3b2246dded74 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -4067,7 +4067,6 @@ static int mv_platform_probe(struct platform_device *pdev)
 	struct ata_host *host;
 	struct mv_host_priv *hpriv;
 	struct resource *res;
-	void __iomem *mmio;
 	int n_ports = 0, irq = 0;
 	int rc;
 	int port;
@@ -4086,9 +4085,8 @@ static int mv_platform_probe(struct platform_device *pdev)
 	 * Get the register base first
 	 */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	mmio = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(mmio))
-		return PTR_ERR(mmio);
+	if (res == NULL)
+		return -EINVAL;
 
 	/* allocate host */
 	if (pdev->dev.of_node) {
@@ -4132,7 +4130,12 @@ static int mv_platform_probe(struct platform_device *pdev)
 	hpriv->board_idx = chip_soc;
 
 	host->iomap = NULL;
-	hpriv->base = mmio - SATAHC0_REG_BASE;
+	hpriv->base = devm_ioremap(&pdev->dev, res->start,
+				   resource_size(res));
+	if (!hpriv->base)
+		return -ENOMEM;
+
+	hpriv->base -= SATAHC0_REG_BASE;
 
 	hpriv->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(hpriv->clk))
-- 
2.11.0

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Gregory CLEMENT]

Hi Andrew,
 
 On mer., mai 24 2017, Andrew Lunn <andrew@lunn.ch> wrote:

> This reverts commit 368e5fbdfc60732643f34f538823ed4bc8829827.
>
> devm_ioremap_resource() enforces that there are no overlapping
> resources, where as devm_ioremap() does not. The sata phy driver needs
> a subset of the sata IO address space, so maps some of the sata
> address space. As a result, sata_mv now fails to probe, reporting it
> cannot get its resources, and so we don't have any SATA disks.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
For this patch as it fixes a regresssion:

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

However, as the same set of register are accessed by two drivers, is
there a risk for a race condition ?

In this case we may consider to use a regmap.

Or maybe they never aces the same register ?

In this case it is safe to let the driver as is, but adding a comment
could be useful.

Thanks,

Gregory



> ---
>  drivers/ata/sata_mv.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index b66bcda88320..3b2246dded74 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -4067,7 +4067,6 @@ static int mv_platform_probe(struct platform_device *pdev)
>  	struct ata_host *host;
>  	struct mv_host_priv *hpriv;
>  	struct resource *res;
> -	void __iomem *mmio;
>  	int n_ports = 0, irq = 0;
>  	int rc;
>  	int port;
> @@ -4086,9 +4085,8 @@ static int mv_platform_probe(struct platform_device *pdev)
>  	 * Get the register base first
>  	 */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	mmio = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(mmio))
> -		return PTR_ERR(mmio);
> +	if (res == NULL)
> +		return -EINVAL;
>  
>  	/* allocate host */
>  	if (pdev->dev.of_node) {
> @@ -4132,7 +4130,12 @@ static int mv_platform_probe(struct platform_device *pdev)
>  	hpriv->board_idx = chip_soc;
>  
>  	host->iomap = NULL;
> -	hpriv->base = mmio - SATAHC0_REG_BASE;
> +	hpriv->base = devm_ioremap(&pdev->dev, res->start,
> +				   resource_size(res));
> +	if (!hpriv->base)
> +		return -ENOMEM;
> +
> +	hpriv->base -= SATAHC0_REG_BASE;
>  
>  	hpriv->clk = clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(hpriv->clk))
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andrew Lunn]

> However, as the same set of register are accessed by two drivers, is
> there a risk for a race condition ?

Hi Gregory

I checked this when i wrote the PHY driver.

phy_power_on() and phy_power_off() are only used in the probe and
remove function. Everything is done synchronously. So there is no
chance of a race.

       Andrew
 

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andy Shevchenko]

On Wed, 2017-05-24 at 15:01 +0200, Gregory CLEMENT wrote:
> Hi Andrew,
>  
>  On mer., mai 24 2017, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > This reverts commit 368e5fbdfc60732643f34f538823ed4bc8829827.
> > 
> > devm_ioremap_resource() enforces that there are no overlapping
> > resources, where as devm_ioremap() does not. The sata phy driver
> > needs
> > a subset of the sata IO address space, so maps some of the sata
> > address space. As a result, sata_mv now fails to probe, reporting it
> > cannot get its resources, and so we don't have any SATA disks.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> For this patch as it fixes a regresssion:
> 
> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> However, as the same set of register are accessed by two drivers, is
> there a risk for a race condition ?
> 
> In this case we may consider to use a regmap.
> 
> Or maybe they never aces the same register ?

I'm wondering where exactly first resource acquiring is happening.

For me seems that the initial patch unhides the issue.

> 
> In this case it is safe to let the driver as is, but adding a comment
> could be useful.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andrew Lunn]

> I'm wondering where exactly first resource acquiring is happening.

drivers/phy/phy-mvebu-sata.c

	Andrew

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andy Shevchenko]

On Wed, 2017-05-24 at 15:41 +0200, Andrew Lunn wrote:
> > I'm wondering where exactly first resource acquiring is happening.
> 
> drivers/phy/phy-mvebu-sata.c
> 

In which tree? I have nothing like this in linux-next.

I'm on the side that revert just hides the real issue back.

So, it looks like better to remove exclusivity from platform core
instead.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Gregory CLEMENT]

Hi Andy,
 
 On mer., mai 24 2017, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, 2017-05-24 at 15:41 +0200, Andrew Lunn wrote:
>> > I'm wondering where exactly first resource acquiring is happening.
>> 
>> drivers/phy/phy-mvebu-sata.c
>> 
>
> In which tree? I have nothing like this in linux-next.

in v4.12-rc2 it still exist:
http://elixir.free-electrons.com/linux/v4.12-rc2/source/drivers/phy/phy-mvebu-sata.c

Gregory

>
> I'm on the side that revert just hides the real issue back.
>
> So, it looks like better to remove exclusivity from platform core
> instead.
>
> -- 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andrew Lunn]

On Wed, May 24, 2017 at 05:00:04PM +0300, Andy Shevchenko wrote:
> On Wed, 2017-05-24 at 15:41 +0200, Andrew Lunn wrote:
> > > I'm wondering where exactly first resource acquiring is happening.
> > 
> > drivers/phy/phy-mvebu-sata.c
> > 
> 
> In which tree? I have nothing like this in linux-next.

git log drivers/phy/phy-mvebu-sata.c

It has been there since Tue Dec 17 21:21:50 2013.

> I'm on the side that revert just hides the real issue back.

What issue? You are allowed to have overlapping memory resources.

     Andrew

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andy Shevchenko]

On Wed, 2017-05-24 at 16:29 +0200, Andrew Lunn wrote:
> On Wed, May 24, 2017 at 05:00:04PM +0300, Andy Shevchenko wrote:
> > On Wed, 2017-05-24 at 15:41 +0200, Andrew Lunn wrote:
> > > > I'm wondering where exactly first resource acquiring is
> > > > happening.
> > > 
> > > drivers/phy/phy-mvebu-sata.c
> > > 
> > 
> > In which tree? I have nothing like this in linux-next.
> 
> git log drivers/phy/phy-mvebu-sata.c
> 
> It has been there since Tue Dec 17 21:21:50 2013.
> 
> > I'm on the side that revert just hides the real issue back.
> 
> What issue? You are allowed to have overlapping memory resources.

Exactly, and then the question why did you do a revert?

AFAIU the problem is that platform core tries to reserve the resource
and you can't do it twice for overlapping region (see insert_resource()
call implementation).

Since it's allowed and we have more devices which requires that
(basically that ones that have phy embedded in common address space) we
might reconsider the policy used in platform core.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andrew Lunn]

> > What issue? You are allowed to have overlapping memory resources.
> 
> Exactly, and then the question why did you do a revert?

Because we are at -rc2. We need a quick fix now, so that my NAS box
disks start spinning again. You can then spend some time to consider a
better fix, and get it merged in the 4.13 merge window.

Please feel free to Cc: me and the other Marvell maintainers on
patches, so we can test them.

       Andrew

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Andy Shevchenko]

On Thu, 2017-05-25 at 15:26 +0200, Andrew Lunn wrote:
> > > What issue? You are allowed to have overlapping memory resources.
> > 
> > Exactly, and then the question why did you do a revert?
> 
> Because we are at -rc2.

No, my point is that (repeating myself) revert just *hides* the actual
issue. 

Yes, I understand that it's a quick fix, and it should not be considered
as best solution.

>  We need a quick fix now, so that my NAS box
> disks start spinning again. You can then spend some time to consider a
> better fix, and get it merged in the 4.13 merge window.
> 

> Please feel free to Cc: me and the other Marvell maintainers on
> patches, so we can test them.

I'm not going to do that since I have no such issue, no possibility to
test and actually choose best strategy with your hardware.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

[Tejun Heo]

On Wed, May 24, 2017 at 01:39:35AM +0200, Andrew Lunn wrote:
> This reverts commit 368e5fbdfc60732643f34f538823ed4bc8829827.
> 
> devm_ioremap_resource() enforces that there are no overlapping
> resources, where as devm_ioremap() does not. The sata phy driver needs
> a subset of the sata IO address space, so maps some of the sata
> address space. As a result, sata_mv now fails to probe, reporting it
> cannot get its resources, and so we don't have any SATA disks.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Applied to libata/v4.12-fixes.

Thanks.

-- 
tejun