Re: [PATCH v3 4/5] net: hpe: Add GXP UMAC Driver

[Andrew Lunn <andrew@lunn.ch>]

> +static int umac_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd)
> +{
> +	if (!netif_running(ndev))
> +		return -EINVAL;
> +
> +	if (!ndev->phydev)
> +		return -ENODEV;
> +
> +	return phy_mii_ioctl(ndev->phydev, ifr, cmd);

Sometimes power management does not allow it, but can you use phy_do_ioctl()?

> +static int umac_init_hw(struct net_device *ndev)


> +{
> +	} else {
> +		value |= UMAC_CFG_FULL_DUPLEX;
> +
> +		if (ndev->phydev->speed == SPEED_1000) {

I'm pretty sure i pointed this out once before. It is not safe to
access phydev members outside of the adjust link callback.

> +static int umac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	struct umac_tx_desc_entry *ptxdesc;
> +	unsigned int length;
> +	u8 *pframe;
> +
> +	ptxdesc = &umac->tx_descs->entrylist[umac->tx_cur];
> +	pframe = umac->tx_descs->framelist[umac->tx_cur];
> +
> +	length = skb->len;
> +	if (length > 1514) {
> +		netdev_err(ndev, "send data %d bytes > 1514, clamp it to 1514\n",
> +			   skb->len);

Than should be rate limited.

Also, if you chop the end of the packet, it is going to be useless. It
is better to drop it, to improve your goodput.

> +		length = 1514;
> +	}
> +
> +	memset(pframe, 0, UMAC_MAX_FRAME_SIZE);
> +	memcpy(pframe, skb->data, length);

Is this cached or uncached memory? uncached is expansive so you want
to avoid touching it twice. Depending on how busy your cache is,
touching it twice might cause it to expelled from L1 on the first
write, so you could be writing to L2 twice for no reason. Do the math
and calculate the tail space you need to zero.

I would also suggest you look at the page pool code and use that for
all you buffer handling. It is likely to be more efficient than what
you have here.

> +static int umac_setup_phy(struct net_device *ndev)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	struct platform_device *pdev = umac->pdev;
> +	struct device_node *phy_handle;
> +	phy_interface_t interface;
> +	struct device_node *eth_ports_np;
> +	struct device_node *port_np;
> +	int ret;
> +	int i;
> +
> +	/* Get child node ethernet-ports. */
> +	eth_ports_np = of_get_child_by_name(pdev->dev.of_node, "ethernet-ports");
> +	if (!eth_ports_np) {
> +		dev_err(&pdev->dev, "No ethernet-ports child node found!\n");
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i < NUMBER_OF_PORTS; i++) {
> +		/* Get port@i of node ethernet-ports */
> +		port_np = gxp_umac_get_eth_child_node(eth_ports_np, i);
> +		if (!port_np)
> +			break;
> +
> +		if (i == INTERNAL_PORT) {
> +			phy_handle = of_parse_phandle(port_np, "phy-handle", 0);
> +			if (phy_handle) {
> +				umac->int_phy_dev = of_phy_find_device(phy_handle);
> +				if (!umac->int_phy_dev)
> +					return -ENODEV;

You appear to find the PHY, and then do nothing with it?

    Andrew