From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luca Boccassi Subject: Re: [PATCH] net/e1000: do not error out if rx_drop_en is set Date: Mon, 08 Oct 2018 15:14:23 +0100 Message-ID: <1539008063.8721.22.camel@debian.org> References: <20180727172607.16890-1-bluca@debian.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: "Lu, Wenzhuo" , "stable@dpdk.org" , qi.z.zhang@intel.com, 3chas3@gmail.com To: "Zhao1, Wei" , "dev@dpdk.org" Return-path: In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, 2018-10-08 at 08:43 +0000, Zhao1, Wei wrote: > Hi,=C2=A0=C2=A0Luca Boccassi >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Luca Boccassi > > Sent: Saturday, July 28, 2018 1:26 AM > > To: dev@dpdk.org > > Cc: Lu, Wenzhuo ; Luca Boccassi > > ; stable@dpdk.org > > Subject: [dpdk-dev] [PATCH] net/e1000: do not error out if > > rx_drop_en is set > >=20 > > rx_drop_en is an optimization that does nothing on single-queue > > devices like > > e1000. Do not force applications that do not care to select per- > > devices >=20 > And aslo, eth_em_rx_queue_setup support set up of multiple queues for > EM device. >=20 > > optimizations flags by returning an error, just log it and carry > > on. >=20 > rx_drop_en is a flag to enable receive packets drop when insufficient > receive descriptors exist to write the packet into system memory. > if we delete this parameter check protection, it may be misleading > some applications for this point, why does not give some requirement=C2= =A0 > for proper usage of this? I do not suggest for this change. > You can also refer to function eth_igb_rx_queue_setup(), igb device > support rx_drop_en set, so we do not have a such parameter check. Hi, As mentioned, because given it does nothing it's not worth aborting the program with an error. Logging a notice level message and carrying on is sufficient. We should try not make it harder than necessary for application developers. > >=20 > > Fixes: 805803445a02 ("e1000: support EM devices (also known as > > e1000/e1000e)") > > Cc: stable@dpdk.org > >=20 > > Signed-off-by: Luca Boccassi > > --- > > =C2=A0drivers/net/e1000/em_rxtx.c | 7 ++++--- > > =C2=A01 file changed, 4 insertions(+), 3 deletions(-) > >=20 > > diff --git a/drivers/net/e1000/em_rxtx.c > > b/drivers/net/e1000/em_rxtx.c > > index a6b3e92a6..81dc41efb 100644 > > --- a/drivers/net/e1000/em_rxtx.c > > +++ b/drivers/net/e1000/em_rxtx.c > > @@ -1416,12 +1416,13 @@ eth_em_rx_queue_setup(struct rte_eth_dev > > *dev, > > =C2=A0 } > >=20 > > =C2=A0 /* > > - =C2=A0* EM devices don't support drop_en functionality > > + =C2=A0* EM devices don't support drop_en functionality. > > + =C2=A0* It's an optimization that does nothing on single-queue > > devices, > > + =C2=A0* so just log the issue and carry on. > > =C2=A0 =C2=A0*/ > > =C2=A0 if (rx_conf->rx_drop_en) { > > - PMD_INIT_LOG(ERR, "drop_en functionality not > > supported > > by " > > + PMD_INIT_LOG(NOTICE, "drop_en functionality not > > supported by " > > =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"device"); > > - return -EINVAL; > > =C2=A0 } > >=20 > > =C2=A0 /* Free memory prior to re-allocation if needed. */ > > -- > > 2.18.0 >=20 >=20 --=20 Kind regards, Luca Boccassi