RE: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr driver support

RE: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr driver support

[Florinel Iordache]

> > +/* Backplane mutex between all KR PHY threads */ static struct mutex
> > +backplane_lock;
> 
> 
> > +/* Read AN Link Status */
> > +static int is_an_link_up(struct phy_device *phydev) {
> > +     struct backplane_device *bpdev = phydev->priv;
> > +     int ret, val = 0;
> > +
> > +     mutex_lock(&bpdev->bpphy_lock);
> 
> Last time i asked the question about how this mutex and the phy mutex interact.
> I don't remember seeing an answer.
> 
>           Andrew

Hi Andrew,
Yes, your question was:
<<How does this mutex interact with phydev->lock? It appears both are trying to do the same thing, serialise access to the PHY hardware.>>
The answer is: yes, you are right, they both are protecting the critical section related to accessing the PHY hardware for a particular PHY.
As you can see the backplane device (bpdev) has associated one phy_device (phydev) so  bpdev->bpphy_lock and phydev->lock are equivalent.
Normally your assumption is correct: backplane driver should use the same phydev->lock but there is the following problem:
Backplane driver needs to protect all accesses to a PHY hardware including the ones coming from backplane scheduled workqueues for all lanes within a PHY.
But phydev->lock is already acquired for a phy_device (from phy.c) before each phy_driver callback is called (e.g.: config_aneg, suspend, ...)
So if I would use phydev->lock instead of bpdev->bpphy_lock then this would result in a deadlock when it is called from phy_driver callbacks.
However a possible solution would be to remove all these locks using bpphy_lock and use instead only one phydev->lock in backplane kr state machine: (bp_kr_state_machine).
But this solution will result in poorer performance, the training total duration will increase because only one single lane can enter the training procedure at a time therefore it would be possible for multi-lane phy training to ultimately fail because training is not finished in under 500ms. So I wanted to avoid this loss of training performance.
Yet another possible solution would be to keep the locks where they are, at the lowest level exactly at phy_read/write_mmd calls, in order to allow lanes training running in parallel, but use instead the phydev->lock as would be normal to be and according to your suggestion.
But in this case I must avoid the deadlock I mentioned above by differentiating between the calls coming from phy_driver callbacks where the phydev->lock is already acquired for this phy_device by the phy framework so the mutex should be skipped in this case and the calls coming from anywhere else (for example from backplane kr state machine) when the phydev->lock was not already acquired for this phy_device and the mutex must be used.
If you agree with this latest solution then I can implement it in next version by using a flag in backplane_device called: 'phy_mutex_already_acquired' or 'skip_phy_mutex' which must be set in all backplane phy_driver callbacks and will be used to skip the locks on phydev->lock used at phy_read/write_mmd calls in these cases.

I'm sorry I have not answered this question the first time when you asked it.
Florin.

Re: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr driver support

[Russell King - ARM Linux admin]

On Mon, Apr 27, 2020 at 12:40:37PM +0000, Florinel Iordache wrote:
> > > +/* Backplane mutex between all KR PHY threads */ static struct mutex
> > > +backplane_lock;
> > 
> > 
> > > +/* Read AN Link Status */
> > > +static int is_an_link_up(struct phy_device *phydev) {
> > > +     struct backplane_device *bpdev = phydev->priv;
> > > +     int ret, val = 0;
> > > +
> > > +     mutex_lock(&bpdev->bpphy_lock);
> > 
> > Last time i asked the question about how this mutex and the phy mutex interact.
> > I don't remember seeing an answer.
> > 
> >           Andrew
> 
> Hi Andrew,
> Yes, your question was:
> <<How does this mutex interact with phydev->lock? It appears both are trying to do the same thing, serialise access to the PHY hardware.>>
> The answer is: yes, you are right, they both are protecting the critical section related to accessing the PHY hardware for a particular PHY.
> As you can see the backplane device (bpdev) has associated one phy_device (phydev) so  bpdev->bpphy_lock and phydev->lock are equivalent.
> Normally your assumption is correct: backplane driver should use the same phydev->lock but there is the following problem:
> Backplane driver needs to protect all accesses to a PHY hardware including the ones coming from backplane scheduled workqueues for all lanes within a PHY.
> But phydev->lock is already acquired for a phy_device (from phy.c) before each phy_driver callback is called (e.g.: config_aneg, suspend, ...)
> So if I would use phydev->lock instead of bpdev->bpphy_lock then this would result in a deadlock when it is called from phy_driver callbacks.
> However a possible solution would be to remove all these locks using bpphy_lock and use instead only one phydev->lock in backplane kr state machine: (bp_kr_state_machine).
> But this solution will result in poorer performance, the training total duration will increase because only one single lane can enter the training procedure at a time therefore it would be possible for multi-lane phy training to ultimately fail because training is not finished in under 500ms. So I wanted to avoid this loss of training performance.
> Yet another possible solution would be to keep the locks where they are, at the lowest level exactly at phy_read/write_mmd calls, in order to allow lanes training running in parallel, but use instead the phydev->lock as would be normal to be and according to your suggestion.
> But in this case I must avoid the deadlock I mentioned above by differentiating between the calls coming from phy_driver callbacks where the phydev->lock is already acquired for this phy_device by the phy framework so the mutex should be skipped in this case and the calls coming from anywhere else (for example from backplane kr state machine) when the phydev->lock was not already acquired for this phy_device and the mutex must be used.
> If you agree with this latest solution then I can implement it in next version by using a flag in backplane_device called: 'phy_mutex_already_acquired' or 'skip_phy_mutex' which must be set in all backplane phy_driver callbacks and will be used to skip the locks on phydev->lock used at phy_read/write_mmd calls in these cases.

I think you have a rather big misunderstanding of the locking in phylib
from what you said above.

The register accessors do not use phydev->lock.

Follow the code.

	phy_read_mmd() uses phy_lock_mdio_bus().

	phy_lock_mdio_bus() locks the phydev->mdio.bus->mdio_lock mutex.

This is the _bus_ level lock, and is entirely different from
phydev->lock.

It is entirely safe to call phy_read_mmd() from any region of code
which is holding phydev->lock - indeed, we have many PHY drivers that
already do this.

So, I think you need to rewrite your entire locking strategy, because
it seems that you've misunderstood the locking here.

However, it's actually way worse, because of the abuse in your driver
of a single phy_device struct, which you use to access multiple PHYs,
randomly changing phydev->mdio.addr according to which PHY needs to be
accessed - you need to _carefully_ consider how your locking is done
for that.  I regard this as a big abuse, and I'm very tempted to NAK
your patches on this abuse alone.

I think you need to take onboard my comments about the (ab)use of
phy_device here.

An alternative solution to this is to push the phy_* accessors up a
level to the mdiobus level (we already have some, and I've already
been converting others) so you don't have to mess with
phydev->mdio.addr at all.

However, I would still consider your use of struct phy_device to be
an abuse.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

RE: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr driver support

[Florinel Iordache]

> > +/* Backplane custom logging */
> > +#define bpdev_fn(fn)                                                 \
> > +void bpdev_##fn(struct phy_device *phydev, char *fmt, ...)           \
> > +{                                                                    \
> > +     struct va_format vaf = {                                        \
> > +             .fmt = fmt,                                             \
> > +     };                                                              \
> > +     va_list args;                                                   \
> > +     va_start(args, fmt);                                            \
> > +     vaf.va = &args;                                                 \
> > +     if (!phydev->attached_dev)                                      \
> > +             dev_##fn(&phydev->mdio.dev, "%pV", &vaf);               \
> > +     else                                                            \
> > +             dev_##fn(&phydev->mdio.dev, "%s: %pV",                  \
> > +                     netdev_name(phydev->attached_dev), &vaf);       \
> > +     va_end(args);                                                   \
> > +}
> > +
> > +bpdev_fn(err)
> > +EXPORT_SYMBOL(bpdev_err);
> > +
> > +bpdev_fn(warn)
> > +EXPORT_SYMBOL(bpdev_warn);
> > +
> > +bpdev_fn(info)
> > +EXPORT_SYMBOL(bpdev_info);
> > +
> > +bpdev_fn(dbg)
> > +EXPORT_SYMBOL(bpdev_dbg);
> 
> Didn't i say something about just using phydev_{err|warn|info|dbg}?
> 
>        Andrew

Hi Andrew,

I used this custom logging in order to be able to add any kind of useful information we might need to all prints (err/warn/info/dbg).
For example all these bpdev_ functions are equivalent with phydev_ but only in the case when there is no attached device: phydev->attached_dev == NULL.
Otherwise, if there is a device attached, then we also want to add its name to all these prints in order to know to which device the information refers to.
For example in this case the print looks like this:
[   50.853515] backplane_qoriq 8c13000:00: eth1: 10gbase-kr link trained, Tx equalization: C(-1)=0x0, C(0)=0x29, C(+1)=0x5
This is very useful because we can see very easy to which interface the information printed is related to: in this case the link was trained for interface: eth1
This information (the name of attached device: eth1) is not printed by phydev_ functions.
I'm sorry I have not explained all this earlier, the first time when you asked about it. 

Florin.

Re: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr driver support

[Andrew Lunn]

On Fri, Apr 24, 2020 at 02:39:54PM +0000, Florinel Iordache wrote:
> > > +/* Backplane custom logging */
> > > +#define bpdev_fn(fn)                                                 \
> > > +void bpdev_##fn(struct phy_device *phydev, char *fmt, ...)           \
> > > +{                                                                    \
> > > +     struct va_format vaf = {                                        \
> > > +             .fmt = fmt,                                             \
> > > +     };                                                              \
> > > +     va_list args;                                                   \
> > > +     va_start(args, fmt);                                            \
> > > +     vaf.va = &args;                                                 \
> > > +     if (!phydev->attached_dev)                                      \
> > > +             dev_##fn(&phydev->mdio.dev, "%pV", &vaf);               \
> > > +     else                                                            \
> > > +             dev_##fn(&phydev->mdio.dev, "%s: %pV",                  \
> > > +                     netdev_name(phydev->attached_dev), &vaf);       \
> > > +     va_end(args);                                                   \
> > > +}
> > > +
> > > +bpdev_fn(err)
> > > +EXPORT_SYMBOL(bpdev_err);
> > > +
> > > +bpdev_fn(warn)
> > > +EXPORT_SYMBOL(bpdev_warn);
> > > +
> > > +bpdev_fn(info)
> > > +EXPORT_SYMBOL(bpdev_info);
> > > +
> > > +bpdev_fn(dbg)
> > > +EXPORT_SYMBOL(bpdev_dbg);
> > 
> > Didn't i say something about just using phydev_{err|warn|info|dbg}?
> > 
> >        Andrew
> 
> Hi Andrew,
> 
> I used this custom logging in order to be able to add any kind of useful information we might need to all prints (err/warn/info/dbg).
> For example all these bpdev_ functions are equivalent with phydev_ but only in the case when there is no attached device: phydev->attached_dev == NULL.
> Otherwise, if there is a device attached, then we also want to add its name to all these prints in order to know to which device the information refers to.
> For example in this case the print looks like this:
> [   50.853515] backplane_qoriq 8c13000:00: eth1: 10gbase-kr link trained, Tx equalization: C(-1)=0x0, C(0)=0x29, C(+1)=0x5
> This is very useful because we can see very easy to which interface the information printed is related to: in this case the link was trained for interface: eth1
> This information (the name of attached device: eth1) is not printed by phydev_ functions.
> I'm sorry I have not explained all this earlier, the first time when you asked about it. 

So why not argue that the phydev_* functions should be extended to
include this information? Is this extra information only valuable for
link training, or for anything a PHY does? If the core does not do
something, fix the core, rather than work around it in your driver.

     Andrew