[PATCH] net: phylink: set the autoneg state in phylink_phy_change

[PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Ioana Ciornei]

The phy_state field of phylink should carry only valid information
especially when this can be passed to the .mac_config callback.
Update the an_enabled field with the autoneg state in the
phylink_phy_change function.

Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phylink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 5d0af041b8f9..dd1feb7b5472 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -688,6 +688,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up,
 		pl->phy_state.pause |= MLO_PAUSE_ASYM;
 	pl->phy_state.interface = phydev->interface;
 	pl->phy_state.link = up;
+	pl->phy_state.an_enabled = phydev->autoneg;
 	mutex_unlock(&pl->state_mutex);
 
 	phylink_run_resolve(pl);
-- 
1.9.1

RE: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Ioana Ciornei]

> Subject: [PATCH] net: phylink: set the autoneg state in phylink_phy_change
> 
> The phy_state field of phylink should carry only valid information especially
> when this can be passed to the .mac_config callback.
> Update the an_enabled field with the autoneg state in the phylink_phy_change
> function.
> 
> Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  drivers/net/phy/phylink.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index
> 5d0af041b8f9..dd1feb7b5472 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -688,6 +688,7 @@ static void phylink_phy_change(struct phy_device
> *phydev, bool up,
>  		pl->phy_state.pause |= MLO_PAUSE_ASYM;
>  	pl->phy_state.interface = phydev->interface;
>  	pl->phy_state.link = up;
> +	pl->phy_state.an_enabled = phydev->autoneg;
>  	mutex_unlock(&pl->state_mutex);
> 
>  	phylink_run_resolve(pl);
> --
> 1.9.1

Unfortunately, I am not able to find this patch on any netdev list archive.

One other interesting thing that I noticed is that both netdev and the linux-kernel list received the last message around '12 Jun 2019 21:15'. [1][2]
I am just trying to see if there is a problem on my side or something else.

--
Ioana

[1] https://marc.info/?l=linux-netdev&m=156037411432212&w=2
[2] https://marc.info/?l=linux-kernel&m=156037415432226&w=2

Re: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Russell King - ARM Linux admin]

On Thu, Jun 13, 2019 at 09:37:51AM +0300, Ioana Ciornei wrote:
> The phy_state field of phylink should carry only valid information
> especially when this can be passed to the .mac_config callback.
> Update the an_enabled field with the autoneg state in the
> phylink_phy_change function.

an_enabled is meaningless to mac_config for PHY mode.  Why do you think
this is necessary?

> 
> Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  drivers/net/phy/phylink.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 5d0af041b8f9..dd1feb7b5472 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -688,6 +688,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up,
>  		pl->phy_state.pause |= MLO_PAUSE_ASYM;
>  	pl->phy_state.interface = phydev->interface;
>  	pl->phy_state.link = up;
> +	pl->phy_state.an_enabled = phydev->autoneg;
>  	mutex_unlock(&pl->state_mutex);
>  
>  	phylink_run_resolve(pl);
> -- 
> 1.9.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Ioana Ciornei]

> Subject: Re: [PATCH] net: phylink: set the autoneg state in
> phylink_phy_change
> 
> On Thu, Jun 13, 2019 at 09:37:51AM +0300, Ioana Ciornei wrote:
> > The phy_state field of phylink should carry only valid information
> > especially when this can be passed to the .mac_config callback.
> > Update the an_enabled field with the autoneg state in the
> > phylink_phy_change function.
> 
> an_enabled is meaningless to mac_config for PHY mode.  Why do you think
> this is necessary?

Well, it's not necessarily used in PHY mode but, from my opinion, it should be set to the correct value nonetheless.

Just to give you more context, I am working on adding phylink support on NXP's DPAA2 platforms where any interaction between the PHY management layer and the Ethernet devices is made through a firmware.
When the .mac_config callback is invoked, the driver communicates the new configuration to the firmware so that the corresponding net_device can see the correct info.
In this case, the an_enabled field is not used for other purpose than to inform the net_device of the current configuration and nothing more.

--
Ioana


