Message-ID: <90A7E81AE28BAE4CBDDB3B35F187D2644071DF4F@CHN-SV-EXMX02.mchp-main.com>
Date:   Fri, 23 Feb 2018 22:44:26 +0000
From:   <Bryan.Whitehead@...rochip.com>
To:     <f.fainelli@...il.com>, <davem@...emloft.net>
CC:     <UNGLinuxDriver@...rochip.com>, <netdev@...r.kernel.org>
Subject: RE: [PATCH v2 net-next 1/2] lan743x: Add main source files for new
 lan743x driver

Hi Florian,
Thanks for your review. I have the following questions/comments.

> On 02/21/2018 11:06 AM, Bryan Whitehead wrote:
> > Add main source files for new lan743x driver.
> >
> > Signed-off-by: Bryan Whitehead <Bryan.Whitehead@...rochip.com>
> > ---
> 
> > +lan743x-objs := lan743x_main.o
> 
> Should we assume that you have additional object files you would like to
> contribute at a later point? If that is the case, this is fine, if this is going to be
> only file of this driver, consider renaming it so you don't even have to have
> this lan743x-objs line at all.

I am planning to add additional source files later. At the very least there will
be a lan743x_ethtool.o, and a lan743x_ptp.o. I didn't want to have to change
the name of the main file later, so I gave it the expected name now.

