[PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Andrew Lunn]

Armada 370 and XP do not have a SATA phy driver.  The generic phy
layer does not cleanly support optional phys. It is not possible to
determine from the error code if there is expected to be a phy
according to DT, but it cannot be found, or no phy is listed in
DT. All that can be determined is that a phy is expected, but the
driver has not been loaded yet, in which case -EPROBE_DEFER is
returned. Thus for 370 and XP the driver failed to probe.  Play safe,
consider all errors except -EPROBE_DEFER to be none fatal and keep
going, and in the case of -EPROBE_DEFER exit the probe function with
that error code.

Tested on Kirkwood with a sata phy driver and on 370 without a sata
phy driver.

Reported-by: Jean Pihet <jean.pihet@linaro.org>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Gregory Clement <gregory.clement@free-electrons.com>
---
 drivers/ata/sata_mv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index eaa21eddbe70..148ff5a82c8b 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -4115,9 +4115,8 @@ static int mv_platform_probe(struct platform_device *pdev)
 		if (IS_ERR(hpriv->port_phys[port])) {
 			rc = PTR_ERR(hpriv->port_phys[port]);
 			hpriv->port_phys[port] = NULL;
-			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
-				dev_warn(&pdev->dev, "error getting phy");
-			goto err;
+			if (rc == -EPROBE_DEFER)
+				goto err;
 		} else
 			phy_power_on(hpriv->port_phys[port]);
 	}
-- 
1.8.5.2

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Ezequiel Garcia]

On Thu, Jan 30, 2014 at 09:50:35PM +0100, Andrew Lunn wrote:
> Armada 370 and XP do not have a SATA phy driver.  The generic phy
> layer does not cleanly support optional phys. It is not possible to
> determine from the error code if there is expected to be a phy
> according to DT, but it cannot be found, or no phy is listed in
> DT. All that can be determined is that a phy is expected, but the
> driver has not been loaded yet, in which case -EPROBE_DEFER is
> returned. Thus for 370 and XP the driver failed to probe.  Play safe,
> consider all errors except -EPROBE_DEFER to be none fatal and keep
> going, and in the case of -EPROBE_DEFER exit the probe function with
> that error code.
> 
> Tested on Kirkwood with a sata phy driver and on 370 without a sata
> phy driver.
> 
> Reported-by: Jean Pihet <jean.pihet@linaro.org>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Gregory Clement <gregory.clement@free-electrons.com>
> ---
>  drivers/ata/sata_mv.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index eaa21eddbe70..148ff5a82c8b 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -4115,9 +4115,8 @@ static int mv_platform_probe(struct platform_device *pdev)
>  		if (IS_ERR(hpriv->port_phys[port])) {
>  			rc = PTR_ERR(hpriv->port_phys[port]);
>  			hpriv->port_phys[port] = NULL;
> -			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
> -				dev_warn(&pdev->dev, "error getting phy");
> -			goto err;
> +			if (rc == -EPROBE_DEFER)
> +				goto err;

It feels a bit fishy to check for a specific errno.

How about not considering the lack of phy an error in all cases? In
other words, remove the check completely.

Isn't the phy used only for power saving purposes? Or do we want this
for another purpose?

Or as a different solution, can't we check for the compatible-string
and only try to get a phy for orion-sata?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Andrew Lunn]

