Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH 8/9] surface_aggregator: Add DebugFS interface
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
        =?UTF-8?Q?Bla=c5=be_Hrastnik?= <blaz@mxxn.io>,
        Dorian Stoll <dorian.stoll@tmsp.io>
References: <20200923151511.3842150-1-luzmaximilian@gmail.com>
 <20200923151511.3842150-9-luzmaximilian@gmail.com>
 <20200923161416.GA3723109@kroah.com>
From:   Maximilian Luz <luzmaximilian@gmail.com>
Message-ID: <7d571ed4-862e-cfbd-44d4-0fda25f03294@gmail.com>
Date:   Wed, 23 Sep 2020 20:03:38 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20200923161416.GA3723109@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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