On 20/03/2025 12:29, Ilya Maximets wrote:
> 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.
>
Yes another approach would be to fail the config now. I was trying to
have some fallback as an info_get() fail is not critical now, but
info_get() shouldn't fail in general. One benefit of failing now is that
from a user perspective, the default lsc config used will never change.
So I'm fine with just failing the config now.
@Jay, what do you think ?
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
