netdev - RE: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter 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]

Date:	Fri, 1 Jul 2016 09:02:32 +0000
From:	Appana Durga Kedareswara Rao <appana.durga.rao@...inx.com>
To:	Nicolas Ferre <nicolas.ferre@...el.com>,
	Michal Simek <michals@...inx.com>,
	Punnaiah Choudary Kalluri <punnaia@...inx.com>,
	Anirudha Sarangi <anirudh@...inx.com>,
	Harini Katakam <harinik@...inx.com>,
	Florian Fainelli <f.fainelli@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support

Hi Nicolas Ferre,
	
	Thanks for the quick review...

> 
> Le 01/07/2016 08:20, Kedareswara rao Appana a écrit :
> > This patch adds support for gmii2rgmii phy converter in the macb
> > driver.
> 
> Okay, I'd like more explanation here.
> Hints & key words:
> - dt property
> - mdio bus
> - mac speed

Sure will fix in the next version...

> 
> 
> > Signed-off-by: Kedareswara rao Appana <appanad@...inx.com>
> > ---
> >  drivers/net/ethernet/cadence/macb.c |   21 ++++++++++++++++++++-
> >  drivers/net/ethernet/cadence/macb.h |    3 +++
> >  2 files changed, 23 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb.c
> > b/drivers/net/ethernet/cadence/macb.c
> > index cb07d95..de0ad71 100644
> > --- a/drivers/net/ethernet/cadence/macb.c
> > +++ b/drivers/net/ethernet/cadence/macb.c
> > @@ -257,6 +257,14 @@ static int macb_mdio_write(struct mii_bus *bus, int
> mii_id, int regnum,
> >  	return 0;
> >  }
> >
> > +static inline void macb_hw_fix_mac_speed(struct macb *bp,
> > +					 struct phy_device *phydev)
> > +{
> > +	if (likely(bp->converter_phy.fix_mac_speed))
> 
> What is the purpose of this branch bias? The code isn't in some hot path, so I
> suspect that its not needed.

If we won't put this check driver will crash with NULL pointer dereference for the below cases
---> Converter driver is disabled
---> Converter driver is enabled but the converter probe is not called from the macb driver.

> 
> > +		bp->converter_phy.fix_mac_speed(&bp->converter_phy,
> > +						phydev->speed);
> > +}
> > +
> >  /**
> >   * macb_set_tx_clk() - Set a clock to a new frequency
> >   * @clk		Pointer to the clock to change
> > @@ -329,6 +337,7 @@ static void macb_handle_link_change(struct
> net_device *dev)
> >  				reg |= GEM_BIT(GBE);
> >
> >  			macb_or_gem_writel(bp, NCFGR, reg);
> > +			macb_hw_fix_mac_speed(bp, phydev);
> >
> >  			bp->speed = phydev->speed;
> >  			bp->duplex = phydev->duplex;
> > @@ -2884,7 +2893,7 @@ static int macb_probe(struct platform_device
> *pdev)
> >  			struct clk **, struct clk **)
> >  					      = macb_clk_init;
> >  	int (*init)(struct platform_device *) = macb_init;
> > -	struct device_node *np = pdev->dev.of_node;
> > +	struct device_node *np = pdev->dev.of_node, *np1, *np11;
> 
> Nitpicking:
> Be more explicit on variable names. Simple name for the iterator is okay, the
> other is better if changed.
> I also like to see variable on separated lines.

Ok sure will fix in the next version...

> 
> >  	struct device_node *phy_node;
> >  	const struct macb_config *macb_config = NULL;
> >  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL; @@ -3011,6 +3020,16
> > @@ static int macb_probe(struct platform_device *pdev)
> >  		goto err_out_free_netdev;
> >
> >  	phydev = bp->phy_dev;
> > +	np1 = of_get_next_child(np, NULL);
> > +	for_each_child_of_node(np1, np11) {
> > +		if (of_device_is_compatible(np11, "xlnx,gmiitorgmii")) {
> 
> This would definitively need documentation and at least link in the macb binding
> to show how this emulated phy connect to the mdio bus...
> 
> I'm not able to judge if the node arrangement is okay: I let Florian tell his view
> on this...

Ok will document in the macb binding doc.

> 
> > +			bp->converter_phy.dev = dev;
> > +			bp->converter_phy.mii_bus = bp->mii_bus;
> > +			bp->converter_phy.mdio_write = macb_mdio_write;
> > +			bp->converter_phy.platform_data = bp->pdev-
> >dev.of_node;
> > +			gmii2rgmii_phyprobe(&bp->converter_phy);
> > +		}
> > +	}
> >
> >  	netif_carrier_off(dev);
> >
> > diff --git a/drivers/net/ethernet/cadence/macb.h
> > b/drivers/net/ethernet/cadence/macb.h
> > index 8a13824..735bce2 100644
> > --- a/drivers/net/ethernet/cadence/macb.h
> > +++ b/drivers/net/ethernet/cadence/macb.h
> > @@ -10,6 +10,8 @@
> >  #ifndef _MACB_H
> >  #define _MACB_H
> >
> > +#include <linux/xilinx_gmii2rgmii.h>
> 
> No, put it in the macb.c.

Ok will fix in v2...

> 
> > +
> >  #define MACB_GREGS_NBR 16
> >  #define MACB_GREGS_VERSION 2
> >  #define MACB_MAX_QUEUES 8
> > @@ -846,6 +848,7 @@ struct macb {
> >  	unsigned int		jumbo_max_len;
> >
> >  	u32			wol;
> > +	struct	gmii2rgmii	converter_phy;
> >  };
> >
> >  static inline bool macb_is_gem(struct macb *bp)
> 
> 
> If Florian and phy guys are okay with the approach, I'm fine with this patch, once
> corrected.

Ok thanks...

Regards,
Kedar.

> 
> Thanks, bye,
> --
> Nicolas Ferre