Re: [PATCH v3 6/6] net/failsafe: fix removed device handling

[Matan Azrad <matan@mellanox.com>]

Hi Gaetan

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, December 20, 2017 12:22 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Thomas Monjalon
> <thomas@monjalon.net>; dev@dpdk.org
> Subject: Re: [PATCH v3 6/6] net/failsafe: fix removed device handling
> 
> Hi Matan,
> 
> On Tue, Dec 19, 2017 at 05:10:15PM +0000, Matan Azrad wrote:
> > 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.
> >
> > Fixes: a46f8d5 ("net/failsafe: add fail-safe PMD")
> > Fixes: b737a1e ("net/failsafe: support flow API")
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> 
> <snip>
> 
> > +/*
> > + * Check if error should be reported to the user.
> > + */
> > +static inline bool
> > +fs_is_error(struct sub_device *sdev, int err) {
> > +	/* A device removal shouldn't be reported as an error. */
> > +	if (err == 0 || sdev->remove == 1 || err == -EIO)
> > +		return false;
> > +	return true;
> > +}
> 
> This is better, thanks.
> 
> However is there a reason you did not follow the same pattern as ethdev
> with eth_err? I see the two functions as similar in their intent, making them
> close to each other would be clearer to a reader being familiar with the
> ethdev API and that would be interested in fail-safe.
> 
> What do you think?
> 

I think that there is a real different between eth_err function to fs_is_error:
ethdev uses eth_err function to adjust removal return value to be -EIO.
fail-safe uses fs_is_error function to check if an error should be reported to the user to save the fail-safe principle that the app should not be aware of device removal  -  this is the main idea that also causes me to change the name from fs_is_removed to fs_is_error.

> --
> Gaëtan Rivet
> 6WIND