> 
> >
> > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  drivers/net/phy/phylink.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 5d0af041b8f9..dd1feb7b5472 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -688,6 +688,7 @@ static void phylink_phy_change(struct phy_device
> *phydev, bool up,
> >  		pl->phy_state.pause |= MLO_PAUSE_ASYM;
> >  	pl->phy_state.interface = phydev->interface;
> >  	pl->phy_state.link = up;
> > +	pl->phy_state.an_enabled = phydev->autoneg;
> >  	mutex_unlock(&pl->state_mutex);
> >
> >  	phylink_run_resolve(pl);
> > --
> > 1.9.1
> >
> >

Re: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Russell King - ARM Linux admin]

On Thu, Jun 13, 2019 at 08:55:16AM +0000, Ioana Ciornei wrote:
> > Subject: Re: [PATCH] net: phylink: set the autoneg state in
> > phylink_phy_change
> > 
> > On Thu, Jun 13, 2019 at 09:37:51AM +0300, Ioana Ciornei wrote:
> > > The phy_state field of phylink should carry only valid information
> > > especially when this can be passed to the .mac_config callback.
> > > Update the an_enabled field with the autoneg state in the
> > > phylink_phy_change function.
> > 
> > an_enabled is meaningless to mac_config for PHY mode.  Why do you think
> > this is necessary?
> 
> Well, it's not necessarily used in PHY mode but, from my opinion, it should be set to the correct value nonetheless.
> 
> Just to give you more context, I am working on adding phylink support on NXP's DPAA2 platforms where any interaction between the PHY management layer and the Ethernet devices is made through a firmware.
> When the .mac_config callback is invoked, the driver communicates the new configuration to the firmware so that the corresponding net_device can see the correct info.
> In this case, the an_enabled field is not used for other purpose than to inform the net_device of the current configuration and nothing more.

The fields that are applicable depend on the negotiation mode:

- Non-inband (PHY or FIXED): set the speed, duplex and pause h/w
   parameters as per the state's speed, duplex and pause settings.
   Every other state setting should be ignored; they are not defined
   for this mode of operation.

- Inband SGMII: set for inband SGMII reporting of speed and duplex
   h/w parameters.  Set pause mode h/w parameters as per the state's
   pause settings.  Every other state setting should be ignored; they
   are not defined for this mode of operation.

- Inband 802.3z: set for 1G or 2.5G depending on the PHY interface mode.
   If an_enabled is true, allow inband 802.3z to set the duplex h/w
   parameter.  If an_enabled and the MLO_PAUSE_AN bit of the pause
   setting are true, allow 802.3z to set the pause h/w parameter.
   Advertise capabilities depending on the 'advertising' setting.

There's only one case where an_enabled is used, which is 802.3z
negotiation, because the MAC side is responsible for negotiating the
link mode.  In all other cases, the MAC is not responsible for any
autonegotiation.

It is important to stick to the above, which will ensure correct
functioning of your driver - going off and doing your own thing (such
as reading from other fields) is not guaranteed to give good results.

> 
> --
> Ioana
> 
> 
> > 
> > >
> > > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > ---
> > >  drivers/net/phy/phylink.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index 5d0af041b8f9..dd1feb7b5472 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -688,6 +688,7 @@ static void phylink_phy_change(struct phy_device
> > *phydev, bool up,
> > >  		pl->phy_state.pause |= MLO_PAUSE_ASYM;
> > >  	pl->phy_state.interface = phydev->interface;
> > >  	pl->phy_state.link = up;
> > > +	pl->phy_state.an_enabled = phydev->autoneg;
> > >  	mutex_unlock(&pl->state_mutex);
> > >
> > >  	phylink_run_resolve(pl);
> > > --
> > > 1.9.1
> > >
> > >
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Ioana Ciornei]

