Re: [PATCH net] net: ethernet: ti: cpsw: fix min eth packet size for non-switch use-cases

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 11/06/2021 20:54, Ben Hutchings wrote:
> On Fri, Jun 11, 2021 at 04:27:32PM +0300, Grygorii Strashko wrote:
> [...]
>> --- a/drivers/net/ethernet/ti/cpsw_new.c
>> +++ b/drivers/net/ethernet/ti/cpsw_new.c
>> @@ -918,14 +918,17 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
>>   	struct cpts *cpts = cpsw->cpts;
>>   	struct netdev_queue *txq;
>>   	struct cpdma_chan *txch;
>> +	unsigned int len;
>>   	int ret, q_idx;
>>   
>> -	if (skb_padto(skb, CPSW_MIN_PACKET_SIZE)) {
>> +	if (skb_padto(skb, priv->tx_packet_min)) {
>>   		cpsw_err(priv, tx_err, "packet pad failed\n");
>>   		ndev->stats.tx_dropped++;
>>   		return NET_XMIT_DROP;
>>   	}
>>   
>> +	len = skb->len < priv->tx_packet_min ? priv->tx_packet_min : skb->len;
>> +
>>   	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
>>   	    priv->tx_ts_enabled && cpts_can_timestamp(cpts, skb))
>>   		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>> @@ -937,7 +940,7 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
>>   	txch = cpsw->txv[q_idx].ch;
>>   	txq = netdev_get_tx_queue(ndev, q_idx);
>>   	skb_tx_timestamp(skb);
>> -	ret = cpdma_chan_submit(txch, skb, skb->data, skb->len,
>> +	ret = cpdma_chan_submit(txch, skb, skb->data, len,
>>   				priv->emac_port);
>>   	if (unlikely(ret != 0)) {
>>   		cpsw_err(priv, tx_err, "desc submit failed\n");
> 
> This change is odd because cpdma_chan_submit() already pads the DMA
> length.
> 
> Would it not make more sense to update cpdma_params::min_packet_size
> instead of adding a second minimum?

i've been thinking about it, but cpdma parameter copied into cpdma context once at probe,
so change will be more complex.
Can be done if you insist.

> 
> [...]
>> @@ -1686,6 +1690,7 @@ static int cpsw_dl_switch_mode_set(struct devlink *dl, u32 id,
>>   
>>   			priv = netdev_priv(sl_ndev);
>>   			slave->port_vlan = vlan;
>> +			priv->tx_packet_min = CPSW_MIN_PACKET_SIZE_VLAN;
>>   			if (netif_running(sl_ndev))
>>   				cpsw_port_add_switch_def_ale_entries(priv,
>>   								     slave);
>> @@ -1714,6 +1719,7 @@ static int cpsw_dl_switch_mode_set(struct devlink *dl, u32 id,
>>   
>>   			priv = netdev_priv(slave->ndev);
>>   			slave->port_vlan = slave->data->dual_emac_res_vlan;
>> +			priv->tx_packet_min = CPSW_MIN_PACKET_SIZE;
>>   			cpsw_port_add_dual_emac_def_ale_entries(priv, slave);
>>   		}
>>
> [...]
> 
> What happens if this races with the TX path?  Should there be a
> netif_tx_lock() / netif_tx_unlock() around this change?

Mode change operation is heavy and expected to be done once when bridge is configured.
It will completely wipe out all ALE entries and so all VLAN setting -
which any way need to be configured (reconfigured) during bridge configuration.
So, traffic can be disturbed during mode change operation.

As result, in my opinion, it make no sense to add additional complexity here.

-- 
Best regards,
grygorii