I think it is safe to fail the config whenever the info_get() fails. I will post the next fix for that.
Thanks, Jay On Thu, Mar 27, 2025 at 6:54 AM Kevin Traynor <[email protected]> wrote: > 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 <[email protected]> > wrote: > >>>> > >>>> From: Jay Ding <[email protected]> > >>>> > >>>> 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 <[email protected]> > >>>> --- > >>>> 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. > > > > > -- This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
