On 3/11/25 18:56, Kevin Traynor wrote:
> On 06/03/2025 19:38, Mike Pattrick wrote:
>> On Thu, Mar 6, 2025 at 9:53 AM Kevin Traynor <ktray...@redhat.com> wrote:
>>>
>>> From: Jay Ding <jay.d...@broadcom.com>
>>>
>>> rte_eth_dev_info_get() could fail due to device reset, etc.
>>>
>>> The return value should be checked before the device info
>>> pointer is dereferenced.
>>>
>>> Fixes: 2f196c80e716 ("netdev-dpdk: Use LSC interrupt mode.")
>>> Signed-off-by: Jay Ding <jay.d...@broadcom.com>
>>> ---
>>> lib/netdev-dpdk.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index 549887b31..35cd2ae17 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -2424,5 +2424,6 @@ netdev_dpdk_set_config(struct netdev *netdev, const
>>> struct smap *args,
>>>
>>> lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", true);
>>> - if (lsc_interrupt_mode && !(*info.dev_flags & RTE_ETH_DEV_INTR_LSC)) {
>>> + if (lsc_interrupt_mode && !ret &&
>>> + !(*info.dev_flags & RTE_ETH_DEV_INTR_LSC)) {
>>
>> Hello,
>>
>> With this, if rte_eth_dev_info_get() returns an error and the card
>> does not support lsc_interrupt_mode, we will always set
>> requested_lsc_interrupt_mode.
>>
>> Maybe it would be preferable to wrap all the lines related to
>> lsc_interrupt_mode in an "if (!ret)" ?
>>
>
> Hi Jay, did you see this comment ^^^ from Mike on your patch - what do
> you think ?
>
> If we can't get the device info and the user has requested
> "dpdk-lsc-interrupt" == true, then perhaps we should continue to
> configure what they requested.
>
> OTOH, we could change the default if the user has not explicitly
> requested anything and it is a safer option to prevent a configuration
> failure later.
>
> Something like below..?
>
> - lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", true);
> + lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt",
> + !ret ? true : false);
> if (lsc_interrupt_mode && !ret &&
> !(*info.dev_flags & RTE_ETH_DEV_INTR_LSC)) {
>
Hi, Kevin. Do you know if there is an actually valid case where the
info_get() would fail? Should we just fail the config entirely in
this case if we can't get the information about the device? Seems
weird to guess the configuration.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
