Re: [dpdk-dev] [PATCH v2 09/44] net/virtio: move MSIX detection to PCI ethdev

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 1/21/21 8:12 AM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, January 20, 2021 5:25 AM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
>> amorenoz@redhat.com; david.marchand@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH v2 09/44] net/virtio: move MSIX detection to PCI ethdev
>>
>> This patch introduces a new callback to notify the bus
>> driver some interrupt related operation was done.
>>
>> This is used by Virtio PCI driver to check msix status.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_ethdev.c     |  12 +--
>>  drivers/net/virtio/virtio_pci.c        | 120 ++++++++++++++-----------
>>  drivers/net/virtio/virtio_pci.h        |   6 +-
>>  drivers/net/virtio/virtio_pci_ethdev.c |   2 +
>>  4 files changed, 82 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>> b/drivers/net/virtio/virtio_ethdev.c
>> index a3e81f336d..13d5a76376 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1287,8 +1287,8 @@ virtio_intr_unmask(struct rte_eth_dev *dev)
>>  	if (rte_intr_ack(dev->intr_handle) < 0)
>>  		return -1;
>>
>> -	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type ==
>> VIRTIO_BUS_PCI_MODERN)
>> -		hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev));
>> +	if (VTPCI_OPS(hw)->intr_event)
> 
> Emmm.. why is callback called intr_event? The callback detects/checks msix and lsc.
> IMHO, maybe intr_detect? Or another similar verb you like 😊

I called it intr_event because it could be used for other purpose than
intr detection. But this is the only use for now so let's immplement
your suggestion and we can revisit later if needed.

>> +		VTPCI_OPS(hw)->intr_event(hw);
>>
>>  	return 0;
> 
> <snip>
> 
>>
>> +msix_detect:
>> +	hw->use_msix = vtpci_msix_detect(dev);
>> +
> 
> Just use use the callback introduced? Should also work later when intr_lsc is added.

I didn't do it given that it was in the same file and that the callback
was to report an event happened. With renaming the callback as you
suggest, it makes sense to use it.

I will fix in v3.

Thanks,
Maxime

> Thanks,
> Chenbo
>