> Subject: Re: [PATCH] net: phylink: set the autoneg state in
> phylink_phy_change
> 
> On Thu, Jun 13, 2019 at 08:55:16AM +0000, Ioana Ciornei wrote:
> > > Subject: Re: [PATCH] net: phylink: set the autoneg state in
> > > phylink_phy_change
> > >
> > > On Thu, Jun 13, 2019 at 09:37:51AM +0300, Ioana Ciornei wrote:
> > > > The phy_state field of phylink should carry only valid information
> > > > especially when this can be passed to the .mac_config callback.
> > > > Update the an_enabled field with the autoneg state in the
> > > > phylink_phy_change function.
> > >
> > > an_enabled is meaningless to mac_config for PHY mode.  Why do you
> > > think this is necessary?
> >
> > Well, it's not necessarily used in PHY mode but, from my opinion, it should
> be set to the correct value nonetheless.
> >
> > Just to give you more context, I am working on adding phylink support on
> NXP's DPAA2 platforms where any interaction between the PHY
> management layer and the Ethernet devices is made through a firmware.
> > When the .mac_config callback is invoked, the driver communicates the
> new configuration to the firmware so that the corresponding net_device can
> see the correct info.
> > In this case, the an_enabled field is not used for other purpose than to
> inform the net_device of the current configuration and nothing more.
> 
> The fields that are applicable depend on the negotiation mode:
> 
> - Non-inband (PHY or FIXED): set the speed, duplex and pause h/w
>    parameters as per the state's speed, duplex and pause settings.
>    Every other state setting should be ignored; they are not defined
>    for this mode of operation.
> 
> - Inband SGMII: set for inband SGMII reporting of speed and duplex
>    h/w parameters.  Set pause mode h/w parameters as per the state's
>    pause settings.  Every other state setting should be ignored; they
>    are not defined for this mode of operation.
> 
> - Inband 802.3z: set for 1G or 2.5G depending on the PHY interface mode.
>    If an_enabled is true, allow inband 802.3z to set the duplex h/w
>    parameter.  If an_enabled and the MLO_PAUSE_AN bit of the pause
>    setting are true, allow 802.3z to set the pause h/w parameter.
>    Advertise capabilities depending on the 'advertising' setting.
> 
> There's only one case where an_enabled is used, which is 802.3z negotiation,
> because the MAC side is responsible for negotiating the link mode.  In all
> other cases, the MAC is not responsible for any autonegotiation.

It's clear for me that an_enabled is of use for the MAC only when clause 37 auto-negotiation is used.

However,  the DPAA2 software architecture abstracts the MAC and the network interface into 2 separate entities that are managed by two different drivers.
These drivers communicate only through a firmware.

This means that any ethtool issued on a DPAA2 network interface will go directly to the firmware for the latest link information.
When the MAC driver is not capable to inform the firmware of the proper link configuration (eg whether the autoneg is on or not), the ethtool output will not be the correct one.

> 
> It is important to stick to the above, which will ensure correct functioning of
> your driver - going off and doing your own thing (such as reading from other
> fields) is not guaranteed to give good results.
> 

You're right, but unfortunately I am not dealing with a straight-forward architecture. 

--
Ioana

> >
> > --
> > Ioana
> >
> >
> > >
> > > >
> > > > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index 5d0af041b8f9..dd1feb7b5472 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -688,6 +688,7 @@ static void phylink_phy_change(struct
> > > > phy_device
> > > *phydev, bool up,
> > > >  		pl->phy_state.pause |= MLO_PAUSE_ASYM;
> > > >  	pl->phy_state.interface = phydev->interface;
> > > >  	pl->phy_state.link = up;
> > > > +	pl->phy_state.an_enabled = phydev->autoneg;
> > > >  	mutex_unlock(&pl->state_mutex);
> > > >
> > > >  	phylink_run_resolve(pl);
> > > > --
> > > > 1.9.1
> > > >
> > > >
> >
> >
> 

Re: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Russell King - ARM Linux admin]

