Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-03-11 Thread Michal Simek
Hi,

On 09. 03. 19 17:19, Andrew Lunn wrote:
>> Related to this, I have a query on how the DT node for gmii2rgmii should 
>> look.
>> One of the users of gmii2rgmii is Cadence macb driver. In Xilinx tree, we use
>> this piece of code to register this mdiobus:
>> + mdio_np = of_get_child_by_name(np, "mdio");
>> + if (mdio_np) {
>> + of_node_put(mdio_np);
>> + err = of_mdiobus_register(bp->mii_bus, mdio_np);
>> + if (err)
>> + goto err_out_unregister_bus;
>>
>> And the DT node looks like this:
>> ethernet {
>> phy-mode = "gmii";
>> phy-handle = <>;
>>
>> mdio {
>> extphy {
>> reg = ;
>> };
>> gmii_to_rgmii{
>> compatible = "xlnx,gmii-to-rgmii-1.0";
>> phy-handle = <>;
>> reg = ;
>> };
>> };
>> };
> 
> Hi Harini
> 
> You have this setup:
>   
> MAC <==> GMII2RGMII <==> RGMII_PHY
> 
> So you want the MAC phy-handle to point to the gmii_to_rgmii 'PHY'.

That means that MAC should talk to GMII2RGMII bridge as normall talks to
RGMII_PHY and then GMII2RGMII bridge should talk to RGMII_PHY.

Anyway good to read it because that's exactly how we have done it in u-boot.

Thanks,
Michal


Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-03-09 Thread Andrew Lunn
> Related to this, I have a query on how the DT node for gmii2rgmii should look.
> One of the users of gmii2rgmii is Cadence macb driver. In Xilinx tree, we use
> this piece of code to register this mdiobus:
> + mdio_np = of_get_child_by_name(np, "mdio");
> + if (mdio_np) {
> + of_node_put(mdio_np);
> + err = of_mdiobus_register(bp->mii_bus, mdio_np);
> + if (err)
> + goto err_out_unregister_bus;
> 
> And the DT node looks like this:
> ethernet {
> phy-mode = "gmii";
> phy-handle = <>;
> 
> mdio {
> extphy {
> reg = ;
> };
> gmii_to_rgmii{
> compatible = "xlnx,gmii-to-rgmii-1.0";
> phy-handle = <>;
> reg = ;
> };
> };
> };

Hi Harini

You have this setup:
  
MAC <==> GMII2RGMII <==> RGMII_PHY

So you want the MAC phy-handle to point to the gmii_to_rgmii 'PHY'.

Feel free to submit a patch extending
Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt to include
a MAC node, etc.

   Andrew


Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-03-09 Thread Harini Katakam
Hi Andrew,
On Thu, Feb 28, 2019 at 1:03 PM Harini Katakam  wrote:
>
> Hi,
> On Wed, Feb 27, 2019 at 2:35 PM Harini Katakam  wrote:
> >
> > Hi Andrew, Paul,
> >
> > On Wed, Feb 27, 2019 at 2:15 PM Michal Simek  
> > wrote:
> > >
> > > On 21. 02. 19 12:03, Michal Simek wrote:
> > > > On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> > > >> Hi,
> > > >>
> > > >> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> > > >>> Hi,
> > > >>>
> > > >>> On 19. 02. 19 18:25, Andrew Lunn wrote:
> 
> > > >> Understood. I think we need to start a discussion about how the general
> > > >> design of this driver can be improved.
> > > >>
> > > >> In particular, I wonder if it could work better to make this driver a
> > > >> PHY driver that just redirects all its ops to the actual PHY driver,
> > > >> except for read_status where it should also add some code.
> >
> > Thanks, I'm looking into this option and also a way to expose the correct
> > interface mode setting as you mentioned below. I'll get back before the
> > end of the week. Please do let me know if you have any further suggestions.
> >
> This IP does not have a PHY identifier or status register that can be accessed
> from the phy framework. We could discuss with our design team to add these
> in the future. But that would take sometime and this version should be still 
> be
> supported. Also, if this IP has a PHY driver, then two phy drivers would have
> to be probed which are connected in a serial manner and I believe I'll have to
> update the framework to support that. Could you please let me know if you have
> any inputs on this?
> OR since this is just a bridge IP, is it acceptable to address the error 
> cases?
> -> Module loading/unloading
> -> Spinlocks for protection
> -> Correct phy mode information to the driver.
> -> Any other concerns
> I could do a respin of this patch after addressing Andrew's comments:
> https://patchwork.kernel.org/patch/9290231/

Related to this, I have a query on how the DT node for gmii2rgmii should look.
One of the users of gmii2rgmii is Cadence macb driver. In Xilinx tree, we use
this piece of code to register this mdiobus:
+ mdio_np = of_get_child_by_name(np, "mdio");
+ if (mdio_np) {
+ of_node_put(mdio_np);
+ err = of_mdiobus_register(bp->mii_bus, mdio_np);
+ if (err)
+ goto err_out_unregister_bus;

And the DT node looks like this:
ethernet {
phy-mode = "gmii";
phy-handle = <>;

mdio {
extphy {
reg = ;
};
gmii_to_rgmii{
compatible = "xlnx,gmii-to-rgmii-1.0";
phy-handle = <>;
reg = ;
};
};
};

Could you please clarify if phy-handle in ethernet should point to
external PHY or gmii2rgmii?

Regards,
Harini


Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-27 Thread Harini Katakam
Hi,
On Wed, Feb 27, 2019 at 2:35 PM Harini Katakam  wrote:
>
> Hi Andrew, Paul,
>
> On Wed, Feb 27, 2019 at 2:15 PM Michal Simek  wrote:
> >
> > On 21. 02. 19 12:03, Michal Simek wrote:
> > > On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> > >> Hi,
> > >>
> > >> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> > >>> Hi,
> > >>>
> > >>> On 19. 02. 19 18:25, Andrew Lunn wrote:

> > >> Understood. I think we need to start a discussion about how the general
> > >> design of this driver can be improved.
> > >>
> > >> In particular, I wonder if it could work better to make this driver a
> > >> PHY driver that just redirects all its ops to the actual PHY driver,
> > >> except for read_status where it should also add some code.
>
> Thanks, I'm looking into this option and also a way to expose the correct
> interface mode setting as you mentioned below. I'll get back before the
> end of the week. Please do let me know if you have any further suggestions.
>
This IP does not have a PHY identifier or status register that can be accessed
from the phy framework. We could discuss with our design team to add these
in the future. But that would take sometime and this version should be still be
supported. Also, if this IP has a PHY driver, then two phy drivers would have
to be probed which are connected in a serial manner and I believe I'll have to
update the framework to support that. Could you please let me know if you have
any inputs on this?
OR since this is just a bridge IP, is it acceptable to address the error cases?
-> Module loading/unloading
-> Spinlocks for protection
-> Correct phy mode information to the driver.
-> Any other concerns
I could do a respin of this patch after addressing Andrew's comments:
https://patchwork.kernel.org/patch/9290231/

Regards,
Harini


Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-27 Thread Harini Katakam
Hi Andrew, Paul,

On Wed, Feb 27, 2019 at 2:15 PM Michal Simek  wrote:
>
> On 21. 02. 19 12:03, Michal Simek wrote:
> > On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> >> Hi,
> >>
> >> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> >>> Hi,
> >>>
> >>> On 19. 02. 19 18:25, Andrew Lunn wrote:
> > Thanks for the suggestion! So I had a closer look at that driver to try
> > and see what could go wrong and it looks like I found a few things
> > there.
> 
>  Hi Paul
> 
>  Yes, this driver has issues. If i remember correctly, it got merged
>  while i was on vacation. I pointed out a few issues, but the authors
>  never responded. Feel free to fix it up.
> >>>

Sorry for this - I've synced up with the author and got the comments from the
time this driver was upstreamed. I'll try to address those and Paul's
suggestions
going forward.

> >>> Will be good to know who was that person.
> >>>
> >>> I can't do much this week with this because responsible person for this
> >>> driver is out of office this week. That's why please give us some time
> >>> to get back to this.
> >>
> >> Understood. I think we need to start a discussion about how the general
> >> design of this driver can be improved.
> >>
> >> In particular, I wonder if it could work better to make this driver a
> >> PHY driver that just redirects all its ops to the actual PHY driver,
> >> except for read_status where it should also add some code.

Thanks, I'm looking into this option and also a way to expose the correct
interface mode setting as you mentioned below. I'll get back before the
end of the week. Please do let me know if you have any further suggestions.

Regards,
Harini

> >
> > I didn't take a look at Linux driver but it should work in a way that it
> > checks description (more below) and then wait for attached phy to do its
> > work and on the way back just setup this bridge based on that.
> >
> >> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
> >> this way. Currently, the PHY mode has to be set to GMII for the MAC to
> >> be configured correctly, but the PHY also gets this information while
> >> it should be told that RGMII is in use. This doesn't seem to play a big
> >> role in PHY configuration though, but it's still inadequate.
> >>
> >> What do you think?



Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-27 Thread Michal Simek
On 21. 02. 19 12:03, Michal Simek wrote:
> On 21. 02. 19 11:24, Paul Kocialkowski wrote:
>> Hi,
>>
>> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
>>> Hi,
>>>
>>> On 19. 02. 19 18:25, Andrew Lunn wrote:
> Thanks for the suggestion! So I had a closer look at that driver to try
> and see what could go wrong and it looks like I found a few things
> there.

 Hi Paul

 Yes, this driver has issues. If i remember correctly, it got merged
 while i was on vacation. I pointed out a few issues, but the authors
 never responded. Feel free to fix it up.
>>>
>>> Will be good to know who was that person.
>>>
>>> I can't do much this week with this because responsible person for this
>>> driver is out of office this week. That's why please give us some time
>>> to get back to this.
>>
>> Understood. I think we need to start a discussion about how the general
>> design of this driver can be improved.
>>
>> In particular, I wonder if it could work better to make this driver a
>> PHY driver that just redirects all its ops to the actual PHY driver,
>> except for read_status where it should also add some code.
> 
> I didn't take a look at Linux driver but it should work in a way that it
> checks description (more below) and then wait for attached phy to do its
> work and on the way back just setup this bridge based on that.
> 
>> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
>> this way. Currently, the PHY mode has to be set to GMII for the MAC to
>> be configured correctly, but the PHY also gets this information while
>> it should be told that RGMII is in use. This doesn't seem to play a big
>> role in PHY configuration though, but it's still inadequate.
>>
>> What do you think?
> 
> I stop the driver to be applied to u-boot because exactly this
> gmii/rgmii configuration. There is a need that mac is configured to gmii
> and this needs to be checked but to phy rgmii needs to be exposed.
> I was trying to find out hardware design and board where I can do some
> experiments but didn't find out. That's why I need to wait for
> colleagues to point me to that.

Harini: Can you please respond this thread?

Thanks,
Michal




Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-21 Thread Michal Simek
On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> Hi,
> 
> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
>> Hi,
>>
>> On 19. 02. 19 18:25, Andrew Lunn wrote:
 Thanks for the suggestion! So I had a closer look at that driver to try
 and see what could go wrong and it looks like I found a few things
 there.
>>>
>>> Hi Paul
>>>
>>> Yes, this driver has issues. If i remember correctly, it got merged
>>> while i was on vacation. I pointed out a few issues, but the authors
>>> never responded. Feel free to fix it up.
>>
>> Will be good to know who was that person.
>>
>> I can't do much this week with this because responsible person for this
>> driver is out of office this week. That's why please give us some time
>> to get back to this.
> 
> Understood. I think we need to start a discussion about how the general
> design of this driver can be improved.
> 
> In particular, I wonder if it could work better to make this driver a
> PHY driver that just redirects all its ops to the actual PHY driver,
> except for read_status where it should also add some code.

I didn't take a look at Linux driver but it should work in a way that it
checks description (more below) and then wait for attached phy to do its
work and on the way back just setup this bridge based on that.

> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
> this way. Currently, the PHY mode has to be set to GMII for the MAC to
> be configured correctly, but the PHY also gets this information while
> it should be told that RGMII is in use. This doesn't seem to play a big
> role in PHY configuration though, but it's still inadequate.
> 
> What do you think?

I stop the driver to be applied to u-boot because exactly this
gmii/rgmii configuration. There is a need that mac is configured to gmii
and this needs to be checked but to phy rgmii needs to be exposed.
I was trying to find out hardware design and board where I can do some
experiments but didn't find out. That's why I need to wait for
colleagues to point me to that.

Thanks,
Michal



Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-21 Thread Paul Kocialkowski
Hi,

On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> Hi,
> 
> On 19. 02. 19 18:25, Andrew Lunn wrote:
> > > Thanks for the suggestion! So I had a closer look at that driver to try
> > > and see what could go wrong and it looks like I found a few things
> > > there.
> > 
> > Hi Paul
> > 
> > Yes, this driver has issues. If i remember correctly, it got merged
> > while i was on vacation. I pointed out a few issues, but the authors
> > never responded. Feel free to fix it up.
> 
> Will be good to know who was that person.
> 
> I can't do much this week with this because responsible person for this
> driver is out of office this week. That's why please give us some time
> to get back to this.

Understood. I think we need to start a discussion about how the general
design of this driver can be improved.

In particular, I wonder if it could work better to make this driver a
PHY driver that just redirects all its ops to the actual PHY driver,
except for read_status where it should also add some code.

Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
this way. Currently, the PHY mode has to be set to GMII for the MAC to
be configured correctly, but the PHY also gets this information while
it should be told that RGMII is in use. This doesn't seem to play a big
role in PHY configuration though, but it's still inadequate.

What do you think?

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com



Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-19 Thread Michal Simek
Hi,

On 19. 02. 19 18:25, Andrew Lunn wrote:
>> Thanks for the suggestion! So I had a closer look at that driver to try
>> and see what could go wrong and it looks like I found a few things
>> there.
> 
> Hi Paul
> 
> Yes, this driver has issues. If i remember correctly, it got merged
> while i was on vacation. I pointed out a few issues, but the authors
> never responded. Feel free to fix it up.

Will be good to know who was that person.

I can't do much this week with this because responsible person for this
driver is out of office this week. That's why please give us some time
to get back to this.

Thanks,
Michal





Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-19 Thread Andrew Lunn
> Thanks for the suggestion! So I had a closer look at that driver to try
> and see what could go wrong and it looks like I found a few things
> there.

Hi Paul

Yes, this driver has issues. If i remember correctly, it got merged
while i was on vacation. I pointed out a few issues, but the authors
never responded. Feel free to fix it up.

  Andrew


Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-19 Thread Paul Kocialkowski
Hi,

On Fri, 2019-02-15 at 10:53 -0800, Florian Fainelli wrote:
> On 2/15/19 10:34 AM, Paul Kocialkowski wrote:
> > As I was mentionning to Andrew in the initial submission of this patch,
> > this driver is a bit unusual since it represents a GMII to RGMII
> > bridge, so it's not actually a PHY driver on its own -- it just sticks
> > itself in between the actual PHY and the MAC.
> 
> Yes, my bad, you should still consider checking priv->phy_drv though, if
> someone unbinds the PHY driver on either side, you are toast.

Thanks for the suggestion! So I had a closer look at that driver to try
and see what could go wrong and it looks like I found a few things
there.

First, we have:
> priv->phy_dev->priv = priv;

Which basically overwrites that target PHY driver's priv with the
gmii2rgmii driver's. It looks like most PHY drivers don't use their
priv data so much so it kind of works in practice. But that's still a
receipe for disaster. I don't really see an immediate easy fix for that
one.

It might help to do things the other way round and bind the gmii2rgmii
PHY to the MAC, which itself would bind the actual PHY. That way we can
just have the gmii2rgmii redirect all ops to the actual PHY, except for
read_status. Maybe there was a reason I'm not seeing for doing things
the way they are done now though?

Then, it looks like there is no way for the gmii2rgmii driver to know
whether the PHY driver is still alive. It gets a pointer to the initial
priv->phy_dev->drv and then overwrites it. So when the target driver is
removed, the pointer will still be alive. Perhaps the memory backing it
will have been freed too.

How realistic does this scneario sound? I guess there are not many
cases where the PHY driver will be unregistered once it was picked up
by the gmii2rgmii driver, but I'm pretty new to the subsystem.

Cheers,

Paul

> > > >  drivers/net/phy/xilinx_gmii2rgmii.c | 5 -
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c 
> > > > b/drivers/net/phy/xilinx_gmii2rgmii.c
> > > > index 74a8782313cf..bd6084e315de 100644
> > > > --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> > > > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> > > > @@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct 
> > > > phy_device *phydev)
> > > > u16 val = 0;
> > > > int err;
> > > >  
> > > > -   err = priv->phy_drv->read_status(phydev);
> > > > +   if (priv->phy_drv->read_status)
> > > > +   err = priv->phy_drv->read_status(phydev);
> > > > +   else
> > > > +   err = genphy_read_status(phydev);
> > > > if (err < 0)
> > > > return err;
> > > >  
> > > > 
> 
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com



Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-15 Thread Florian Fainelli
On 2/15/19 10:34 AM, Paul Kocialkowski wrote:
> Hi,
> 
> On Fri, 2019-02-15 at 09:38 -0800, Florian Fainelli wrote:
>> On 2/15/19 8:32 AM, Paul Kocialkowski wrote:
>>> Some PHY drivers like the generic one do not provide a read_status
>>> callback on their own but rely on genphy_read_status being called
>>> directly.
>>>
>>> With the current code, this results in a NULL function pointer call.
>>> Call genphy_read_status instead when there is no specific callback.
>>>
>>> Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
>>> Signed-off-by: Paul Kocialkowski 
>>> ---
>>> Added Fixes tag and net label for resend.
>>
>> You would want to use phy_read_status() which encapsulates that check as
>> well as checks that the phy_drv pointer is not NULL.
> 
> Well, this driver is a bit different and our priv->phy_drv != phydev-
>> drv, so we can't use the helper directly. I should probably have
> mentionned it in the commit message, sorry!
> 
> As I was mentionning to Andrew in the initial submission of this patch,
> this driver is a bit unusual since it represents a GMII to RGMII
> bridge, so it's not actually a PHY driver on its own -- it just sticks
> itself in between the actual PHY and the MAC.

Yes, my bad, you should still consider checking priv->phy_drv though, if
someone unbinds the PHY driver on either side, you are toast.

> 
> Cheers and thanks for the review,
> 
> Paul
> 
>>>  drivers/net/phy/xilinx_gmii2rgmii.c | 5 -
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c 
>>> b/drivers/net/phy/xilinx_gmii2rgmii.c
>>> index 74a8782313cf..bd6084e315de 100644
>>> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
>>> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
>>> @@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct phy_device 
>>> *phydev)
>>> u16 val = 0;
>>> int err;
>>>  
>>> -   err = priv->phy_drv->read_status(phydev);
>>> +   if (priv->phy_drv->read_status)
>>> +   err = priv->phy_drv->read_status(phydev);
>>> +   else
>>> +   err = genphy_read_status(phydev);
>>> if (err < 0)
>>> return err;
>>>  
>>>
>>
>>


-- 
Florian


Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-15 Thread Paul Kocialkowski
Hi,

On Fri, 2019-02-15 at 09:38 -0800, Florian Fainelli wrote:
> On 2/15/19 8:32 AM, Paul Kocialkowski wrote:
> > Some PHY drivers like the generic one do not provide a read_status
> > callback on their own but rely on genphy_read_status being called
> > directly.
> > 
> > With the current code, this results in a NULL function pointer call.
> > Call genphy_read_status instead when there is no specific callback.
> > 
> > Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
> > Signed-off-by: Paul Kocialkowski 
> > ---
> > Added Fixes tag and net label for resend.
> 
> You would want to use phy_read_status() which encapsulates that check as
> well as checks that the phy_drv pointer is not NULL.

Well, this driver is a bit different and our priv->phy_drv != phydev-
>drv, so we can't use the helper directly. I should probably have
mentionned it in the commit message, sorry!

As I was mentionning to Andrew in the initial submission of this patch,
this driver is a bit unusual since it represents a GMII to RGMII
bridge, so it's not actually a PHY driver on its own -- it just sticks
itself in between the actual PHY and the MAC.

Cheers and thanks for the review,

Paul

> >  drivers/net/phy/xilinx_gmii2rgmii.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c 
> > b/drivers/net/phy/xilinx_gmii2rgmii.c
> > index 74a8782313cf..bd6084e315de 100644
> > --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> > @@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct phy_device 
> > *phydev)
> > u16 val = 0;
> > int err;
> >  
> > -   err = priv->phy_drv->read_status(phydev);
> > +   if (priv->phy_drv->read_status)
> > +   err = priv->phy_drv->read_status(phydev);
> > +   else
> > +   err = genphy_read_status(phydev);
> > if (err < 0)
> > return err;
> >  
> > 
> 
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com



Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-15 Thread Florian Fainelli
On 2/15/19 8:32 AM, Paul Kocialkowski wrote:
> Some PHY drivers like the generic one do not provide a read_status
> callback on their own but rely on genphy_read_status being called
> directly.
> 
> With the current code, this results in a NULL function pointer call.
> Call genphy_read_status instead when there is no specific callback.
> 
> Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
> Signed-off-by: Paul Kocialkowski 
> ---
> Added Fixes tag and net label for resend.

You would want to use phy_read_status() which encapsulates that check as
well as checks that the phy_drv pointer is not NULL.

> 
>  drivers/net/phy/xilinx_gmii2rgmii.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c 
> b/drivers/net/phy/xilinx_gmii2rgmii.c
> index 74a8782313cf..bd6084e315de 100644
> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> @@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct phy_device 
> *phydev)
>   u16 val = 0;
>   int err;
>  
> - err = priv->phy_drv->read_status(phydev);
> + if (priv->phy_drv->read_status)
> + err = priv->phy_drv->read_status(phydev);
> + else
> + err = genphy_read_status(phydev);
>   if (err < 0)
>   return err;
>  
> 


-- 
Florian


Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

2019-02-15 Thread Andrew Lunn
On Fri, Feb 15, 2019 at 05:32:20PM +0100, Paul Kocialkowski wrote:
> Some PHY drivers like the generic one do not provide a read_status
> callback on their own but rely on genphy_read_status being called
> directly.
> 
> With the current code, this results in a NULL function pointer call.
> Call genphy_read_status instead when there is no specific callback.
> 
> Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
> Signed-off-by: Paul Kocialkowski 

Reviewed-by: Andrew Lunn 

Andrew