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

[Claudiu Manoil]

>-----Original Message-----
>From: Andrew Lunn <andrew@xxxxxxx>
>Sent: Saturday, November 17, 2018 2:30 AM
>To: Claudiu Manoil <claudiu.manoil@xxxxxxx>
>Cc: David S . Miller <davem@xxxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; Alexandru Marginean
><alexandru.marginean@xxxxxxx>; Catalin Horghidan
><catalin.horghidan@xxxxxxx>
>Subject: Re: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC
>ethernet drivers
>
[...]
>> +
>> +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).  As you can see, only some perf
critical functions are forward declared here, for the xmit() and rx/tx cleanup paths.
Does this make sense to you?  Could you point me to some reading on this topic otherwise?

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

[...]

>> +static void adjust_link(struct net_device *ndev)
>> +{
>> +	struct phy_device *phydev = ndev->phydev;
>> +
>> +	phy_print_status(phydev);
>
>You normally need more than that. Maybe later patches add to this?
>

Yes, later patches.  The first boards will be soon available.
Once we validate the remaining support for PHYs (including MDIO support which
is on the todo list) the patches will be available.  Also, we're looking at phylink too.
Once we can confirm whether it works on our boards, the driver will be updated accordingly.

>> +static int enetc_phy_connect(struct net_device *ndev)
>> +{
>> +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>> +	struct phy_device *phydev;
>> +
>> +	if (!priv->phy_node)
>> +		return 0; /* phy-less mode */
>
>What are your user-cases for phy-less? If you don't have a PHY it is
>mostly because you are connected to an Ethernet switch. In that case
>you use fixed-link, which gives you a phy.
>

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.

[...]

>> +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, [...]"

For the rest of the comments I didn't include in this reply: OK.

Thanks,
Claudiu