Messages in this thread | | | Subject | Re: [RFC PATCH 8/9] surface_aggregator: Add DebugFS interface | From | Maximilian Luz <> | Date | Wed, 23 Sep 2020 20:03:38 +0200 |
| |
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.
> And no copyright line?
Forgot to add that, sorry. Will add it for the next version. That's also the case for all other files.
[...]
>> + >> +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".
[...]
>> +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.
[...]
> 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.
> > And again, no versioning, that is never needed. >
Got it, will drop that.
[...]
>> +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) 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.
>> +} >> + >> +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.
Thanks, Max
| |