Re: VME: devices not removed after commit 050c3d52cc7

[Martyn Welch <martyn.welch@collabora.co.uk>]

On Fri, Jan 13, 2017 at 05:28:37PM +0100, Stefano Babic wrote:
> Hi Paul,
> 
> On 13/01/2017 16:39, Paul Gortmaker wrote:
> > [Adding Martyn to Cc]
> > 
> 
> Sorry, I forgot to run get_maintainer before posting :-)
> 

No worries, and sorry for the delay, this ended up in my spam filter :-/

> > [VME: devices not removed after commit 050c3d52cc7] On 13/01/2017 (Fri 11:03) Stefano Babic wrote:
> > 
> >> Hi,
> >>
> >> I have updated a custom VME device driver (mainly based on vme_user.c)
> >> to 4.9 (previously it was for 3.14-).
> >>
> >> I see that VME device drivers cannot be loaded and unloaded due to this
> >> commit:
> >>
> >> commit 050c3d52cc7810d9d17b8cd231708609af6876ae
> >> Author: Paul Gortmaker <paul.gortmaker@windriver.com>
> >> Date:   Sun Jul 3 14:05:56 2016 -0400
> >>
> >>     vme: make core vme support explicitly non-modular
> > 
> > I've gone back and looked at this, and vme_user.c and I'm not yet 100%
> > convinced this is the right conclusion.  But perhaps, and I've put
> > Martyn on the Cc, in the hopes that he can clarify as well, if needed.
> 
> Thanks. What I am seeing is that (*remove) in bus_type is called when a
> device is removed from the bus, and not when the bus is removed. This
> looks consistent with other busses.  And in fact, the function was:
> 
> static int vme_bus_remove(struct device *dev)
> {
>        int retval = -ENODEV;
>        struct vme_driver *driver;
>        struct vme_dev *vdev = dev_to_vme_dev(dev);
> 
>        driver = dev->platform_data;
> 
>        if (driver->remove != NULL)
>                retval = driver->remove(vdev);
> 
> So this is the point where the remove for the VME's device is called, as
> far as I understand.
> 
> 

Yup, I agree - we need to re-instate this function, it's called when a
device driver is removed.

Thanks for noticing this, it's been a while since I wrote this code and
the patch /looked/ sane...

Martyn

> > 
> >>
> >>
> >> In fact, this drops the remove function, that scans all devices attached
> >> to the bus and call their remove function.
> > 
> > So I guess my confusion here is between removal of a VME device, vs. the
> > removal of a complete VME bus.
> 
> Right, this is what must be cleared. In my understanding, the dropped
> remove function is called when a device is removed from the bus, that
> leads to the fact that the VME's device is not cleaned unloaded.
> 
> >  The above commit you reference was based
> > on the premise that removal of a VME bus is not supported.
> 
> Agree, and I fully agree that loading / unloading of VME makes less sense.
> 
> >  Which is not
> > to say that a VME device removal is not supported.
> 
> I agree to reach this goal - just the dropped remove() is called IMHO
> when a device is dropped from the VME bus and not when the bus is
> removed from system. This is what we need to clarify here.
> 
> > 
> >>
> >> That means that "remove" entry points in VME device driver (let see in
> >> drivers/staging/vme/devices/vme_user.c) are now dead code and the
> >> required cleanup code is not called at all (devices and class are not
> >> removed). Reloading the same driver cause errors due to the missing
> >> cleanup by unloading.  This does not let build VME device drivers as
> >> module, as it is supposed to be done.
> > 
> > Again, I don't think this analysis is 100% right, but I can't be sure
> > because your driver is out of tree and I don't know what it does
> > precisely.  Looking at vme_user.c example, it has its own .remove
> > function that should be executed at module unload, and that would do all
> > the cleanup (see vme_user_remove).
> 
> In my test, vme_user's remove is never called with the patch applied.
> Reverting the patch, it works again, and remove is called: loading /
> unloading of VME's device drivers works again.
> 
> > 
> >>
> >> Paul, what do you mind ?
> > 
> > For sure, we can restore the .remove and vme_bus_remove portions of that
> > commit if it is a real regression against a correct use of the
> > infrastructure,
> 
> I absolutely agree that we have to clarify the point before doing something.
> 
> > but I'm still not clear how you'd be triggering the
> > vme_bus_remove unless the vme device driver was going up into its
> > parent's bus struct directly.
> 
> No, this is not done !
> 
> >  Maybe Martyn can spot where I've
> > misunderstood the bus vs. device separation here.
> > 
> 
> 
> Best regards,
> Stefano
> 
> 
> 
> -- 
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================