Date:   Fri, 10 Dec 2021 15:39:44 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     David Brazdil <dbrazdil@google.com>
Cc:     Rob Herring <robh+dt@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
        Derek Kiernan <derek.kiernan@xilinx.com>,
        Dragan Cvetic <dragan.cvetic@xilinx.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Hans de Goede <hdegoede@redhat.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, Andrew Scull <ascull@google.com>,
        Will Deacon <will@kernel.org>
Subject: Re: [PATCH v2 2/2] misc: dice: Add driver to forward secrets to
 userspace
Message-ID: <YbNmsFAYDVUYopFO@kroah.com>
References: <20211209151123.3759999-1-dbrazdil@google.com>
 <20211209151123.3759999-3-dbrazdil@google.com>
 <YbIhaWC8b2DV5C7Y@kroah.com>
 <YbM29thQ7U4oUmhi@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YbM29thQ7U4oUmhi@google.com>
Precedence: bulk

On Fri, Dec 10, 2021 at 11:16:06AM +0000, David Brazdil wrote:
> On Thu, Dec 09, 2021 at 04:31:53PM +0100, Greg Kroah-Hartman wrote:
> > What is the module name, please add that here.
> > 
> > And "dice" is a very generic name.  I don't mind, but if you want to
> > name it a bit more specific, that might be better.
> Does "open-dice" sound good? I think that's the shorthand used on the
> official website.

That might be better.

Naming is hard.

> > > +static long dice_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > > +{
> > > +	switch (cmd) {
> > > +	case DICE_GET_SIZE:
> > > +		/* Checked against INT_MAX in dice_probe(). */
> > > +		return dice_rmem->size;
> > > +	case DICE_WIPE:
> > > +		return dice_wipe();
> > > +	}
> > > +
> > > +	return -ENOIOCTLCMD;
> > 
> > -ENOTTY please.
> I have no personal attachment to ENOIOCTLCMD, but it is documented as
> "no ioctl command" and converted to ENOTTY before returning to userspace.
> That made me think this was the right thing to do.

ENOTTY is better please.

> > As you only have 2 ioctls, why not just use read/write for this?  Write
> > would cause dice_wipe() to happen, and read would return the size in the
> > buffer provided.  Then no ioctl is needed at all.
> Fine by me but does feel like a bit of a hack. Is that a common pattern?

ioctls are hacks too :)

read/write like this is fine to do, might make the code simpler, and
allow the code to be used by scripts easier.  At the very least, wipe
can be done by any language instead of only those that allow ioctls.

thanks,

greg k-h