On Thu, Jan 30, 2014 at 07:12:28PM -0300, Ezequiel Garcia wrote:
> On Thu, Jan 30, 2014 at 09:50:35PM +0100, Andrew Lunn wrote:
> > Armada 370 and XP do not have a SATA phy driver.  The generic phy
> > layer does not cleanly support optional phys. It is not possible to
> > determine from the error code if there is expected to be a phy
> > according to DT, but it cannot be found, or no phy is listed in
> > DT. All that can be determined is that a phy is expected, but the
> > driver has not been loaded yet, in which case -EPROBE_DEFER is
> > returned. Thus for 370 and XP the driver failed to probe.  Play safe,
> > consider all errors except -EPROBE_DEFER to be none fatal and keep
> > going, and in the case of -EPROBE_DEFER exit the probe function with
> > that error code.
> > 
> > Tested on Kirkwood with a sata phy driver and on 370 without a sata
> > phy driver.
> > 
> > Reported-by: Jean Pihet <jean.pihet@linaro.org>
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > Tested-by: Gregory Clement <gregory.clement@free-electrons.com>
> > ---
> >  drivers/ata/sata_mv.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > index eaa21eddbe70..148ff5a82c8b 100644
> > --- a/drivers/ata/sata_mv.c
> > +++ b/drivers/ata/sata_mv.c
> > @@ -4115,9 +4115,8 @@ static int mv_platform_probe(struct platform_device *pdev)
> >  		if (IS_ERR(hpriv->port_phys[port])) {
> >  			rc = PTR_ERR(hpriv->port_phys[port]);
> >  			hpriv->port_phys[port] = NULL;
> > -			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
> > -				dev_warn(&pdev->dev, "error getting phy");
> > -			goto err;
> > +			if (rc == -EPROBE_DEFER)
> > +				goto err;
> 
> It feels a bit fishy to check for a specific errno.
> 
> How about not considering the lack of phy an error in all cases? In
> other words, remove the check completely.

Bad things would happen. EPROBE_DEFER means there is a phy driver, but
because of the non-deterministic ordering of loading drivers, it has
not been loaded yet. The sata_mv driver needs to fail its probe with
EPROBE_DEFER, giving the phy driver chance to load, and then when
sata_mv loads for a second time it will find the phy driver. If we
ignored the EPROBE_DEFER and sata_mv loaded, it would be out of sync
with the phy driver, resulting in the phy being turned off, and the
discs would never be found.

So

EPROBE_DEFER: We need to fail the probe, but it is not fatal.
ENOSYS: No generic PHY framework, sata_mv can load.
ENODEV: No phy, probably because it is optional and not there, sata_mv can load.
ENOMEM, EINVAL, etc are real errors and should probably be fatal and
returned by the probe function.

So i could reverse the comparison, look for ENOSYS and ENODEV and
allow the probe to succeed and return the error in all other cases.

> Isn't the phy used only for power saving purposes? Or do we want this
> for another purpose?

Yes. On Dove it can save around 10% of the idle power. I don't have
kirkwood numbers at the moment, but it is probably similar.
 
> Or as a different solution, can't we check for the compatible-string
> and only try to get a phy for orion-sata?

Orion5x cannot control its phy. Nor can PCI cards using the same IP
core in discreet chips. I also hope that at some point 370 and XP gain
phy support. I would really like this to work just like clocks do,
where the clocks are optional and if they are in the DT node they are
used, otherwise they are not.

      Andrew

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Tejun Heo]

On Fri, Jan 31, 2014 at 11:54:11AM +0100, Andrew Lunn wrote:
> EPROBE_DEFER: We need to fail the probe, but it is not fatal.
> ENOSYS: No generic PHY framework, sata_mv can load.
> ENODEV: No phy, probably because it is optional and not there, sata_mv can load.
> ENOMEM, EINVAL, etc are real errors and should probably be fatal and
> returned by the probe function.
> 
> So i could reverse the comparison, look for ENOSYS and ENODEV and
> allow the probe to succeed and return the error in all other cases.

Or add a helper, e.g. is_phy_error_fatal(), so that the knowledge
about specific error codes don't end up getting spread through the
code base?

Thanks.

-- 
tejun

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Andrew Lunn]

