Re: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC ethernet drivers

[Andrew Lunn <andrew@lunn.ch>]

> >> +static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb);
> >> +static void enetc_unmap_tx_buff(struct enetc_bdr *tx_ring,
> >> +				struct enetc_tx_swbd *tx_swbd);
> >> +static int enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int budget);
> >> +
> >> +static struct sk_buff *enetc_map_rx_buff_to_skb(struct enetc_bdr *rx_ring,
> >> +						int i, u16 size);
> >> +static void enetc_add_rx_buff_to_skb(struct enetc_bdr *rx_ring, int i,
> >> +				     u16 size, struct sk_buff *skb);
> >> +static void enetc_process_skb(struct enetc_bdr *rx_ring, struct sk_buff *skb);
> >> +static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
> >> +			       struct napi_struct *napi, int work_limit);
> >> +
> >
> >Please try to avoid forward declarations. Move the code around so you
> >don't need them.
> >
> 
> Maybe I could drop some of these, but...
> For some performance critical functions on the datapath this would be an
> attempt to improve icache hit rate by having the caller function located in
> memory just before it's children (callees).

That is kind of the point of moving the functions. GCC can only inline
a function when it has already seen it, at least as far as i know. So
by having the functions in the correct order, you increase the
likelihood gcc inlines it, making the icache layout better, no
function calls, etc.

Try building the code both ways, and then disassemble it. See which
looks better.

> >> +static bool enetc_tx_csum(struct sk_buff *skb, union enetc_tx_bd *txbd)
> >> +{
> >> +	int l3_start, l3_hsize;
> >> +	u16 l3_flags, l4_flags;
> >> +
> >> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> >> +		return false;
> >> +
> >> +	switch (skb->csum_offset) {
> >> +	case offsetof(struct tcphdr, check):
> >> +		l4_flags = ENETC_TXBD_L4_TCP;
> >> +		break;
> >> +	case offsetof(struct udphdr, check):
> >> +		l4_flags = ENETC_TXBD_L4_UDP;
> >> +		break;
> >> +	default:
> >> +		skb_checksum_help(skb);
> >> +		return false;
> >> +	}
> >> +
> >> +	l3_start = skb_network_offset(skb);
> >> +	l3_hsize = skb_network_header_len(skb);
> >> +
> >> +	l3_flags = 0;
> >> +	if (skb->protocol == htons(ETH_P_IPV6))
> >> +		l3_flags = ENETC_TXBD_L3_IPV6;
> >
> >Is there no need to do anything with IPv4?
> >
> 
> No, 0 for this bit means IPv4. Only UDP and TCP over IPv4 and IPv6 supported.

Ah, O.K. Some i assume there is some way to tell it this is actually
an IP packet, not an IPX packet, etc.

> This code path is also reached by the VF driver (via the open() hook which is common to both
> PF and VF drivers, and VFs don't manage PHYs). 
> Also,  the driver may be exercised in MAC loopback mode (ethtool -K loopback on), when the
> PHY nodes are removed.  And it's always good to be able to run the driver on a simulator or
> an emulator without having to modify it.

O.K, that is fine.

> >> +int enetc_pci_probe(struct pci_dev *pdev, const char *name, int sizeof_priv)
> >> +{
> >> +	struct enetc_si *si, *p;
> >> +	struct enetc_hw *hw;
> >> +	size_t alloc_size;
> >> +	int err, len;
> >> +
> >> +	err = pci_enable_device_mem(pdev);
> >> +	if (err) {
> >> +		dev_err(&pdev->dev, "device enable failed\n");
> >> +		return err;
> >> +	}
> >> +
> >> +	/* set up for high or low dma */
> >> +	err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> >> +	if (err) {
> >> +		err = dma_set_mask_and_coherent(&pdev->dev,
> >DMA_BIT_MASK(32));
> >> +		if (err) {
> >> +			dev_err(&pdev->dev,
> >> +				"DMA configuration failed: 0x%x\n", err);
> >> +			goto err_dma;
> >> +		}
> >> +	}
> >
> >Humm, i thought drivers were not supposed to do this. The architecture
> >code should be setting masks. But i've not followed all those
> >conversations...
> >
> 
> Documentation/DMA-API-HOWTO.txt still states:
> " For correct operation, you must interrogate the kernel in your device
> probe routine to see if the DMA controller on the machine can properly
> support the DMA addressing limitation your device has.  It is good
> style to do this even if your device holds the default setting, [...]"

O.K, thanks for the reference.

     Andrew