[PATCH] of_mdio: fix phy interrupt passing

[PATCH] of_mdio: fix phy interrupt passing

[Ben Dooks]

The of_mdiobus_register_phy() is not setting phy->irq this causing
some drivers to incorrectly assume that the PHY does not have an
IRQ associated with it or install an interrupt handler for the
PHY.

Simplify the code setting irq and set the phy->irq at the same
time so that the case if mdio->irq is not NULL is easier to read.

This fixes the issue:
 net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI

to the correct:
 net eth0: attached PHY 1 (IRQ 416) to driver Micrel KSZ8041RNLI

Signed-off-by: Ben Dooks <ben.dooks-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>
---
 drivers/of/of_mdio.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 875b7b6..7b3e7b0 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -44,7 +44,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 {
 	struct phy_device *phy;
 	bool is_c45;
-	int rc, prev_irq;
+	int rc;
 	u32 max_speed = 0;
 
 	is_c45 = of_device_is_compatible(child,
@@ -55,11 +55,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 		return 1;
 
 	if (mdio->irq) {
-		prev_irq = mdio->irq[addr];
-		mdio->irq[addr] =
-			irq_of_parse_and_map(child, 0);
-		if (!mdio->irq[addr])
-			mdio->irq[addr] = prev_irq;
+		rc = irq_of_parse_and_map(child, 0);
+		if (rc > 0) {
+			mdio->irq[addr] = rc;
+			phy->irq = rc;
+		}
 	}
 
 	/* Associate the OF node with the device structure so it
-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of_mdio: fix phy interrupt passing

[Florian Fainelli]

Hi Ben,

2014-02-17 8:29 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>:
> The of_mdiobus_register_phy() is not setting phy->irq this causing
> some drivers to incorrectly assume that the PHY does not have an
> IRQ associated with it or install an interrupt handler for the
> PHY.
>
> Simplify the code setting irq and set the phy->irq at the same
> time so that the case if mdio->irq is not NULL is easier to read.

The real bug fix, which is not properly explained here, is that
irq_of_parse_and_map() should return values > 0 when the interrupt is
valid, so this makes me wonder why we are not propagating the return
value from irq_of_parse_and_map() in case the call to
of_irq_parse_one() does return something non-zero?

Other than that, I agree with the resolution, but it deserves a better
commit message, and eventually doing this in two steps:

- first fix the bug by changing the if (!mdio->irq[addr]) into a if
(mdio->irq[addr] <= 0)
- second submit a patch which cleanups the assignment

>
> This fixes the issue:
>  net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI
>
> to the correct:
>  net eth0: attached PHY 1 (IRQ 416) to driver Micrel KSZ8041RNLI
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/of/of_mdio.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 875b7b6..7b3e7b0 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -44,7 +44,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>  {
>         struct phy_device *phy;
>         bool is_c45;
> -       int rc, prev_irq;
> +       int rc;
>         u32 max_speed = 0;
>
>         is_c45 = of_device_is_compatible(child,
> @@ -55,11 +55,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>                 return 1;
>
>         if (mdio->irq) {
> -               prev_irq = mdio->irq[addr];
> -               mdio->irq[addr] =
> -                       irq_of_parse_and_map(child, 0);
> -               if (!mdio->irq[addr])
> -                       mdio->irq[addr] = prev_irq;
> +               rc = irq_of_parse_and_map(child, 0);
> +               if (rc > 0) {
> +                       mdio->irq[addr] = rc;
> +                       phy->irq = rc;
> +               }
>         }
>
>         /* Associate the OF node with the device structure so it
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Florian

Re: [PATCH] of_mdio: fix phy interrupt passing

[Ben Dooks]

On 17/02/14 17:26, Florian Fainelli wrote:
> Hi Ben,
>
> 2014-02-17 8:29 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>:
>> The of_mdiobus_register_phy() is not setting phy->irq this causing
>> some drivers to incorrectly assume that the PHY does not have an
>> IRQ associated with it or install an interrupt handler for the
>> PHY.
>>
>> Simplify the code setting irq and set the phy->irq at the same
>> time so that the case if mdio->irq is not NULL is easier to read.
>
> The real bug fix, which is not properly explained here, is that
> irq_of_parse_and_map() should return values > 0 when the interrupt is
> valid, so this makes me wonder why we are not propagating the return
> value from irq_of_parse_and_map() in case the call to
> of_irq_parse_one() does return something non-zero?

No, the first issue is phy->dev never gets set, which causes the
issue. The cleanup was added as it seemed easier to put it in with
this.

I think phy->irq is already initialised to PHY_POLL and thus there
is no need to set phy->irq if the irq_of_parse_and_map() fails.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] of_mdio: fix phy interrupt passing

[Florian Fainelli]

2014-02-17 9:42 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>:
> On 17/02/14 17:26, Florian Fainelli wrote:
>>
>> Hi Ben,
>>
>> 2014-02-17 8:29 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>:
>>>
>>> The of_mdiobus_register_phy() is not setting phy->irq this causing
>>> some drivers to incorrectly assume that the PHY does not have an
>>> IRQ associated with it or install an interrupt handler for the
>>> PHY.
>>>
>>> Simplify the code setting irq and set the phy->irq at the same
>>> time so that the case if mdio->irq is not NULL is easier to read.
>>
>>
>> The real bug fix, which is not properly explained here, is that
>> irq_of_parse_and_map() should return values > 0 when the interrupt is
>> valid, so this makes me wonder why we are not propagating the return
>> value from irq_of_parse_and_map() in case the call to
>> of_irq_parse_one() does return something non-zero?
>
>
> No, the first issue is phy->dev never gets set, which causes the
> issue. The cleanup was added as it seemed easier to put it in with
> this.

Ok, that really needs to be mentioned in the commit message, even
being quite familiar (and possibly dumb too) with the code, I could
not figure this out by reading your patch.

>
> I think phy->irq is already initialised to PHY_POLL and thus there
> is no need to set phy->irq if the irq_of_parse_and_map() fails.

That is correct, the reason why I introduced 7d97637 ("net: of_mdio:
do not overwrite PHY interrupt configuration") was that you are also
allowed to change the irq type before calling into
of_mdiobus_register(), so we want to preserve other irq values being
set here, such as PHY_IGNORE_INTERRUPT. Your patch does take care of
that since it only overrides the irq in case we could parse it.
-- 
Florian

Re: [PATCH] of_mdio: fix phy interrupt passing

[Ben Dooks]

On 17/02/14 17:26, Florian Fainelli wrote:
> Hi Ben,
>
> 2014-02-17 8:29 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>:
>> The of_mdiobus_register_phy() is not setting phy->irq this causing
>> some drivers to incorrectly assume that the PHY does not have an
>> IRQ associated with it or install an interrupt handler for the
>> PHY.
>>
>> Simplify the code setting irq and set the phy->irq at the same
>> time so that the case if mdio->irq is not NULL is easier to read.
>
> The real bug fix, which is not properly explained here, is that
> irq_of_parse_and_map() should return values > 0 when the interrupt is
> valid, so this makes me wonder why we are not propagating the return
> value from irq_of_parse_and_map() in case the call to
> of_irq_parse_one() does return something non-zero?

	rc = irq_of_parse_and_map(child, 0);
	if (rc > 0) {
		mdio->irq[addr] = rc;
		phy->irq = rc;
	}

Unfortunately we do not get any idea of what error went wrong
when doing this, all we get it 0 on failure. So we cannot tell
if the problem was due to no interrupt being there or there was
an interrupt and it could not be parsed properly.

The best we can do is assume that the PHY is happy with being
polled and the specific driver will error out if it expects to
be able to work with an interrupt.

If we error out, we may end up stopping any PHYs where there is
no interrupt available from working. Also, if we fail to parse
then we can generally fall back to polling.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] of_mdio: fix phy interrupt passing

[Ben Dooks]

On 17/02/14 17:48, Florian Fainelli wrote:
> 2014-02-17 9:42 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>:
>> On 17/02/14 17:26, Florian Fainelli wrote:
>>>
>>> Hi Ben,
>>>
>>> 2014-02-17 8:29 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>:
>>>>
>>>> The of_mdiobus_register_phy() is not setting phy->irq this causing
>>>> some drivers to incorrectly assume that the PHY does not have an
>>>> IRQ associated with it or install an interrupt handler for the
>>>> PHY.
>>>>
>>>> Simplify the code setting irq and set the phy->irq at the same
>>>> time so that the case if mdio->irq is not NULL is easier to read.
>>>
>>>
>>> The real bug fix, which is not properly explained here, is that
>>> irq_of_parse_and_map() should return values > 0 when the interrupt is
>>> valid, so this makes me wonder why we are not propagating the return
>>> value from irq_of_parse_and_map() in case the call to
>>> of_irq_parse_one() does return something non-zero?
>>
>>
>> No, the first issue is phy->dev never gets set, which causes the
>> issue. The cleanup was added as it seemed easier to put it in with
>> this.
>
> Ok, that really needs to be mentioned in the commit message, even
> being quite familiar (and possibly dumb too) with the code, I could
> not figure this out by reading your patch.
>
>>
>> I think phy->irq is already initialised to PHY_POLL and thus there
>> is no need to set phy->irq if the irq_of_parse_and_map() fails.
>
> That is correct, the reason why I introduced 7d97637 ("net: of_mdio:
> do not overwrite PHY interrupt configuration") was that you are also
> allowed to change the irq type before calling into
> of_mdiobus_register(), so we want to preserve other irq values being
> set here, such as PHY_IGNORE_INTERRUPT. Your patch does take care of
> that since it only overrides the irq in case we could parse it.

Ok, the first sentence has a spell of thus, but does refer to phy->irq
being the problem we are looking to fix in this patch.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] of_mdio: fix phy interrupt passing

[Grant Likely]

On Mon, 17 Feb 2014 16:29:40 +0000, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> The of_mdiobus_register_phy() is not setting phy->irq this causing
> some drivers to incorrectly assume that the PHY does not have an
> IRQ associated with it or install an interrupt handler for the
> PHY.
> 
> Simplify the code setting irq and set the phy->irq at the same
> time so that the case if mdio->irq is not NULL is easier to read.
> 
> This fixes the issue:
>  net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI
> 
> to the correct:
>  net eth0: attached PHY 1 (IRQ 416) to driver Micrel KSZ8041RNLI
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/of/of_mdio.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 875b7b6..7b3e7b0 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -44,7 +44,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>  {
>  	struct phy_device *phy;
>  	bool is_c45;
> -	int rc, prev_irq;
> +	int rc;
>  	u32 max_speed = 0;
>  
>  	is_c45 = of_device_is_compatible(child,
> @@ -55,11 +55,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>  		return 1;
>  
>  	if (mdio->irq) {
> -		prev_irq = mdio->irq[addr];
> -		mdio->irq[addr] =
> -			irq_of_parse_and_map(child, 0);
> -		if (!mdio->irq[addr])
> -			mdio->irq[addr] = prev_irq;

I cannot for the life for me remeber why the code was structured that
way. Your change is better.

> +		rc = irq_of_parse_and_map(child, 0);
> +		if (rc > 0) {
> +			mdio->irq[addr] = rc;
> +			phy->irq = rc;
> +		}
>  	}

The outer if is merely protecting against no irq array being allocated
for the bus. Would not the following be better:

	rc = irq_of_parse_and_map(child, 0);
	if (rc > 0) {
		phy->irq = rc;
		if (mdio->irq)
			mdio->irq[addr] = rc;
	}

g.

Re: [PATCH] of_mdio: fix phy interrupt passing

[Ben Dooks]

On 18/02/14 09:30, Grant Likely wrote:
> On Mon, 17 Feb 2014 16:29:40 +0000, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> The of_mdiobus_register_phy() is not setting phy->irq this causing
>> some drivers to incorrectly assume that the PHY does not have an
>> IRQ associated with it or install an interrupt handler for the
>> PHY.
>>
>> Simplify the code setting irq and set the phy->irq at the same
>> time so that the case if mdio->irq is not NULL is easier to read.
>>
>> This fixes the issue:
>>   net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI
>>
>> to the correct:
>>   net eth0: attached PHY 1 (IRQ 416) to driver Micrel KSZ8041RNLI
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/of/of_mdio.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>> index 875b7b6..7b3e7b0 100644
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
>> @@ -44,7 +44,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>>   {
>>   	struct phy_device *phy;
>>   	bool is_c45;
>> -	int rc, prev_irq;
>> +	int rc;
>>   	u32 max_speed = 0;
>>
>>   	is_c45 = of_device_is_compatible(child,
>> @@ -55,11 +55,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>>   		return 1;
>>
>>   	if (mdio->irq) {
>> -		prev_irq = mdio->irq[addr];
>> -		mdio->irq[addr] =
>> -			irq_of_parse_and_map(child, 0);
>> -		if (!mdio->irq[addr])
>> -			mdio->irq[addr] = prev_irq;
>
> I cannot for the life for me remeber why the code was structured that
> way. Your change is better.
>
>> +		rc = irq_of_parse_and_map(child, 0);
>> +		if (rc > 0) {
>> +			mdio->irq[addr] = rc;
>> +			phy->irq = rc;
>> +		}
>>   	}
>
> The outer if is merely protecting against no irq array being allocated
> for the bus. Would not the following be better:
>
> 	rc = irq_of_parse_and_map(child, 0);
> 	if (rc > 0) {
> 		phy->irq = rc;
> 		if (mdio->irq)
> 			mdio->irq[addr] = rc;
> 	}

Thanks, that makes sense, although we've both failed to work
out if mdio->irq is set, and rc <= 0 case, so:

  	rc = irq_of_parse_and_map(child, 0);
  	if (rc > 0) {
  		phy->irq = rc;
  		if (mdio->irq)
  			mdio->irq[addr] = rc;
  	} else {
		if (mdio->irq)
			phy->irq = mdio->irq[addr];
	}

I think that covers all cases. This does rely on mdio->irq
being initialised to PHY_POLL if allocated.

Once this is in, it may be easier then to not allocate
mdio->irq for the OF case by default unless the driver
registering the PHY knows it has the interrupt number(s)
for the PHY already.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] of_mdio: fix phy interrupt passing

[Grant Likely]

On Tue, 18 Feb 2014 09:40:39 +0000, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 18/02/14 09:30, Grant Likely wrote:
> > On Mon, 17 Feb 2014 16:29:40 +0000, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> >> The of_mdiobus_register_phy() is not setting phy->irq this causing
> >> some drivers to incorrectly assume that the PHY does not have an
> >> IRQ associated with it or install an interrupt handler for the
> >> PHY.
> >>
> >> Simplify the code setting irq and set the phy->irq at the same
> >> time so that the case if mdio->irq is not NULL is easier to read.
> >>
> >> This fixes the issue:
> >>   net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI
> >>
> >> to the correct:
> >>   net eth0: attached PHY 1 (IRQ 416) to driver Micrel KSZ8041RNLI
> >>
> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >> ---
> >>   drivers/of/of_mdio.c | 12 ++++++------
> >>   1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> >> index 875b7b6..7b3e7b0 100644
> >> --- a/drivers/of/of_mdio.c
> >> +++ b/drivers/of/of_mdio.c
> >> @@ -44,7 +44,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
> >>   {
> >>   	struct phy_device *phy;
> >>   	bool is_c45;
> >> -	int rc, prev_irq;
> >> +	int rc;
> >>   	u32 max_speed = 0;
> >>
> >>   	is_c45 = of_device_is_compatible(child,
> >> @@ -55,11 +55,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
> >>   		return 1;
> >>
> >>   	if (mdio->irq) {
> >> -		prev_irq = mdio->irq[addr];
> >> -		mdio->irq[addr] =
> >> -			irq_of_parse_and_map(child, 0);
> >> -		if (!mdio->irq[addr])
> >> -			mdio->irq[addr] = prev_irq;
> >
> > I cannot for the life for me remeber why the code was structured that
> > way. Your change is better.
> >
> >> +		rc = irq_of_parse_and_map(child, 0);
> >> +		if (rc > 0) {
> >> +			mdio->irq[addr] = rc;
> >> +			phy->irq = rc;
> >> +		}
> >>   	}
> >
> > The outer if is merely protecting against no irq array being allocated
> > for the bus. Would not the following be better:
> >
> > 	rc = irq_of_parse_and_map(child, 0);
> > 	if (rc > 0) {
> > 		phy->irq = rc;
> > 		if (mdio->irq)
> > 			mdio->irq[addr] = rc;
> > 	}
> 
> Thanks, that makes sense, although we've both failed to work
> out if mdio->irq is set, and rc <= 0 case, so:
> 
>   	rc = irq_of_parse_and_map(child, 0);
>   	if (rc > 0) {
>   		phy->irq = rc;
>   		if (mdio->irq)
>   			mdio->irq[addr] = rc;
>   	} else {
> 		if (mdio->irq)
> 			phy->irq = mdio->irq[addr];
> 	}

Is this actually a valid thing to do? I think the only time mdio->irq[]
is non-zero is when it is set to PHY_POLL. Is it valid to set phy->irq
to PHY_POLL? I didn't think it was.

g.

> 
> I think that covers all cases. This does rely on mdio->irq
> being initialised to PHY_POLL if allocated.
> 
> Once this is in, it may be easier then to not allocate
> mdio->irq for the OF case by default unless the driver
> registering the PHY knows it has the interrupt number(s)
> for the PHY already.
> 
> -- 
> Ben Dooks				http://www.codethink.co.uk/
> Senior Engineer				Codethink - Providing Genius

Re: [PATCH] of_mdio: fix phy interrupt passing

[Sergei Shtylyov]

Hello.

On 02/18/2014 08:02 PM, Grant Likely wrote:

>>> On Mon, 17 Feb 2014 16:29:40 +0000, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>>>> The of_mdiobus_register_phy() is not setting phy->irq this causing
>>>> some drivers to incorrectly assume that the PHY does not have an
>>>> IRQ associated with it or install an interrupt handler for the
>>>> PHY.

>>>> Simplify the code setting irq and set the phy->irq at the same
>>>> time so that the case if mdio->irq is not NULL is easier to read.

>>>> This fixes the issue:
>>>>    net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI

>>>> to the correct:
>>>>    net eth0: attached PHY 1 (IRQ 416) to driver Micrel KSZ8041RNLI

>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>> ---
>>>>    drivers/of/of_mdio.c | 12 ++++++------
>>>>    1 file changed, 6 insertions(+), 6 deletions(-)

>>>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>>>> index 875b7b6..7b3e7b0 100644
>>>> --- a/drivers/of/of_mdio.c
>>>> +++ b/drivers/of/of_mdio.c
>>>> @@ -44,7 +44,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>>>>    {
>>>>    	struct phy_device *phy;
>>>>    	bool is_c45;
>>>> -	int rc, prev_irq;
>>>> +	int rc;
>>>>    	u32 max_speed = 0;
>>>>
>>>>    	is_c45 = of_device_is_compatible(child,
>>>> @@ -55,11 +55,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>>>>    		return 1;
>>>>
>>>>    	if (mdio->irq) {
>>>> -		prev_irq = mdio->irq[addr];
>>>> -		mdio->irq[addr] =
>>>> -			irq_of_parse_and_map(child, 0);
>>>> -		if (!mdio->irq[addr])
>>>> -			mdio->irq[addr] = prev_irq;

>>> I cannot for the life for me remeber why the code was structured that
>>> way. Your change is better.

>>>> +		rc = irq_of_parse_and_map(child, 0);
>>>> +		if (rc > 0) {
>>>> +			mdio->irq[addr] = rc;
>>>> +			phy->irq = rc;
>>>> +		}
>>>>    	}

>>> The outer if is merely protecting against no irq array being allocated
>>> for the bus. Would not the following be better:

>>> 	rc = irq_of_parse_and_map(child, 0);
>>> 	if (rc > 0) {
>>> 		phy->irq = rc;
>>> 		if (mdio->irq)
>>> 			mdio->irq[addr] = rc;
>>> 	}

>> Thanks, that makes sense, although we've both failed to work
>> out if mdio->irq is set, and rc <= 0 case, so:

>>    	rc = irq_of_parse_and_map(child, 0);
>>    	if (rc > 0) {
>>    		phy->irq = rc;
>>    		if (mdio->irq)
>>    			mdio->irq[addr] = rc;
>>    	} else {
>> 		if (mdio->irq)
>> 			phy->irq = mdio->irq[addr];
>> 	}

> Is this actually a valid thing to do? I think the only time mdio->irq[]
> is non-zero is when it is set to PHY_POLL. Is it valid to set phy->irq
> to PHY_POLL? I didn't think it was.

    It is valid, AFAIK.

> g.

>> --
>> Ben Dooks				http://www.codethink.co.uk/
>> Senior Engineer				Codethink - Providing Genius

WBR, Sergei

Re: [PATCH] of_mdio: fix phy interrupt passing

[Sergei Shtylyov]

Hello.

On 02/17/2014 08:48 PM, Florian Fainelli wrote:

>>>> The of_mdiobus_register_phy() is not setting phy->irq this causing
>>>> some drivers to incorrectly assume that the PHY does not have an
>>>> IRQ associated with it or install an interrupt handler for the
>>>> PHY.

>>>> Simplify the code setting irq and set the phy->irq at the same
>>>> time so that the case if mdio->irq is not NULL is easier to read.

>>> The real bug fix, which is not properly explained here, is that
>>> irq_of_parse_and_map() should return values > 0 when the interrupt is
>>> valid, so this makes me wonder why we are not propagating the return
>>> value from irq_of_parse_and_map() in case the call to
>>> of_irq_parse_one() does return something non-zero?

>> No, the first issue is phy->dev never gets set, which causes the
>> issue. The cleanup was added as it seemed easier to put it in with
>> this.

> Ok, that really needs to be mentioned in the commit message, even
> being quite familiar (and possibly dumb too) with the code, I could
> not figure this out by reading your patch.

    Yes, the main issue was that phy->irq wasn't overridden from the value set 
by phy_device_create().

>> I think phy->irq is already initialised to PHY_POLL and thus there
>> is no need to set phy->irq if the irq_of_parse_and_map() fails.

> That is correct, the reason why I introduced 7d97637 ("net: of_mdio:
> do not overwrite PHY interrupt configuration") was that you are also
> allowed to change the irq type before calling into
> of_mdiobus_register(),

    Not really: of_mdiobus_register() init's all of mdio->irq[] with IRQ_POLL, 
thus wiping out all of the previous initialization, so I don't know what you 
did achieve with the aforementioned commit.

> so we want to preserve other irq values being
> set here, such as PHY_IGNORE_INTERRUPT. Your patch does take care of
> that since it only overrides the irq in case we could parse it.

    And it should have stayed that way. The latter change was unnecessary.

WBR, Sergei