linux-kernel - Re: [PATCH v7 1/6] fieldbus_dev: add Fieldbus Device subsystem.

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 22 Jan 2019 19:52:46 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Andreas Färber <afaerber@...e.de>
Cc:     Paul Gortmaker <paul.gortmaker@...driver.com>,
        Sven Van Asbroeck <thesven73@...il.com>, svendev@...x.com,
        robh+dt@...nel.org, linus.walleij@...aro.org, lee.jones@...aro.org,
        mark.rutland@....com, treding@...dia.com, david@...hnology.com,
        noralf@...nnes.org, johan@...nel.org, monstr@...str.eu,
        michal.vokac@...ft.com, arnd@...db.de, john.garry@...wei.com,
        geert+renesas@...der.be, robin.murphy@....com,
        sebastien.bourdelin@...oirfairelinux.com, icenowy@...c.io,
        stuyoder@...il.com, maxime.ripard@...tlin.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 1/6] fieldbus_dev: add Fieldbus Device subsystem.

On Tue, Jan 22, 2019 at 06:12:31PM +0100, Andreas Färber wrote:
> Am 22.01.19 um 17:56 schrieb Paul Gortmaker:
> > [[PATCH v7 1/6] fieldbus_dev: add Fieldbus Device subsystem.] On 22/01/2019 (Tue 10:21) Sven Van Asbroeck wrote:
> > 
> >> Fieldbus device (client) adapters allow data exchange with a PLC aka.
> >> "Fieldbus Controller" over a fieldbus (Profinet, FLNet, etc.)
> >>
> >> They are typically used when a Linux device wants to expose itself
> >> as an actuator, motor, console light, switch, etc. over the fieldbus.
> >>
> >> This framework is designed to provide a generic interface to Fieldbus
> >> Devices from both the Linux Kernel and the userspace.
> >>
> >> Signed-off-by: Sven Van Asbroeck <TheSven73@...glemail.com>
> > 
> > [...]
> > 
> >> diff --git a/drivers/fieldbus/Kconfig b/drivers/fieldbus/Kconfig
> >> new file mode 100644
> >> index 000000000000..5c2bef950d04
> >> --- /dev/null
> >> +++ b/drivers/fieldbus/Kconfig
> >> @@ -0,0 +1,19 @@
> >> +menuconfig FIELDBUS_DEV
> >> +	bool "Fieldbus Device Support"
> > 
> > OK, so the core support is NOT tristate, ie not modular,
> 
> Is that intentional though, and if so, why?
> 
> If possible, I would much rather see it changed to tristate and leave
> all the metadata below.
> 
> > so I think...
> > 
> >> diff --git a/drivers/fieldbus/dev_core.c b/drivers/fieldbus/dev_core.c
> >> new file mode 100644
> >> index 000000000000..c816df3201bb
> >> --- /dev/null
> >> +++ b/drivers/fieldbus/dev_core.c
> >> @@ -0,0 +1,348 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Fieldbus Device Driver Core
> >> + *
> >> + */
> >> +
> >> +#include <linux/fieldbus_dev.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/module.h>
> > 
> > ...you don't need module.h here
> > 
> >> +#include <linux/device.h>
> >> +#include <linux/idr.h>
> >> +#include <linux/fs.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/poll.h>
> >> +
> > 
> > [...]
> > 
> >> +
> >> +static void __exit fieldbus_exit(void)
> >> +{
> >> +	unregister_chrdev_region(fieldbus_devt, MAX_FIELDBUSES);
> >> +	class_unregister(&fieldbus_class);
> >> +}
> >> +
> >> +subsys_initcall(fieldbus_init);
> >> +module_exit(fieldbus_exit);
> > 
> > ...and the module_exit is never called, so fieldbus_exit is dead code
> > and hence both should be removed.
> > 
> >> +
> >> +MODULE_AUTHOR("Sven Van Asbroeck <TheSven73@...glemail.com>");
> >> +MODULE_AUTHOR("Jonathan Stiles <jonathans@...x.com>");
> >> +MODULE_DESCRIPTION("Fieldbus Device Driver Core");
> >> +MODULE_LICENSE("GPL v2");
> 
> Sven, as this code appears to be all by ArcX people, any chance you can
> make it just GPL (SPDX GPL-2.0-or-later)? Only reason to have it v2-only

SPDX "GPL-2.0+" please.

> would be if this code was derived from v2-only code that can't be
> relicensed anymore.

Are you _sure_ you want v2+?  I ask as I am forced to :)

And I thought when I made the comment about this code being a module,
that would be the change, not this one forcing it not to be :(

thanks,

greg k-h