linux-kernel - Re: [PATCH v5 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:   Wed, 5 Dec 2018 11:16:59 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     svendev@...x.com, robh+dt@...nel.org, linus.walleij@...aro.org,
        lee.jones@...aro.org, mark.rutland@....com, afaerber@...e.de,
        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, paul.gortmaker@...driver.com,
        sebastien.bourdelin@...oirfairelinux.com, icenowy@...c.io,
        stuyoder@...il.com, maxime.ripard@...tlin.com,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v5 1/6] fieldbus_dev: add Fieldbus Device subsystem.

On Tue, Dec 04, 2018 at 05:02:19PM -0500, Sven Van Asbroeck wrote:
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-fieldbus-dev
> @@ -0,0 +1,63 @@
> +What:		/sys/class/fieldbus_dev/fieldbus_devX/card_name

Here's a good example of your naming being a bit too "verbose" :)

There is no need to name your devices with the same prefix as your
class.  Just make it:
	/sys/class/fieldbus/XXX/card_name

And why is this a class and not just a "normal" device and bus?  Devices
live on busses, not generally as a class.  Can your devices live on
different types of busses (USB, PCI, etc.)?

But, if you really just want to leave this as a "class", then just call
it "fieldbus" and make it simple.  You are only exposing the
"fieldbus-specific" type of attributes here, so that's probably good
enough for now.

> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -187,4 +187,5 @@ obj-$(CONFIG_TEE)		+= tee/
>  obj-$(CONFIG_MULTIPLEXER)	+= mux/
>  obj-$(CONFIG_UNISYS_VISORBUS)	+= visorbus/
>  obj-$(CONFIG_SIOX)		+= siox/
> +obj-$(CONFIG_FIELDBUS_DEV)	+= fieldbus/
>  obj-$(CONFIG_GNSS)		+= gnss/

Why not after gnss?

> --- /dev/null
> +++ b/drivers/fieldbus/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for fieldbus_dev drivers.
> +#
> +
> +obj-$(CONFIG_FIELDBUS_DEV)	+= fieldbus_dev_core.o

Why not just "fieldbus.o"?

> +static ssize_t online_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct fieldbus_dev *fb = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",

Nit, sysfs attributes are always so small that you don't need to care
about the size of the buffer because you "always" know that you will not
overflow it.

So this can just be a simple:
	return sprintf(buf, "%s\n", ...);

> +		fb->online ? "online" : "offline");
> +}
> +static DEVICE_ATTR_RO(online);
> +
> +static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct fieldbus_dev *fb = dev_get_drvdata(dev);
> +
> +	if (!fb->enable_get)
> +		return -EINVAL;
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +		fb->enable_get(fb) ? ctrl_enabled : ctrl_disabled);
> +}

New line?

> +static ssize_t enabled_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t n)
> +{
> +	struct fieldbus_dev *fb = dev_get_drvdata(dev);
> +	int err;
> +
> +	if (!fb->simple_enable_set) {
> +		n = -ENOTSUPP;
> +	} else if (sysfs_streq(buf, ctrl_enabled)) {

Why not just have the normal "0/1" type of parsing?  We have a function
for it already, kstrtobool().

thanks,

greg k-h