On Thu, Jun 13, 2019 at 02:32:06PM +0000, Ioana Ciornei wrote:
> 
> > Subject: Re: [PATCH] net: phylink: set the autoneg state in
> > phylink_phy_change
> > 
> > On Thu, Jun 13, 2019 at 08:55:16AM +0000, Ioana Ciornei wrote:
> > > > Subject: Re: [PATCH] net: phylink: set the autoneg state in
> > > > phylink_phy_change
> > > >
> > > > On Thu, Jun 13, 2019 at 09:37:51AM +0300, Ioana Ciornei wrote:
> > > > > The phy_state field of phylink should carry only valid information
> > > > > especially when this can be passed to the .mac_config callback.
> > > > > Update the an_enabled field with the autoneg state in the
> > > > > phylink_phy_change function.
> > > >
> > > > an_enabled is meaningless to mac_config for PHY mode.  Why do you
> > > > think this is necessary?
> > >
> > > Well, it's not necessarily used in PHY mode but, from my opinion, it should
> > be set to the correct value nonetheless.
> > >
> > > Just to give you more context, I am working on adding phylink support on
> > NXP's DPAA2 platforms where any interaction between the PHY
> > management layer and the Ethernet devices is made through a firmware.
> > > When the .mac_config callback is invoked, the driver communicates the
> > new configuration to the firmware so that the corresponding net_device can
> > see the correct info.
> > > In this case, the an_enabled field is not used for other purpose than to
> > inform the net_device of the current configuration and nothing more.
> > 
> > The fields that are applicable depend on the negotiation mode:
> > 
> > - Non-inband (PHY or FIXED): set the speed, duplex and pause h/w
> >    parameters as per the state's speed, duplex and pause settings.
> >    Every other state setting should be ignored; they are not defined
> >    for this mode of operation.
> > 
> > - Inband SGMII: set for inband SGMII reporting of speed and duplex
> >    h/w parameters.  Set pause mode h/w parameters as per the state's
> >    pause settings.  Every other state setting should be ignored; they
> >    are not defined for this mode of operation.
> > 
> > - Inband 802.3z: set for 1G or 2.5G depending on the PHY interface mode.
> >    If an_enabled is true, allow inband 802.3z to set the duplex h/w
> >    parameter.  If an_enabled and the MLO_PAUSE_AN bit of the pause
> >    setting are true, allow 802.3z to set the pause h/w parameter.
> >    Advertise capabilities depending on the 'advertising' setting.
> > 
> > There's only one case where an_enabled is used, which is 802.3z negotiation,
> > because the MAC side is responsible for negotiating the link mode.  In all
> > other cases, the MAC is not responsible for any autonegotiation.
> 
> It's clear for me that an_enabled is of use for the MAC only when clause 37 auto-negotiation is used.
> 
> However,  the DPAA2 software architecture abstracts the MAC and the network interface into 2 separate entities that are managed by two different drivers.
> These drivers communicate only through a firmware.
> 
> This means that any ethtool issued on a DPAA2 network interface will go directly to the firmware for the latest link information.

So you won't be calling phylink_ethtool_ksettings_get(), which means
you won't be returning correct information anyway.

> When the MAC driver is not capable to inform the firmware of the proper link configuration (eg whether the autoneg is on or not), the ethtool output will not be the correct one.

You don't get to know the list of supported link modes, so I don't see
how the ethtool information can be correct.

I'd like to see the patches _before_ I consider accepting your proposed
phylink change.

> > It is important to stick to the above, which will ensure correct functioning of
> > your driver - going off and doing your own thing (such as reading from other
> > fields) is not guaranteed to give good results.
> 
> You're right, but unfortunately I am not dealing with a straight-forward architecture. 

At this point, I think you need to explain why you want to use phylink,
as you seem to be saying that your driver is unable to conform to what
phylink expects due to firmware.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Ioana Ciornei]

