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;
Sender: sidgup=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 95F0DC433ED
Subject: Re: PROBLEM: Firmware loader fallback mechanism no longer works with
 sendfile
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     mcgrof@kernel.org, rafael@kernel.org, viro@zeniv.linux.org.uk,
        linux-fsdevel@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "psodagud@codeaurora.org" <psodagud@codeaurora.org>
References: <7e6f44b1-a0d2-d1d1-9c11-dcea163f8f03@codeaurora.org>
 <X/QJCgoLPhfECEmP@kroah.com>
 <180bdfaf-8c84-6946-b46f-3729d4eb17cc@codeaurora.org>
 <X/WSA7nmsUSrpsfr@kroah.com>
 <62583aaa-d557-8c9a-5959-52c9efad1fe3@codeaurora.org>
 <X/hv634I9JOoHZRk@kroah.com>
 <1adf9aa4-ed7e-8f05-a354-57419d61ec18@codeaurora.org>
 <YAHAzW1cVSaMzBYj@kroah.com>
From:   Siddharth Gupta <sidgup@codeaurora.org>
Message-ID: <be2ae542-b543-173d-d256-b4203aad7756@codeaurora.org>
Date:   Sun, 17 Jan 2021 16:59:52 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
MIME-Version: 1.0
In-Reply-To: <YAHAzW1cVSaMzBYj@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 1/15/2021 8:20 AM, Greg KH wrote:
> On Tue, Jan 12, 2021 at 10:31:26AM -0800, Siddharth Gupta wrote:
>> On 1/8/2021 6:44 AM, Greg KH wrote:
>>> On Thu, Jan 07, 2021 at 02:03:47PM -0800, Siddharth Gupta wrote:
>>>> On 1/6/2021 2:33 AM, Greg KH wrote:
>>>>>>>> Since the binary attributes don't support splice_{read,write} functions the
>>>>>>>> calls to splice_{read,write} used the default kernel_{read,write} functions.
>>>>>>>> With the above change this results in an -EINVAL return from
>>>>>>>> do_splice_from[4].
>>>>>>>>
>>>>>>>> This essentially means that sendfile will not work for any binary attribute
>>>>>>>> in the sysfs.
>>>>>>> Have you tried fixing this with a patch much like what we did for the
>>>>>>> proc files that needed this?  If not, can you?
>>>>>> I am not aware of this fix, could you provide me a link for reference? I
>>>>>> will try it out.
>>>>> Look at the series of commits starting at fe33850ff798 ("proc: wire up
>>>>> generic_file_splice_read for iter ops") for how this was fixed in procfs
>>>>> as an example of what also needs to be done for binary sysfs files.
>>>> I tried to follow these fixes, but I am unfamiliar with fs code. I don't see
>>>> the generic_file_splice_write function anymore on newer kernels, also AFAICT
>>>> kernfs_ops does not define {read,write}_iter operations. If the solution is
>>>> simple and someone could provide the patches I would be happy to test them
>>>> out. If not, some more information about how to proceed would be nice.
>>> Can you try this tiny patch out below?
>> Sorry for the delay, I tried out the patch, but I am still seeing the error.
>> Please take a look at these logs with
>> android running in the userspace[1]:
>>
>> [   62.295056][  T249] remoteproc remoteproc1: powering up
>> xxxxxxxx.remoteproc-cdsp
>> [   62.304138][  T249] remoteproc remoteproc1: Direct firmware load for
>> cdsp.mdt failed with error -2
>> [   62.312976][  T249] remoteproc remoteproc1: Falling back to sysfs
>> fallback for: cdsp.mdt
>> [   62.469748][  T394] ueventd: firmware: loading 'cdsp.mdt' for '/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt'
>> [   62.498700][  T394] ueventd: firmware: sendfile failed { '/sys/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt',
>> 'cdsp.mdt' }: Invalid argument
>>
>> Thanks,
>> Sid
>>
>> [1]: https://android.googlesource.com/platform/system/core/+/refs/heads/master/init/firmware_handler.cpp#57
> Thanks for letting me know.  I'll try to work on this on Monday and add
> splice to the in-kernel firmware testing suite, as it would have caught
> this...
If my understanding is correct these default (or generic if you prefer)
functions require the write_iter[1] and read_iter[2] file ops to be 
defined for
kernfs_file_fops. Are there any similar default {write,read}_iter 
functions that
I can use, or do these need to be thin wrappers for the read function[3] 
for bin
attributes itself?

Thanks,
Sid

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/splice.c#n686
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/splice.c#n311
[3]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/sysfs/file.c#n235
> greg k-h