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]

Message-ID: <20181128174238.GA4367@kroah.com>
Date:   Wed, 28 Nov 2018 18:42:38 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     Sven Van Asbroeck <svendev@...x.com>, robh+dt@...nel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Lee Jones <lee.jones@...aro.org>, mark.rutland@....com,
        Andreas Färber <afaerber@...e.de>,
        treding@...dia.com, David Lechner <david@...hnology.com>,
        noralf@...nnes.org, johan@...nel.org,
        Michal Simek <monstr@...str.eu>, michal.vokac@...ft.com,
        Arnd Bergmann <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,
        Stuart Yoder <stuyoder@...il.com>, maxime.ripard@...tlin.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>
Subject: Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device
 subsystem.

On Wed, Nov 28, 2018 at 10:39:41AM -0500, Sven Van Asbroeck wrote:
> Wow Greg, thanks for the review, this is awesome !!
> 
> On Tue, Nov 27, 2018 at 2:54 AM Greg KH <gregkh@...uxfoundation.org> wrote:
> 
> >> +     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?
> 
> The proposed fieldbus API needs a single /dev/fieldbus_devX node for every
> device. I just looked around the drivers/ tree to see how others accomplish
> this.
> 
> Is there a better way?

It depends on what you want to do with this device node.  You can use a
static one in your structure if you use it to "cast back" to your real
structure in the open() call, do you do that?  If not, just create one
dynamically.

Or use a misc device?  How many of these do you need?

> >> +     fb->online_sd = sysfs_get_dirent(fb->dev->kobj.sd, "online");
> >
> > Ick, what?  Why?  Why are you messing around with a raw sysfs attribute?
> 
> The proposed fieldbus API has a sysfs attribute that can be poll/select'ed on.
> Is this behaviour still allowed / ok?

For an online/offline attribute, no need to poll on it, just do a
'kobject change' event for online/offline and all should be fine.  This
is not a high-frequency event, right?

> Now that I (hopefully) have a few seconds of your attention...
> I suppose the fieldbus API in this patch can't go anywhere, without buy-in from
> multiple people who also want to use fieldbus. Right now, there are none.

Hey, if no one wants to use it, either no need to write any code at all,
or you get to decide everything.  Either way, you're in charge!  :)

But you do need to document the thing, in Documentation/ABI/ to get it
correct and able to be reviewed.

thanks,

greg k-h