On Fri, Jan 31, 2014 at 06:04:22AM -0500, Tejun Heo wrote:
> On Fri, Jan 31, 2014 at 11:54:11AM +0100, Andrew Lunn wrote:
> > EPROBE_DEFER: We need to fail the probe, but it is not fatal.
> > ENOSYS: No generic PHY framework, sata_mv can load.
> > ENODEV: No phy, probably because it is optional and not there, sata_mv can load.
> > ENOMEM, EINVAL, etc are real errors and should probably be fatal and
> > returned by the probe function.
> > 
> > So i could reverse the comparison, look for ENOSYS and ENODEV and
> > allow the probe to succeed and return the error in all other cases.
> 
> Or add a helper, e.g. is_phy_error_fatal(), so that the knowledge
> about specific error codes don't end up getting spread through the
> code base?

Hi Tejun

The problem here is there is a different between optional and non
optional phys. If it is not optional, ENODEV is fatal. If it is
optional ENODEV is not fatal. So it needs at least to be

bool is_phy_error_fatal(struct phy *phy, bool optional)

Also, EPROBE_DEFER is not fatal, but still needs the probe to return
an error code. So the code ends up something like

	phy = devm_phy_get(...);
   	if (IS_ERR(phy) {
      		if (is_phy_error_fatal(phy, true)) {
			dev_err(dev, "Fatal phy error);
	 		goto out;
      		}
		if (PTR_ERR(phy) == -EPROBE_DEFER)
			goto out;
	}

I can implement this if you want.

  Andrew

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Tejun Heo]

On Fri, Jan 31, 2014 at 12:46:19PM +0100, Andrew Lunn wrote:
> The problem here is there is a different between optional and non
> optional phys. If it is not optional, ENODEV is fatal. If it is
> optional ENODEV is not fatal. So it needs at least to be
> 
> bool is_phy_error_fatal(struct phy *phy, bool optional)
> 
> Also, EPROBE_DEFER is not fatal, but still needs the probe to return
> an error code. So the code ends up something like
> 
> 	phy = devm_phy_get(...);
>    	if (IS_ERR(phy) {
>       		if (is_phy_error_fatal(phy, true)) {
> 			dev_err(dev, "Fatal phy error);
> 	 		goto out;
>       		}
> 		if (PTR_ERR(phy) == -EPROBE_DEFER)
> 			goto out;
> 	}
> 
> I can implement this if you want.

Ugh... it's too ugly.  This really should be contained in phy layer.
How about introducing devm_phy_get_optional() which does the error
value interpretation inside it and just returns 0 if probing can go on
and -errno if it shouldn't.

Thanks.

-- 
tejun

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Gregory CLEMENT]

Hi Andrew, Ezequiel,

On 31/01/2014 11:54, Andrew Lunn wrote:
> On Thu, Jan 30, 2014 at 07:12:28PM -0300, Ezequiel Garcia wrote:
>> On Thu, Jan 30, 2014 at 09:50:35PM +0100, Andrew Lunn wrote:
>>> Armada 370 and XP do not have a SATA phy driver.  The generic phy
>>> layer does not cleanly support optional phys. It is not possible to
>>> determine from the error code if there is expected to be a phy
>>> according to DT, but it cannot be found, or no phy is listed in
>>> DT. All that can be determined is that a phy is expected, but the
>>> driver has not been loaded yet, in which case -EPROBE_DEFER is
>>> returned. Thus for 370 and XP the driver failed to probe.  Play safe,
>>> consider all errors except -EPROBE_DEFER to be none fatal and keep
>>> going, and in the case of -EPROBE_DEFER exit the probe function with
>>> that error code.
>>>
>>> Tested on Kirkwood with a sata phy driver and on 370 without a sata
>>> phy driver.

As expected kernel fails booting on Armada 370 and Armada XP when SATA
is selected (so by default with mvebu_defconfig and multi_v7_defconfig)
on 3.14-rc1. I would realy like to see this issue fixed for 3.14-rc2.

>>>
>>> Reported-by: Jean Pihet <jean.pihet@linaro.org>
>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>>> Tested-by: Gregory Clement <gregory.clement@free-electrons.com>
>>> ---
>>>  drivers/ata/sata_mv.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>>> index eaa21eddbe70..148ff5a82c8b 100644
>>> --- a/drivers/ata/sata_mv.c
>>> +++ b/drivers/ata/sata_mv.c
>>> @@ -4115,9 +4115,8 @@ static int mv_platform_probe(struct platform_device *pdev)
>>>  		if (IS_ERR(hpriv->port_phys[port])) {
>>>  			rc = PTR_ERR(hpriv->port_phys[port]);
>>>  			hpriv->port_phys[port] = NULL;
>>> -			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
>>> -				dev_warn(&pdev->dev, "error getting phy");
>>> -			goto err;
>>> +			if (rc == -EPROBE_DEFER)
>>> +				goto err;
>>
>> It feels a bit fishy to check for a specific errno.

EPROBE_DEFER is a very special errno so from my point of view it is
not so surprising to have a specific treatment for this case.

>>
>> How about not considering the lack of phy an error in all cases? In
>> other words, remove the check completely.
> 
> Bad things would happen. EPROBE_DEFER means there is a phy driver, but
> because of the non-deterministic ordering of loading drivers, it has
> not been loaded yet. The sata_mv driver needs to fail its probe with
> EPROBE_DEFER, giving the phy driver chance to load, and then when
> sata_mv loads for a second time it will find the phy driver. If we
> ignored the EPROBE_DEFER and sata_mv loaded, it would be out of sync
> with the phy driver, resulting in the phy being turned off, and the
> discs would never be found.
> 
> So
> 
> EPROBE_DEFER: We need to fail the probe, but it is not fatal.
> ENOSYS: No generic PHY framework, sata_mv can load.
> ENODEV: No phy, probably because it is optional and not there, sata_mv can load.
> ENOMEM, EINVAL, etc are real errors and should probably be fatal and
> returned by the probe function.
> 
> So i could reverse the comparison, look for ENOSYS and ENODEV and
> allow the probe to succeed and return the error in all other cases.

This looks more unusual for me, but I understand the logic. Indeed this
solution seems better.

Andrew, could you post a new version?
if you add the explanation you gave inside a comment just before the check,
I am sure it will be perfectly acceptable.

Thanks,

Gregory

> 
>> Isn't the phy used only for power saving purposes? Or do we want this
>> for another purpose?
> 
> Yes. On Dove it can save around 10% of the idle power. I don't have
> kirkwood numbers at the moment, but it is probably similar.
>  
>> Or as a different solution, can't we check for the compatible-string
>> and only try to get a phy for orion-sata?
> 
> Orion5x cannot control its phy. Nor can PCI cards using the same IP
> core in discreet chips. I also hope that at some point 370 and XP gain
> phy support. I would really like this to work just like clocks do,
> where the clocks are optional and if they are in the DT node they are
> used, otherwise they are not.
> 
>       Andrew
> 


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

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Jason Cooper]

+ Kevin Hilman (context kept for Kevin)

Tejun, a request below.

On Mon, Feb 03, 2014 at 04:32:46PM +0100, Gregory CLEMENT wrote:
> Hi Andrew, Ezequiel,
> 
> On 31/01/2014 11:54, Andrew Lunn wrote:
> > On Thu, Jan 30, 2014 at 07:12:28PM -0300, Ezequiel Garcia wrote:
> >> On Thu, Jan 30, 2014 at 09:50:35PM +0100, Andrew Lunn wrote:
> >>> Armada 370 and XP do not have a SATA phy driver.  The generic phy
> >>> layer does not cleanly support optional phys. It is not possible to
> >>> determine from the error code if there is expected to be a phy
> >>> according to DT, but it cannot be found, or no phy is listed in
> >>> DT. All that can be determined is that a phy is expected, but the
> >>> driver has not been loaded yet, in which case -EPROBE_DEFER is
> >>> returned. Thus for 370 and XP the driver failed to probe.  Play safe,
> >>> consider all errors except -EPROBE_DEFER to be none fatal and keep
> >>> going, and in the case of -EPROBE_DEFER exit the probe function with
> >>> that error code.
> >>>
> >>> Tested on Kirkwood with a sata phy driver and on 370 without a sata
> >>> phy driver.
> 
> As expected kernel fails booting on Armada 370 and Armada XP when SATA
> is selected (so by default with mvebu_defconfig and multi_v7_defconfig)
> on 3.14-rc1. I would realy like to see this issue fixed for 3.14-rc2.
> 
> >>>
> >>> Reported-by: Jean Pihet <jean.pihet@linaro.org>
> >>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> >>> Tested-by: Gregory Clement <gregory.clement@free-electrons.com>
> >>> ---
> >>>  drivers/ata/sata_mv.c | 5 ++---
> >>>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> >>> index eaa21eddbe70..148ff5a82c8b 100644
> >>> --- a/drivers/ata/sata_mv.c
> >>> +++ b/drivers/ata/sata_mv.c
> >>> @@ -4115,9 +4115,8 @@ static int mv_platform_probe(struct platform_device *pdev)
> >>>  		if (IS_ERR(hpriv->port_phys[port])) {
> >>>  			rc = PTR_ERR(hpriv->port_phys[port]);
> >>>  			hpriv->port_phys[port] = NULL;
> >>> -			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
> >>> -				dev_warn(&pdev->dev, "error getting phy");
> >>> -			goto err;
> >>> +			if (rc == -EPROBE_DEFER)
> >>> +				goto err;
> >>
> >> It feels a bit fishy to check for a specific errno.
> 
> EPROBE_DEFER is a very special errno so from my point of view it is
> not so surprising to have a specific treatment for this case.
> 
> >>
> >> How about not considering the lack of phy an error in all cases? In
> >> other words, remove the check completely.
> > 
> > Bad things would happen. EPROBE_DEFER means there is a phy driver, but
> > because of the non-deterministic ordering of loading drivers, it has
> > not been loaded yet. The sata_mv driver needs to fail its probe with
> > EPROBE_DEFER, giving the phy driver chance to load, and then when
> > sata_mv loads for a second time it will find the phy driver. If we
> > ignored the EPROBE_DEFER and sata_mv loaded, it would be out of sync
> > with the phy driver, resulting in the phy being turned off, and the
> > discs would never be found.
> > 
> > So
> > 
> > EPROBE_DEFER: We need to fail the probe, but it is not fatal.
> > ENOSYS: No generic PHY framework, sata_mv can load.
> > ENODEV: No phy, probably because it is optional and not there, sata_mv can load.
> > ENOMEM, EINVAL, etc are real errors and should probably be fatal and
> > returned by the probe function.
> > 
> > So i could reverse the comparison, look for ENOSYS and ENODEV and
> > allow the probe to succeed and return the error in all other cases.
> 
> This looks more unusual for me, but I understand the logic. Indeed this
> solution seems better.
> 
> Andrew, could you post a new version?
> if you add the explanation you gave inside a comment just before the check,
> I am sure it will be perfectly acceptable.

Tejun,

This patch is needed for our arm-soc bootfarms to continue testing.  It
would be helpful if, once you're ok with the patch, we took it through
arm-soc.  Would you mind Ack'ing it once you're happy with it?

thx,

Jason.

> >> Isn't the phy used only for power saving purposes? Or do we want this
> >> for another purpose?
> > 
> > Yes. On Dove it can save around 10% of the idle power. I don't have
> > kirkwood numbers at the moment, but it is probably similar.
> >  
> >> Or as a different solution, can't we check for the compatible-string
> >> and only try to get a phy for orion-sata?
> > 
> > Orion5x cannot control its phy. Nor can PCI cards using the same IP
> > core in discreet chips. I also hope that at some point 370 and XP gain
> > phy support. I would really like this to work just like clocks do,
> > where the clocks are optional and if they are in the DT node they are
> > used, otherwise they are not.
> > 
> >       Andrew
> > 
> 
> 
> -- 
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Gregory CLEMENT]

On 03/02/2014 17:50, Jason Cooper wrote:
> 
> + Kevin Hilman (context kept for Kevin)
> 
> Tejun, a request below.
> 
> On Mon, Feb 03, 2014 at 04:32:46PM +0100, Gregory CLEMENT wrote:
>> Hi Andrew, Ezequiel,
>>
>> On 31/01/2014 11:54, Andrew Lunn wrote:
>>> On Thu, Jan 30, 2014 at 07:12:28PM -0300, Ezequiel Garcia wrote:
>>>> On Thu, Jan 30, 2014 at 09:50:35PM +0100, Andrew Lunn wrote:
>>>>> Armada 370 and XP do not have a SATA phy driver.  The generic phy
>>>>> layer does not cleanly support optional phys. It is not possible to
>>>>> determine from the error code if there is expected to be a phy
>>>>> according to DT, but it cannot be found, or no phy is listed in
>>>>> DT. All that can be determined is that a phy is expected, but the
>>>>> driver has not been loaded yet, in which case -EPROBE_DEFER is
>>>>> returned. Thus for 370 and XP the driver failed to probe.  Play safe,
>>>>> consider all errors except -EPROBE_DEFER to be none fatal and keep
>>>>> going, and in the case of -EPROBE_DEFER exit the probe function with
>>>>> that error code.
>>>>>
>>>>> Tested on Kirkwood with a sata phy driver and on 370 without a sata
>>>>> phy driver.
>>
>> As expected kernel fails booting on Armada 370 and Armada XP when SATA
>> is selected (so by default with mvebu_defconfig and multi_v7_defconfig)
>> on 3.14-rc1. I would realy like to see this issue fixed for 3.14-rc2.
>>
>>>>>
>>>>> Reported-by: Jean Pihet <jean.pihet@linaro.org>
>>>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>>>>> Tested-by: Gregory Clement <gregory.clement@free-electrons.com>
>>>>> ---
>>>>>  drivers/ata/sata_mv.c | 5 ++---
>>>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>>>>> index eaa21eddbe70..148ff5a82c8b 100644
>>>>> --- a/drivers/ata/sata_mv.c
>>>>> +++ b/drivers/ata/sata_mv.c
>>>>> @@ -4115,9 +4115,8 @@ static int mv_platform_probe(struct platform_device *pdev)
>>>>>  		if (IS_ERR(hpriv->port_phys[port])) {
>>>>>  			rc = PTR_ERR(hpriv->port_phys[port]);
>>>>>  			hpriv->port_phys[port] = NULL;
>>>>> -			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
>>>>> -				dev_warn(&pdev->dev, "error getting phy");
>>>>> -			goto err;
>>>>> +			if (rc == -EPROBE_DEFER)
>>>>> +				goto err;
>>>>
>>>> It feels a bit fishy to check for a specific errno.
>>
>> EPROBE_DEFER is a very special errno so from my point of view it is
>> not so surprising to have a specific treatment for this case.
>>
>>>>
>>>> How about not considering the lack of phy an error in all cases? In
>>>> other words, remove the check completely.
>>>
>>> Bad things would happen. EPROBE_DEFER means there is a phy driver, but
>>> because of the non-deterministic ordering of loading drivers, it has
>>> not been loaded yet. The sata_mv driver needs to fail its probe with
>>> EPROBE_DEFER, giving the phy driver chance to load, and then when
>>> sata_mv loads for a second time it will find the phy driver. If we
>>> ignored the EPROBE_DEFER and sata_mv loaded, it would be out of sync
>>> with the phy driver, resulting in the phy being turned off, and the
>>> discs would never be found.
>>>
>>> So
>>>
>>> EPROBE_DEFER: We need to fail the probe, but it is not fatal.
>>> ENOSYS: No generic PHY framework, sata_mv can load.
>>> ENODEV: No phy, probably because it is optional and not there, sata_mv can load.
>>> ENOMEM, EINVAL, etc are real errors and should probably be fatal and
>>> returned by the probe function.
>>>
>>> So i could reverse the comparison, look for ENOSYS and ENODEV and
>>> allow the probe to succeed and return the error in all other cases.
>>
>> This looks more unusual for me, but I understand the logic. Indeed this
>> solution seems better.
>>
>> Andrew, could you post a new version?
>> if you add the explanation you gave inside a comment just before the check,
>> I am sure it will be perfectly acceptable.
> 
> Tejun,
> 
> This patch is needed for our arm-soc bootfarms to continue testing.  It
> would be helpful if, once you're ok with the patch, we took it through
> arm-soc.  Would you mind Ack'ing it once you're happy with it?
> 

Hi Jason,

Actually there were a new version of this series. I didn't notice it because
until today I didn't subscribed to the linux-ide mailing list.

Tejun already told he agreed to give his acked-by:

http://thread.gmane.org/gmane.linux.ide/56706/focus=56718

I also tested this series and after few try it finally worked, but Andrew
planned to send a v2 soon.

Sorry for the mess!

Gregory

> thx,
> 
> Jason.
> 
>>>> Isn't the phy used only for power saving purposes? Or do we want this
>>>> for another purpose?
>>>
>>> Yes. On Dove it can save around 10% of the idle power. I don't have
>>> kirkwood numbers at the moment, but it is probably similar.
>>>  
>>>> Or as a different solution, can't we check for the compatible-string
>>>> and only try to get a phy for orion-sata?
>>>
>>> Orion5x cannot control its phy. Nor can PCI cards using the same IP
>>> core in discreet chips. I also hope that at some point 370 and XP gain
>>> phy support. I would really like this to work just like clocks do,
>>> where the clocks are optional and if they are in the DT node they are
>>> used, otherwise they are not.
>>>
>>>       Andrew
>>>
>>
>>
>> -- 
>> Gregory Clement, Free Electrons
>> Kernel, drivers, real-time and embedded Linux
>> development, consulting, training and support.
>> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Tejun Heo]

Hello,

On Mon, Feb 03, 2014 at 11:50:12AM -0500, Jason Cooper wrote:
> > Andrew, could you post a new version?
> > if you add the explanation you gave inside a comment just before the check,
> > I am sure it will be perfectly acceptable.
> 
> Tejun,
> 
> This patch is needed for our arm-soc bootfarms to continue testing.  It
> would be helpful if, once you're ok with the patch, we took it through
> arm-soc.  Would you mind Ack'ing it once you're happy with it?

The other patches fix the same issue and should be good for -rc2,
right?

Thanks.

-- 
tejun

Re: [PATCH] ATA: SATA_MV: Fix probe failure when no phy exists

[Jason Cooper]

On Mon, Feb 03, 2014 at 12:32:40PM -0500, Tejun Heo wrote:
> Hello,
> 
> On Mon, Feb 03, 2014 at 11:50:12AM -0500, Jason Cooper wrote:
> > > Andrew, could you post a new version?
> > > if you add the explanation you gave inside a comment just before the check,
> > > I am sure it will be perfectly acceptable.
> > 
> > Tejun,
> > 
> > This patch is needed for our arm-soc bootfarms to continue testing.  It
> > would be helpful if, once you're ok with the patch, we took it through
> > arm-soc.  Would you mind Ack'ing it once you're happy with it?
> 
> The other patches fix the same issue and should be good for -rc2,
> right?

Yes, they weren't in my inbox at the time of writing.  Sorry for the
noise.  Thanks for the Ack.

thx,

Jason.