> > +
> > +static void lan743x_pci_cleanup(struct lan743x_adapter *adapter) {
> > +	struct lan743x_pci *pci = &adapter->pci;
> > +
> > +	if (pci->init_flags & INIT_FLAG_PCI_REGIONS_REQUESTED) {
> 
> There is a pattern throughout the driver of maintaining flags to track what
> was initialized and what was not, do you really need that, or can you check
> for specific book keeping private information. Maintaining flags is error prone
> and requires you to keep adding new ones, that does not really scale.

Ok, but it seems to me that what I have is an example of "specific book keeping
private information". Can you clarify the style you prefer?

In cases of allocation where I can just compare a pointer to null, I can easily remove
the flags. But in other cases I need a record of which steps completed in order to
clean up properly. In cases where I need some sort of a flag would you prefer
I avoid a bit mask, and have a standalone variable for each flag?

[snip]

> > +		dmac_int_en &= ioc_bit;
> > +		dmac_int_sts &= dmac_int_en;
> > +		if (dmac_int_sts & ioc_bit) {
> > +			tasklet_schedule(&tx->tx_isr_bottom_half);
> > +			enable_flag = 0;/* tasklet will re-enable later */
> > +		}
> 
> Consider migrating your TX buffer reclamation to a NAPI context. If you have
> one TX queue and one RX, the same NAPI context can be re-used, if you
> have separate RX/TX queues, you may create a NAPI context per RX/TX pair,
> or you may create separate NAPI contexts per TX queues and RX queues.

This may take some time to refactor, But I will see what I can do.

[snip]


> > +static int lan743x_phy_open(struct lan743x_adapter *adapter) {
> > +	struct lan743x_phy *phy = &adapter->phy;
> > +	struct phy_device *phydev;
> > +	struct net_device *netdev;
> > +	int ret = -EIO;
> > +	u32 mii_adv;
> > +
> > +	netdev = adapter->netdev;
> > +	phydev = phy_find_first(adapter->mdiobus);
> > +	if (!phydev) {
> > +		ret = -EIO;
> > +		goto clean_up;
> > +	}
> > +	ret = phy_connect_direct(netdev, phydev,
> > +				 lan743x_phy_link_status_change,
> > +				 PHY_INTERFACE_MODE_GMII);
> > +	if (ret)
> > +		goto clean_up;
> > +	phy->flags |= PHY_FLAG_ATTACHED;
> > +
> > +	/* MAC doesn't support 1000T Half */
> > +	phydev->supported &= ~SUPPORTED_1000baseT_Half;
> > +
> > +	/* support both flow controls */
> > +	phy->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX);
> > +	phydev->advertising &= ~(ADVERTISED_Pause |
> ADVERTISED_Asym_Pause);
> > +	mii_adv = (u32)mii_advertise_flowctrl(phy->fc_request_control);
> > +	phydev->advertising |= mii_adv_to_ethtool_adv_t(mii_adv);
> > +	phy->fc_autoneg = phydev->autoneg;
> 
> No driver does things like that, that appears to be really wrong.
Can you clarify? What is wrong and how should it be?

[snip]

> > +static int lan743x_dmac_init(struct lan743x_adapter *adapter) {
> > +	struct lan743x_dmac *dmac = &adapter->dmac;
> > +	u32 data = 0;
> > +
> > +	dmac->flags = 0;
> > +	dmac->descriptor_spacing = DEFAULT_DMA_DESCRIPTOR_SPACING;
> > +	lan743x_csr_write(adapter, DMAC_CMD, DMAC_CMD_SWR_);
> > +	lan743x_csr_wait_for_bit(adapter, DMAC_CMD,
> DMAC_CMD_SWR_,
> > +				 0, 1000, 20000, 100);
> > +	switch (dmac->descriptor_spacing) {
> > +	case DMA_DESCRIPTOR_SPACING_16:
> > +		data = DMAC_CFG_MAX_DSPACE_16_;
> > +		break;
> > +	case DMA_DESCRIPTOR_SPACING_32:
> > +		data = DMAC_CFG_MAX_DSPACE_32_;
> > +		break;
> > +	case DMA_DESCRIPTOR_SPACING_64:
> > +		data = DMAC_CFG_MAX_DSPACE_64_;
> > +		break;
> > +	case DMA_DESCRIPTOR_SPACING_128:
> > +		data = DMAC_CFG_MAX_DSPACE_128_;
> > +		break;
> > +	default:
> > +		return -EPERM;
> 
> -EINVAL maybe?
I think -EPERM is more appropriate because it reflects an unresolvable error. 
In other words the platform is in compatible with the device.
Would you prefer I use a preprocessor error instead, such as

#if invalid_setting
#error incompatible setting
#endif

> 
> [snip]
> 
> > +
> > +done:
> > +	memset(buffer_info, 0, sizeof(*buffer_info));
> > +	memset(descriptor, 0, sizeof(*descriptor));
> 
> You are likely missing barriers here, what guarantees that the buffer_info
> and descriptor are going to be properly maintained in a non-cache coherent
> architecture?

I believe a memory barrier is only necessary just before the device is likely
to access that memory. The device is only allowed to access that memory
after a write to the TX_TAIL register.

This step happens in lan743x_tx_frame_end, and there is a dma_wmb just before it.

If possible I prefer to avoid unnecessary barriers so the hardware can optimize bus
traffic better.

Do you agree, or am I misunderstanding something?

[snip]

> > +
> > +static void lan743x_tx_release_all_descriptors(struct lan743x_tx *tx)
> > +{
> > +	u32 original_head = 0;
> > +
> > +	original_head = tx->last_head;
> > +	do {
> > +		lan743x_tx_release_desc(tx, tx->last_head, true);
> > +		tx->last_head = lan743x_tx_next_index(tx, tx->last_head);
> > +	} while (tx->last_head != original_head);
> > +	memset(tx->ring_cpu_ptr, 0,
> > +	       sizeof(*tx->ring_cpu_ptr) * (tx->ring_size));
> > +	memset(tx->buffer_info, 0,
> > +	       sizeof(*tx->buffer_info) * (tx->ring_size));
> 
> This is coherent memory, but you still need to prevent possible re-orderings,
> so you need at least a write barrier to guarantee all pending writes are
> completed. Likely you won't ever see this as a problem on x86 platforms.

Again, the driver is not permitting the device to access the memory yet, 
so I think a memory barrier is unnecessary.
Do you agree?

[snip]
 
> > +	tx->frame_flags |= TX_FRAME_FLAG_IN_PROGRESS;
> > +	tx->frame_first = tx->last_tail;
> > +	tx->frame_tail = tx->frame_first;
> > +	tx_descriptor = &tx->ring_cpu_ptr[tx->frame_tail];
> > +	buffer_info = &tx->buffer_info[tx->frame_tail];
> > +	dma_ptr = dma_map_single(dev, first_buffer, first_buffer_length,
> > +				 DMA_TO_DEVICE);
> > +	if (dma_mapping_error(dev, dma_ptr))
> > +		return -ENOMEM;
> > +	tx_descriptor->data1 = DMA_ADDR_LOW32(dma_ptr);
> > +	tx_descriptor->data2 = DMA_ADDR_HIGH32(dma_ptr);
> > +	tx_descriptor->data3 = (frame_length << 16) &
> > +		TX_DESC_DATA3_FRAME_LENGTH_MSS_MASK_;
> > +	buffer_info->skb = NULL;
> > +	buffer_info->dma_ptr = dma_ptr;
> > +	buffer_info->buffer_length = first_buffer_length;
> > +	buffer_info->flags |= TX_BUFFER_INFO_FLAG_ACTIVE;
> > +	tx->frame_data0 = (first_buffer_length &
> > +		TX_DESC_DATA0_BUF_LENGTH_MASK_) |
> > +		TX_DESC_DATA0_DTYPE_DATA_ |
> > +		TX_DESC_DATA0_FS_ |
> > +		TX_DESC_DATA0_FCS_;
> > +	if (check_sum)
> > +		tx->frame_data0 |= TX_DESC_DATA0_ICE_ |
> > +				   TX_DESC_DATA0_IPE_ |
> > +				   TX_DESC_DATA0_TPE_;
> > +
> > +	/* data0 will be programmed in one of other frame assembler
> > +functions */
> 
> You again need some sort of write barrier here to guarantee ordering and
> make sure the descriptors/buffer_info are properly written. 

Again, the driver is not permitting the device to access the memory yet, 
so I think a memory barrier is unnecessary.
Do you agree?


> > +static void lan743x_tx_frame_add_lso(struct lan743x_tx *tx,
> > +				     unsigned int frame_length)
> > +{
> > +	/* called only from within lan743x_tx_xmit_frame.
> > +	 * assuming tx->ring_lock has already been acquired.
> > +	 */
> > +	struct lan743x_tx_descriptor *tx_descriptor = NULL;
> > +	struct lan743x_tx_buffer_info *buffer_info = NULL;
> > +
> > +	/* wrap up previous descriptor */
> > +	tx->frame_data0 |= TX_DESC_DATA0_EXT_;
> > +	tx_descriptor = &tx->ring_cpu_ptr[tx->frame_tail];
> > +	tx_descriptor->data0 = tx->frame_data0;
> > +
> > +	/* move to next descriptor */
> > +	tx->frame_tail = lan743x_tx_next_index(tx, tx->frame_tail);
> > +	tx_descriptor = &tx->ring_cpu_ptr[tx->frame_tail];
> > +	buffer_info = &tx->buffer_info[tx->frame_tail];
> > +
> > +	/* add extension descriptor */
> > +	tx_descriptor->data1 = 0;
> > +	tx_descriptor->data2 = 0;
> > +	tx_descriptor->data3 = 0;
> > +	buffer_info->skb = NULL;
> > +	buffer_info->dma_ptr = 0;
> > +	buffer_info->buffer_length = 0;
> > +	buffer_info->flags |= TX_BUFFER_INFO_FLAG_ACTIVE;
> > +	tx->frame_data0 = (frame_length &
> TX_DESC_DATA0_EXT_PAY_LENGTH_MASK_) |
> > +			  TX_DESC_DATA0_DTYPE_EXT_ |
> > +			  TX_DESC_DATA0_EXT_LSO_;
> 
> 
> And here again.
Again, the driver is not permitting the device to access the memory yet, 
so I think a memory barrier is unnecessary.
Do you agree?

[snip]

> > +static void lan743x_tx_frame_end(struct lan743x_tx *tx,
> > +				 struct sk_buff *skb,
> > +				 bool ignore_sync)
> > +{
> > +	/* called only from within lan743x_tx_xmit_frame
> > +	 * assuming tx->ring_lock has already been acquired
> > +	 */
> > +	struct lan743x_tx_descriptor *tx_descriptor = NULL;
> > +	struct lan743x_tx_buffer_info *buffer_info = NULL;
> > +	struct lan743x_adapter *adapter = tx->adapter;
> > +	u32 tx_tail_flags = 0;
> > +
> > +	/* wrap up previous descriptor */
> > +	tx->frame_data0 |= TX_DESC_DATA0_LS_;
> > +	tx->frame_data0 |= TX_DESC_DATA0_IOC_;
> > +	tx_descriptor = &tx->ring_cpu_ptr[tx->frame_tail];
> > +	buffer_info = &tx->buffer_info[tx->frame_tail];
> > +	buffer_info->skb = skb;
> > +	if (ignore_sync)
> > +		buffer_info->flags |=
> TX_BUFFER_INFO_FLAG_IGNORE_SYNC;
> > +	tx_descriptor->data0 = tx->frame_data0;
> > +	tx->frame_tail = lan743x_tx_next_index(tx, tx->frame_tail);
> > +	tx->last_tail = tx->frame_tail;
> > +	dma_wmb();
Here is the barrier

> > +	if (tx->vector_flags &
> LAN743X_VECTOR_FLAG_VECTOR_ENABLE_AUTO_SET)
> > +		tx_tail_flags |= TX_TAIL_SET_TOP_INT_VEC_EN_;
> > +	if (tx->vector_flags &
> LAN743X_VECTOR_FLAG_SOURCE_ENABLE_AUTO_SET)
> > +		tx_tail_flags |= TX_TAIL_SET_DMAC_INT_EN_ |
> > +		TX_TAIL_SET_TOP_INT_EN_;
> > +	lan743x_csr_write(adapter, TX_TAIL(tx->channel_number),
> > +			  tx_tail_flags | tx->frame_tail);
Here is TX_TAIL write that permits the device to access descriptor ring
And buffers.

> > +	tx->frame_flags &= ~TX_FRAME_FLAG_IN_PROGRESS; }
> > +

> > +static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
> > +					 struct sk_buff *skb)
> > +{
> > +	int required_number_of_descriptors = 0;
> > +	unsigned int start_frame_length = 0;
> > +	unsigned int frame_length = 0;
> > +	unsigned int head_length = 0;
> > +	unsigned long irq_flags = 0;
> > +	bool ignore_sync = false;
> > +	int nr_frags = 0;
> > +	bool gso = false;
> > +	int j;
> > +
> > +	spin_lock_irqsave(&tx->ring_lock, irq_flags);
> > +	required_number_of_descriptors = lan743x_tx_get_desc_cnt(tx,
> skb);
> 
> This can certainly execute outside of the spinlock
Will move
> 
> > +	if (required_number_of_descriptors >
> > +		lan743x_tx_get_avail_desc(tx)) {
> 
> This one, probably not.
> 
> > +		if (required_number_of_descriptors > (tx->ring_size - 1)) {
> > +			dev_kfree_skb(skb);
> > +		} else {
> > +			/* save to overflow buffer */
> > +			tx->overflow_skb = skb;
> > +			netif_stop_queue(tx->adapter->netdev);
> 
> What is an overflow SKB?
If the descriptor ring does not have enough descriptors for the skb then
instead of dropping it, the queue is stopped, and the skb is stored in
the overflow_skb.

When ring space is restored inside lan743x_tx_isr_bottom_half,
the transmission of the overflow_skb is retried before the
netif_wake_queue is called.

Since skb's may require an unknown number of descriptors
this mechanism will stop the queue on the fly. But store it for
retransmission later. 

> 
> [snip]
> 
> > +static struct net_device_stats *lan743x_netdev_get_stats(struct
> > +net_device *nd) {
> > +	struct lan743x_adapter *adapter = netdev_priv(nd);
> 
> 64-bit statistics please.
Will do.

Regards,
Bryan

Powered by blists - more mailing lists