linux-kernel - Re: VME: devices not removed after commit 050c3d52cc7

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <19155f10-7190-0cf6-6ea7-43956dcfc173@denx.de>
Date:   Fri, 13 Jan 2017 17:28:37 +0100
From:   Stefano Babic <sbabic@...x.de>
To:     Paul Gortmaker <paul.gortmaker@...driver.com>,
        Stefano Babic <sbabic@...x.de>
Cc:     linux-kernel@...r.kernel.org,
        Martyn Welch <martyn.welch@...labora.co.uk>
Subject: Re: VME: devices not removed after commit 050c3d52cc7

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@...driver.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@...x.de
=====================================================================