> Subject: Re: [PATCH] net: phylink: set the autoneg state in
> phylink_phy_change
> 
> On Thu, Jun 13, 2019 at 02:32:06PM +0000, Ioana Ciornei wrote:
> >
> > > Subject: Re: [PATCH] net: phylink: set the autoneg state in
> > > phylink_phy_change
> > >
> > > On Thu, Jun 13, 2019 at 08:55:16AM +0000, Ioana Ciornei wrote:
> > > > > Subject: Re: [PATCH] net: phylink: set the autoneg state in
> > > > > phylink_phy_change
> > > > >
> > > > > On Thu, Jun 13, 2019 at 09:37:51AM +0300, Ioana Ciornei wrote:
> > > > > > The phy_state field of phylink should carry only valid
> > > > > > information especially when this can be passed to the .mac_config
> callback.
> > > > > > Update the an_enabled field with the autoneg state in the
> > > > > > phylink_phy_change function.
> > > > >
> > > > > an_enabled is meaningless to mac_config for PHY mode.  Why do
> > > > > you think this is necessary?
> > > >
> > > > Well, it's not necessarily used in PHY mode but, from my opinion,
> > > > it should
> > > be set to the correct value nonetheless.
> > > >
> > > > Just to give you more context, I am working on adding phylink
> > > > support on
> > > NXP's DPAA2 platforms where any interaction between the PHY
> > > management layer and the Ethernet devices is made through a firmware.
> > > > When the .mac_config callback is invoked, the driver communicates
> > > > the
> > > new configuration to the firmware so that the corresponding
> > > net_device can see the correct info.
> > > > In this case, the an_enabled field is not used for other purpose
> > > > than to
> > > inform the net_device of the current configuration and nothing more.
> > >
> > > The fields that are applicable depend on the negotiation mode:
> > >
> > > - Non-inband (PHY or FIXED): set the speed, duplex and pause h/w
> > >    parameters as per the state's speed, duplex and pause settings.
> > >    Every other state setting should be ignored; they are not defined
> > >    for this mode of operation.
> > >
> > > - Inband SGMII: set for inband SGMII reporting of speed and duplex
> > >    h/w parameters.  Set pause mode h/w parameters as per the state's
> > >    pause settings.  Every other state setting should be ignored; they
> > >    are not defined for this mode of operation.
> > >
> > > - Inband 802.3z: set for 1G or 2.5G depending on the PHY interface mode.
> > >    If an_enabled is true, allow inband 802.3z to set the duplex h/w
> > >    parameter.  If an_enabled and the MLO_PAUSE_AN bit of the pause
> > >    setting are true, allow 802.3z to set the pause h/w parameter.
> > >    Advertise capabilities depending on the 'advertising' setting.
> > >
> > > There's only one case where an_enabled is used, which is 802.3z
> > > negotiation, because the MAC side is responsible for negotiating the
> > > link mode.  In all other cases, the MAC is not responsible for any
> autonegotiation.
> >
> > It's clear for me that an_enabled is of use for the MAC only when clause 37
> auto-negotiation is used.
> >
> > However,  the DPAA2 software architecture abstracts the MAC and the
> network interface into 2 separate entities that are managed by two different
> drivers.
> > These drivers communicate only through a firmware.
> >
> > This means that any ethtool issued on a DPAA2 network interface will go
> directly to the firmware for the latest link information.
> 
> So you won't be calling phylink_ethtool_ksettings_get(), which means you
> won't be returning correct information anyway.
> 
> > When the MAC driver is not capable to inform the firmware of the proper
> link configuration (eg whether the autoneg is on or not), the ethtool output
> will not be the correct one.
> 
> You don't get to know the list of supported link modes, so I don't see how
> the ethtool information can be correct.
> 
> I'd like to see the patches _before_ I consider accepting your proposed
> phylink change.

Sure. I'll send an RFC as soon as possible.

> 
> > > It is important to stick to the above, which will ensure correct
> > > functioning of your driver - going off and doing your own thing
> > > (such as reading from other
> > > fields) is not guaranteed to give good results.
> >
> > You're right, but unfortunately I am not dealing with a straight-forward
> architecture.
> 
> At this point, I think you need to explain why you want to use phylink, as you
> seem to be saying that your driver is unable to conform to what phylink
> expects due to firmware.

That's going to be in the cover letter.

--
Ioana

Re: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[David Miller]

From: Ioana Ciornei <ioana.ciornei@nxp.com>
Date: Thu, 13 Jun 2019 09:37:51 +0300

> The phy_state field of phylink should carry only valid information
> especially when this can be passed to the .mac_config callback.
> Update the an_enabled field with the autoneg state in the
> phylink_phy_change function.
> 
> Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>

Applied and queued up for -stable, thanks.

Re: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Russell King - ARM Linux admin]

On Sat, Jun 15, 2019 at 01:30:21PM -0700, David Miller wrote:
> From: Ioana Ciornei <ioana.ciornei@nxp.com>
> Date: Thu, 13 Jun 2019 09:37:51 +0300
> 
> > The phy_state field of phylink should carry only valid information
> > especially when this can be passed to the .mac_config callback.
> > Update the an_enabled field with the autoneg state in the
> > phylink_phy_change function.
> > 
> > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> 
> Applied and queued up for -stable, thanks.

This is not a fix; it is an attempt to make phylink work differently
from how it's been designed for the dpaa2 driver.  I've already stated
that this field is completely meaningless, so I'm surprised you
applied it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Sat, 15 Jun 2019 23:13:28 +0100

