Re: [PATCH] pci: save list of detached devices, and re-probe during driver unload

[Bruce Richardson <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Wed, Mar 04, 2015 at 11:07:41AM +0200, Raz Amir wrote:
> Thank you.
> 
> See answers inline (mostly ack, but not only), and I will send the updated
> patch soon.
> 
>  
> 
> > -----Original Message-----
> 
> > From: Bruce Richardson [mailto:bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org]
> 
> > Sent: 03 March 2015 15:33
> 
> > To: Raz Amir
> 
> > Cc: dev-VfR2kkLFssw@public.gmane.org
> 
> > Subject: Re: [dpdk-dev] [PATCH] pci: save list of detached devices, and
> re-
> 
> > probe during driver unload
> 
> > 
> 
> > On Thu, Feb 26, 2015 at 06:33:20AM +0000, Raz Amir wrote:
> 
> > > Added code that saves the pointers to the detached devices, during
> 
> > > driver loading, and during driver unloading, go over the list, and
> 
> > > re-attach them by calling device_probe_and_attach on each device.
> 
> > >
> 
> > > Signed-off-by: Raz Amir < <mailto:razamir22-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> razamir22-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> > 
> 
> > Couple of minor comments below. Otherwise all looks good to me.
> 
> > 
> 
> > Acked-by: Bruce Richardson < <mailto:bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> > > ---
> 
> > >  lib/librte_eal/bsdapp/nic_uio/nic_uio.c | 26
> 
> > > +++++++++++++++++++++++++-
> 
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> > >
> 
> > > diff --git a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > index 5ae8560..7d702a5 100644
> 
> > > --- a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > +++ b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > @@ -55,6 +55,9 @@ __FBSDID("$FreeBSD$");
> 
> > >
> 
> > >  #define MAX_BARS (PCIR_MAX_BAR_0 + 1)
> 
> > >
> 
> > > +#define MAX_DETACHED_DEVICES               128
> 
> > > +static device_t detached_devices[MAX_DETACHED_DEVICES] = {}; static
> 
> > > +int last_detached = 0;
> 
> > Maybe num_detached/nb_detached or even just "detached" instead of
> 
> > "last_detached".
> 
> Ack.
> 
>  
> 
> > 
> 
> > >
> 
> > >  struct nic_uio_softc {
> 
> > >          device_t        dev_t;
> 
> > > @@ -291,14 +294,35 @@ nic_uio_load(void)
> 
> > >                          if (dev != NULL)
> 
> > 
> 
> > We are getting into some serious levels of indentation below, so maybe
> flip
> 
> > this condition around and put in a "continue" instead, so that we can
> dedent
> 
> > everything below that follows it.
> 
> > 
> 
> Ack.
> 
>  
> 
> > >                                          for (i = 0; i < NUM_DEVICES;
> i++)
> 
> > >                                                          if
> (pci_get_vendor(dev) == devices[i].vend
> 
> > &&
> 
> > > -
> pci_get_device(dev) ==
> 
> > devices[i].dev)
> 
> > > +
> pci_get_device(dev) ==
> 
> > devices[i].dev) {
> 
> > > +
> if (last_detached+1 <
> 
> > MAX_DETACHED_DEVICES) {
> 
> > I don't think you need the +1 here.
> 
> It is needed, otherwise the last object will be added at
> MAX_DETACHED_DEVICES position while the last position is
> MAX_DETACHED_DEVICES-1.

Yes, the last position is MAX_DETACHED_DEVICES-1, but you do the addition
of the element to the array using "detached_devices[last_detached++]", i.e. a
post-increment, so when last_detached == (MAX_DETACHED_DEVICES-1), you still
can fill in an entry. Next time around, when last_detached == MAX_DETACHED_DEVICES
it's no longer safe to add, and the condition "last_detached < MAX_DETACHED_DEVICES)
will now fail. No +1 or -1 necessary to prevent this.

/Bruce