linux-kernel - Re: [PATCH anybus v4 1/7] 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, 27 Nov 2018 08:54:31 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     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 anybus v4 1/7] fieldbus_dev: add Fieldbus Device
 subsystem.

On Wed, Nov 21, 2018 at 10:07:03AM -0500, thesven73@...il.com wrote:
> +static struct attribute *fieldbus_attrs[] = {
> +	&dev_attr_enabled.attr,
> +	&dev_attr_card_name.attr,
> +	&dev_attr_fieldbus_id.attr,
> +	&dev_attr_read_area_size.attr,
> +	&dev_attr_write_area_size.attr,
> +	&dev_attr_online.attr,
> +	&dev_attr_fieldbus_type.attr,
> +	NULL,
> +};
> +
> +static umode_t fieldbus_is_visible(struct kobject *kobj, struct attribute *attr,
> +				int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct fieldbus_dev *fb = dev_get_drvdata(dev);
> +	umode_t mode = attr->mode;
> +
> +	if (attr == &dev_attr_enabled.attr) {
> +		mode = 0;
> +		if (fb->enable_get)
> +			mode |= 0444;
> +		if (fb->simple_enable_set)
> +			mode |= 0200;
> +	}
> +
> +	return mode;
> +}
> +
> +static const struct attribute_group fieldbus_group = {
> +	.attrs = fieldbus_attrs,
> +	.is_visible = fieldbus_is_visible,
> +};
> +__ATTRIBUTE_GROUPS(fieldbus);

Why not just use ATTRIBUTE_GROUPS()?

> +static int __fieldbus_dev_register(struct fieldbus_dev *fb)
> +{
> +	dev_t devno;
> +	int err;
> +
> +	if (!fb)
> +		return -EINVAL;
> +	if (!fb->read_area || !fb->write_area || !fb->fieldbus_id_get)
> +		return -EINVAL;
> +	fb->id = ida_simple_get(&fieldbus_ida, 0, MAX_FIELDBUSES, GFP_KERNEL);
> +	if (fb->id < 0)
> +		return fb->id;
> +	devno = MKDEV(MAJOR(fieldbus_devt), fb->id);
> +	init_waitqueue_head(&fb->dc_wq);
> +	cdev_init(&fb->cdev, &fieldbus_fops);
> +	err = cdev_add(&fb->cdev, devno, 1);
> +	if (err) {
> +		pr_err("fieldbus_dev%d unable to add device %d:%d\n",
> +			fb->id, MAJOR(fieldbus_devt), fb->id);
> +		goto err_cdev;
> +	}

Why do you have a static cdev?

> +	fb->dev = device_create(&fieldbus_class, fb->parent, devno, fb,
> +				"fieldbus_dev%d", fb->id);
> +	if (IS_ERR(fb->dev)) {
> +		err = PTR_ERR(fb->dev);
> +		goto err_dev_create;
> +	}
> +	fb->online_sd = sysfs_get_dirent(fb->dev->kobj.sd, "online");

Ick, what?  Why?  Why are you messing around with a raw sysfs attribute?

Also, you are creating sysfs files and you are not documenting any of
them in Documentation/ABI/ which is not allowed.  Please add that to
this patch for the next round.

thanks,

greg k-h