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 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

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

[Florinel Iordache]

 > Caution: EXT Email
> 
> 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

I think this is a very good idea: I think this extension would be useful for all PHYs not only for link training. 
Its purpose is only more user friendly prints and I needed this feature for backplane debugging.
But since I am not responsible for the PHY core, I made this workaround only in backplane driver.
If this small improvement could be done for all PHYs then I think this would be an added value from user friendliness perspective. 
Thank you.
Florin.

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

[Andrew Lunn]

> But since I am not responsible for the PHY core, I made this
> workaround only in backplane driver.

You don't need to be responsible for the core to make changes to the
code. Just send patches.

      Andrew