> On Sat, Jun 15, 2019 at 01:30:21PM -0700, David Miller wrote:
>> From: Ioana Ciornei <ioana.ciornei@nxp.com>
>> Date: Thu, 13 Jun 2019 09:37:51 +0300
>> 
>> > The phy_state field of phylink should carry only valid information
>> > especially when this can be passed to the .mac_config callback.
>> > Update the an_enabled field with the autoneg state in the
>> > phylink_phy_change function.
>> > 
>> > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
>> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
>> 
>> Applied and queued up for -stable, thanks.
> 
> This is not a fix; it is an attempt to make phylink work differently
> from how it's been designed for the dpaa2 driver.  I've already stated
> that this field is completely meaningless, so I'm surprised you
> applied it.

I'm sorry, I did wait a day or so to see any direct responses to this
patch and I saw no feedback.

I'll revert.

Re: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Russell King - ARM Linux admin]

On Sat, Jun 15, 2019 at 06:08:54PM -0700, David Miller wrote:
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Date: Sat, 15 Jun 2019 23:13:28 +0100
> 
> > On Sat, Jun 15, 2019 at 01:30:21PM -0700, David Miller wrote:
> >> From: Ioana Ciornei <ioana.ciornei@nxp.com>
> >> Date: Thu, 13 Jun 2019 09:37:51 +0300
> >> 
> >> > The phy_state field of phylink should carry only valid information
> >> > especially when this can be passed to the .mac_config callback.
> >> > Update the an_enabled field with the autoneg state in the
> >> > phylink_phy_change function.
> >> > 
> >> > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> >> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> >> 
> >> Applied and queued up for -stable, thanks.
> > 
> > This is not a fix; it is an attempt to make phylink work differently
> > from how it's been designed for the dpaa2 driver.  I've already stated
> > that this field is completely meaningless, so I'm surprised you
> > applied it.
> 
> I'm sorry, I did wait a day or so to see any direct responses to this
> patch and I saw no feedback.
> 
> I'll revert.

Hi Dave,

Thanks for the revert.  There was discussion surrounding this patch:

https://www.mail-archive.com/netdev@vger.kernel.org/thrd2.html#302220

It was then re-posted as part of a later RFC series ("DPAA2 MAC
Driver") which shows why the change was proposed, where the discussion
continued on Friday.  The patch ended up with a slightly different
subject line.

There is still further discussion required to try and work out a way
forward.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

[Ioana Ciornei]

> Subject: Re: [PATCH] net: phylink: set the autoneg state in phylink_phy_change
> 
> On Sat, Jun 15, 2019 at 06:08:54PM -0700, David Miller wrote:
> > From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > Date: Sat, 15 Jun 2019 23:13:28 +0100
> >
> > > On Sat, Jun 15, 2019 at 01:30:21PM -0700, David Miller wrote:
> > >> From: Ioana Ciornei <ioana.ciornei@nxp.com>
> > >> Date: Thu, 13 Jun 2019 09:37:51 +0300
> > >>
> > >> > The phy_state field of phylink should carry only valid
> > >> > information especially when this can be passed to the .mac_config
> callback.
> > >> > Update the an_enabled field with the autoneg state in the
> > >> > phylink_phy_change function.
> > >> >
> > >> > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > >> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > >>
> > >> Applied and queued up for -stable, thanks.
> > >
> > > This is not a fix; it is an attempt to make phylink work differently
> > > from how it's been designed for the dpaa2 driver.  I've already
> > > stated that this field is completely meaningless, so I'm surprised
> > > you applied it.
> >
> > I'm sorry, I did wait a day or so to see any direct responses to this
> > patch and I saw no feedback.
> >
> > I'll revert.
> 
> Hi Dave,
> 
> Thanks for the revert.  There was discussion surrounding this patch:
> 
> https://www.mail-archive.com/netdev@vger.kernel.org/thrd2.html#302220 
> 
> It was then re-posted as part of a later RFC series ("DPAA2 MAC
> Driver") which shows why the change was proposed, where the discussion
> continued on Friday.  The patch ended up with a slightly different subject line.
> 
> There is still further discussion required to try and work out a way forward.
> 
> Thanks.
> 

Hi all, 

Sorry for not commenting on this until now but last weekend I had an unfortunate accident and ended up with a broken ankle. I'll start responding to all the emails and get back into this.

--
Ioana