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

[Gregory CLEMENT <gregory.clement@free-electrons.com>]

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