[dpdk-dev] [PATCH v4 6/6] net/failsafe: fix removed device handling

[Gaëtan Rivet]

Hi Matan,

On Wed, Jan 10, 2018 at 12:43:33PM +0000, Matan Azrad wrote:
> Hi Gaetan
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Matan Azrad
> > Sent: Wednesday, January 10, 2018 2:31 PM
> > To: Thomas Monjalon <thomas at monjalon.net>; Gaetan Rivet
> > <gaetan.rivet at 6wind.com>
> > Cc: dev at dpdk.org
> > Subject: [dpdk-dev] [PATCH v4 6/6] net/failsafe: fix removed device handling
> > 
> > There is time between the physical removal of the device until sub-device
> > PMDs get a RMV interrupt. At this time DPDK PMDs and applications still
> > don't know about the removal and may call sub-device control operation
> > which should return an error.
> > 
> > In previous code this error is reported to the application contrary to fail-safe
> > principle that the app should not be aware of device removal.
> > 
> > Add an removal check in each relevant control command error flow and
> > prevent an error report to application when the sub-device is removed.
> > 
> > Signed-off-by: Matan Azrad <matan at mellanox.com>
> > ---
> >  drivers/net/failsafe/failsafe_flow.c    | 18 ++++++++++-------
> >  drivers/net/failsafe/failsafe_ops.c     | 35 ++++++++++++++++++++++--------
> > ---
> >  drivers/net/failsafe/failsafe_private.h | 11 +++++++++++
> >  3 files changed, 46 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/net/failsafe/failsafe_flow.c
> > b/drivers/net/failsafe/failsafe_flow.c
> > index 153ceee..c072d1e 100644
> > --- a/drivers/net/failsafe/failsafe_flow.c
> > +++ b/drivers/net/failsafe/failsafe_flow.c
> > @@ -87,7 +87,7 @@
> >  		DEBUG("Calling rte_flow_validate on sub_device %d", i);
> >  		ret = rte_flow_validate(PORT_ID(sdev),
> >  				attr, patterns, actions, error);
> > -		if (ret) {
> > +		if ((ret = fs_err(sdev, ret))) {
> This assignment in "if" statement causes to checkpatch error, I sent it as is because you asked it like this.
> If you think I need to change it, I see 2 options:
> 
> 1.
> ret = fs_err(sdev, ret);
> if (ret ) {...}
> 
> 2.
> if (fs_err(sdev, &ret)) {..}
> 
> what do you think?
> 

Yes I forgot that checkpatch was like this.
Our mail crossed, but I acked this patch. I think this is acceptable at
the driver level, or should be at the discretion of the driver
maintainer.

So personally, I'd say leave it this way. If someone or something shouts
about this we will consider alternatives. Otherwise this is readable and
easily understandable as-is.

Regards,
-- 
Gaëtan Rivet
6WIND