* VME: devices not removed after commit 050c3d52cc7
@ 2017-01-13 10:03 Stefano Babic
2017-01-13 15:39 ` Paul Gortmaker
0 siblings, 1 reply; 4+ messages in thread
From: Stefano Babic @ 2017-01-13 10:03 UTC (permalink / raw)
To: paul.gortmaker; +Cc: linux-kernel, stefano babic
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
In fact, this drops the remove function, that scans all devices attached
to the bus and call their remove function.
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.
Paul, what do you mind ?
Best regards,
Stefano Babic
--
=====================================================================
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
=====================================================================
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: VME: devices not removed after commit 050c3d52cc7 2017-01-13 10:03 VME: devices not removed after commit 050c3d52cc7 Stefano Babic @ 2017-01-13 15:39 ` Paul Gortmaker 2017-01-13 16:28 ` Stefano Babic 0 siblings, 1 reply; 4+ messages in thread From: Paul Gortmaker @ 2017-01-13 15:39 UTC (permalink / raw) To: Stefano Babic; +Cc: linux-kernel, Martyn Welch [Adding Martyn to Cc] [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. > > > 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. The above commit you reference was based on the premise that removal of a VME bus is not supported. Which is not to say that a VME device removal is not supported. > > 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). > > 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, 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. Maybe Martyn can spot where I've misunderstood the bus vs. device separation here. Paul. -- > > Best regards, > Stefano Babic > > > -- > ===================================================================== > 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 > ===================================================================== ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: VME: devices not removed after commit 050c3d52cc7 2017-01-13 15:39 ` Paul Gortmaker @ 2017-01-13 16:28 ` Stefano Babic 2017-01-18 21:32 ` Martyn Welch 0 siblings, 1 reply; 4+ messages in thread From: Stefano Babic @ 2017-01-13 16:28 UTC (permalink / raw) To: Paul Gortmaker, Stefano Babic; +Cc: linux-kernel, Martyn Welch Hi Paul, On 13/01/2017 16:39, Paul Gortmaker wrote: > [Adding Martyn to Cc] > Sorry, I forgot to run get_maintainer before posting :-) > [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. > >> >> >> 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 ===================================================================== ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: VME: devices not removed after commit 050c3d52cc7 2017-01-13 16:28 ` Stefano Babic @ 2017-01-18 21:32 ` Martyn Welch 0 siblings, 0 replies; 4+ messages in thread From: Martyn Welch @ 2017-01-18 21:32 UTC (permalink / raw) To: Stefano Babic; +Cc: Paul Gortmaker, linux-kernel 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 > ===================================================================== ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-18 21:40 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-13 10:03 VME: devices not removed after commit 050c3d52cc7 Stefano Babic 2017-01-13 15:39 ` Paul Gortmaker 2017-01-13 16:28 ` Stefano Babic 2017-01-18 21:32 ` Martyn Welch
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.