On Wed, Sep 23, 2020 at 08:03:38PM +0200, Maximilian Luz wrote:
> On 9/23/20 6:14 PM, Greg Kroah-Hartman wrote:
> > On Wed, Sep 23, 2020 at 05:15:10PM +0200, Maximilian Luz wrote:
> [...]
>
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> >
> > Are you sure about -or-later? I have to ask.
>
> Fairly, unless there are any complications with integration of this code
> that I'm not aware of.
Nope, just have to ask :)
> > > +out:
> > > + // always try to set response-length and status
> > > + tmp = put_user(rsp.length, &r->response.length);
> > > + if (!ret)
> > > + ret = tmp;
> >
> > Is that the correct error to return if put_user() fails? Hint, I don't
> > think so...
>
> So the -EFAULT returned by put_user should have precedence? I was aiming
> for "in case it fails, return with the first error".
-EFAULT trumps everything :)
>
> [...]
>
> > > +static long ssam_dbg_device_ioctl(struct file *file, unsigned int cmd,
> > > + unsigned long arg)
> > > +{
> > > + switch (cmd) {
> > > + case SSAM_DBG_IOCTL_GETVERSION:
> > > + return ssam_dbg_if_getversion(file, arg);
> >
> > Not needed, please drop.
> >
> > > +
> > > + case SSAM_DBG_IOCTL_REQUEST:
> > > + return ssam_dbg_if_request(file, arg);
> > > +
> > > + default:
> > > + return -ENOIOCTLCMD;
> >
> > Wrong error value.
>
> I assume -ENOTTY would be correct/preferred then? Kernel doc suggests
> that either one of the two would be correct and essentially result in
> the same behavior.
-ENOTTY is the correct one, it will be turned into a different value
right before it gets back to userspace.
> > Listen, I'm all for doing whatever you want in debugfs, but why are you
> > doing random ioctls here? Why not just read/write a file to do what you
> > need/want to do here instead?
>
> Two reasons, mostly: First, the IOCTL allows me to execute requests in
> parallel with just one open file descriptor and not having to maintain
> some sort of back-buffer to wait around until the reader gets to reading
> the thing. I've used that for stress-testing the EC communication in the
> past, which had some issues (dropping bytes, invalid CRCs, ...) under
> heavy(-ish) load. Second, I'm considering adding support for events to
> this device in the future by having user-space receive events by reading
> from the device. Events would also be enabled or disabled via an IOCTL.
> That could be implemented in a second device though. Events were also my
> main reason for adding a version to this interface: Discerning between
> one that has event support and one that has not.
A misc device can also do this, much simpler, right? Why not use that?
Oh, after fixing up the issues that Arnd pointed out of course :)
> > > +static void ssam_dbg_device_release(struct device *dev)
> > > +{
> > > + // nothing to do
> >
> > That's a lie, and the old documentation would allow me to make fun of
> > you for trying to work around the kernel's error messages here.
> >
> > But I'll be nice and just ask, why do you think it is ok to work around
> > a message that someone has spent a lot of time and energy to provide to
> > you, saying that you are doing something wrong, by ignoring that and
> > providing an empty function? Not kind...
>
> Sorry about that, but may get a pointer to that particular message? This
> setup has been pretty much copied from existing kernel drivers (see
> /drivers/platform/x86/intel_pmc_core_pltdrv.c for one)
Oh wow, time to go yell at people, thanks for pointing that out...
> and I thought
> that I can get around having to dynamically allocate a platform device
> since it's guaranteed to be only there once.
>
> There was no workaround or unkindness of any sorts intended.
See device_release() in drivers/base/core.c for the error message you
would have gotten that this empty function "works around".
> > > +}
> > > +
> > > +static struct platform_device ssam_dbg_device = {
> > > + .name = SSAM_DBG_DEVICE_NAME,
> > > + .id = PLATFORM_DEVID_NONE,
> > > + .dev.release = ssam_dbg_device_release,
> > > +};
> >
> > Dynamic structures that are static are, well, wrong :)
>
> I assume the correct way would be to allocate the device dynamically and
> this holds for all devices?
>
> Sorry if I'm asking such basic questions, but I have not found anything
> regarding this in the documentation, although I have to confess that I
> only skimmed over a larger part, so that's very likely my fault.
>
> > I appreciate the initiative by creating a fake platform device and
> > driver to bind to that device. But I don't think any of it is needed at
> > all, you have made your work a lot harder than you needed to here. This
> > whole file can be _much_ smaller and simpler and not abuse the kernel
> > apis so badly :)
>
> So just tack it onto the core driver? My intention was to keep it a bit
> more separate from the core, but adding it directly would indeed reduce
> the amount of code.
A simple misc device would make it very simple and easy to do instead,
why not do that?
thanks,
greg k-h
