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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM0PR04MB54435DFD9870FA66C01E07CEFBD00@AM0PR04MB5443.eurprd04.prod.outlook.com>
Date:   Fri, 24 Apr 2020 15:08:55 +0000
From:   Florinel Iordache <florinel.iordache@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "corbet@....net" <corbet@....net>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        Leo Li <leoyang.li@....com>,
        "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Florinel Iordache <florinel.iordache@....com>
Subject: RE: [EXT] Re: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr
 driver support

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