VME: devices not removed after commit 050c3d52cc7

VME: devices not removed after commit 050c3d52cc7

[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
=====================================================================

Re: VME: devices not removed after commit 050c3d52cc7

[Paul Gortmaker]

[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
> =====================================================================

Re: VME: devices not removed after commit 050c3d52cc7

[Stefano Babic]

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
=====================================================================

Re: VME: devices not removed after commit 050c3d52cc7

[Martyn Welch]

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
> =====================================================================