Re: [dpdk-dev] [PATCH v13 3/6] drivers/net: update Rx RSS hash offload capabilities

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/25/19 7:10 PM, Pavan Nikhilesh Bhagavatula wrote:
>> On 10/25/19 5:33 PM, pbhagavatula@marvell.com wrote:
>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>
>>> Add DEV_RX_OFFLOAD_RSS_HASH flag for all PMDs that support RSS
>> hash
>>> delivery.
>>>
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> Reviewed-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>> Acked-by: Jerin Jacob <jerinj@marvell.com>
>>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> [snip]
>>
>>> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
>> b/drivers/net/bnxt/bnxt_ethdev.c
>>> index e7ec99e15..d4f8cc92a 100644
>>> --- a/drivers/net/bnxt/bnxt_ethdev.c
>>> +++ b/drivers/net/bnxt/bnxt_ethdev.c
>>> @@ -117,7 +117,8 @@ static const struct rte_pci_id
>> bnxt_pci_id_map[] = {
>>>    				     DEV_RX_OFFLOAD_KEEP_CRC | \
>>>    				     DEV_RX_OFFLOAD_VLAN_EXTEND |
>> \
>>>    				     DEV_RX_OFFLOAD_TCP_LRO | \
>>> -				     DEV_RX_OFFLOAD_SCATTER)
>>> +				     DEV_RX_OFFLOAD_SCATTER | \
>>> +				     DEV_RX_OFFLOAD_RSS_HASH)
>>>
>>>    static int bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int
>> mask);
>>>    static void bnxt_print_link_info(struct rte_eth_dev *eth_dev);
>>> @@ -681,6 +682,12 @@ static int bnxt_dev_configure_op(struct
>> rte_eth_dev *eth_dev)
>>>    	bp->rx_cp_nr_rings = bp->rx_nr_rings;
>>>    	bp->tx_cp_nr_rings = bp->tx_nr_rings;
>>>
>>> +	if (!(rx_offloads & DEV_RX_OFFLOAD_RSS_HASH)) {
>>> +		PMD_DRV_LOG(INFO, "RX_OFFLOAD_RSS_HASH
>> cannot be disabled\n");
>>
>> Shouldn't logging be done from rte_eth_dev_configure()?
>> I.e. a generic function which is called after dev_configure callback and
>> take a look at dev_conf->rx_mode.offloads and
>> dev->data->dev_conf.rxmode.offloads and for each bit which differs
>> log message using rte_eth_dev_rx_offload_name().
>> Same for Tx while we are on the page. I.e. two more patch just before
>> this one.
>>
> Just to be clear this log would effect all offloads which can't be disabled for
> a give PMD.

I think it is perfectly fine and it is my goal in fact.

Thanks,
Andrew.