| Message ID | 20160729143459.2672-1-Liviu.Dudau@arm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 31, 2016 at 03:07:49PM +0200, Greg Kroah-Hartman wrote: > On Fri, Aug 05, 2016 at 01:11:45PM +0200, Nicolai Stange wrote: > > Brian Starkey <brian.starkey@arm.com> writes: > > > > > On Tue, Aug 02, 2016 at 07:31:36PM +0200, Nicolai Stange wrote: > > >>Nicolai Stange <nicstange@gmail.com> writes: > > >>> However, if you wish to have some mmapable debugfs file which *can* go > > >>> away, introducing mmap support in the debugfs full proxy is perfectly > > >>> valid. But please see below. > > >> > > >>Assuming that you've got such a use case, please consider resending your > > >>patch along with the Cocci script below (and the Coccinelle team CC'ed, > > >>of course). If OTOH your mmapable debugfs files are never removed, just > > >>drop this message and use debugfs_create_file_unsafe() instead. > > > > > > So we do have an implementation using this, but it's likely we will > > > keep it out-of-tree (it's a stop-gap until we can get a non-debugfs > > > implementation of the functionality into mainline). > > > > > > Do you think it's worth merging this (and your cocci script) anyway to > > > save someone else doing the same thing later? > > > > I personally think that having ->mmap() support in debugfs would be a > > good thing to have in general and I expect there to be some further > > demand in the future. > > Ugh, mmap in debugfs, that's funny. And sad... Yeah. While our need for the mmap-ing the debugfs entry is at best a temporary option and a hack, I would be interested to know what alternatives could be used to read a large amount of data that does not need the seq_operations API? The out-of-tree proof-of-concept code that we have to interact with a memory write engine needs to be able to access the output buffer from userspace, but that output buffer is created by the kernel KMS driver. > > > But I also think that it is a little bit fragile in the current state: > > how many people actually run the Cocci scripts on their changes? AFAICT, > > even the kbuild test robot doesn't do this. And after all, the Cocci > > script I provided could very well miss some obfuscated writes to > > vma->vm_ops: if they aren't done from ->mmap() themselves, but from some > > helper function invoked therein, for example. > > > > I would personally prefer a hand coded full_proxy_mmap() which WARN()s > > if the proxied ->mmap() changes vma->vm_ops: > > - this would add an extra safety net > > - ->mmap() for debugfs files isn't performance critical > > - and lastly, we're already doing something similar to this in > > open_proxy_open(). > > Yes, that would be the best thing to do here. Thanks a lot for the feedback and specially to Nicolai for the provided Cocci script! Sorry for not replying earlier, I went on a long holiday and just returned. Best regards, Liviu > > thanks, > > greg k-h > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯
On Thu, Sep 01, 2016 at 08:19:33AM +0200, Greg Kroah-Hartman wrote: > On Wed, Aug 31, 2016 at 04:23:52PM +0100, Liviu Dudau wrote: > > On Wed, Aug 31, 2016 at 03:07:49PM +0200, Greg Kroah-Hartman wrote: > > > On Fri, Aug 05, 2016 at 01:11:45PM +0200, Nicolai Stange wrote: > > > > Brian Starkey <brian.starkey@arm.com> writes: > > > > > > > > > On Tue, Aug 02, 2016 at 07:31:36PM +0200, Nicolai Stange wrote: > > > > >>Nicolai Stange <nicstange@gmail.com> writes: > > > > >>> However, if you wish to have some mmapable debugfs file which *can* go > > > > >>> away, introducing mmap support in the debugfs full proxy is perfectly > > > > >>> valid. But please see below. > > > > >> > > > > >>Assuming that you've got such a use case, please consider resending your > > > > >>patch along with the Cocci script below (and the Coccinelle team CC'ed, > > > > >>of course). If OTOH your mmapable debugfs files are never removed, just > > > > >>drop this message and use debugfs_create_file_unsafe() instead. > > > > > > > > > > So we do have an implementation using this, but it's likely we will > > > > > keep it out-of-tree (it's a stop-gap until we can get a non-debugfs > > > > > implementation of the functionality into mainline). > > > > > > > > > > Do you think it's worth merging this (and your cocci script) anyway to > > > > > save someone else doing the same thing later? > > > > > > > > I personally think that having ->mmap() support in debugfs would be a > > > > good thing to have in general and I expect there to be some further > > > > demand in the future. > > > > > > Ugh, mmap in debugfs, that's funny. And sad... > > > > Yeah. > > > > While our need for the mmap-ing the debugfs entry is at best a temporary > > option and a hack, I would be interested to know what alternatives could > > be used to read a large amount of data that does not need the seq_operations > > API? The out-of-tree proof-of-concept code that we have to interact with > > a memory write engine needs to be able to access the output buffer from > > userspace, but that output buffer is created by the kernel KMS driver. > > What type of debugging do you need this for? Taking snapshots of a composition scene using the KMS driver for Mali DP. > > A binary sysfs attribute also might work well for you, on the device > that you are talking to, but if not, yeah, mmap on debugfs will work > just fine, seems to be the best fit. We felt sysfs gives a whiff of official support for the feature, while in reality is a stop gap until we work out the V4L2 functionality to do the same thing. Best regards, Liviu > > thanks, > > greg k-h > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index 592059f..d87148a 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -168,6 +168,10 @@ FULL_PROXY_FUNC(write, ssize_t, filp, loff_t *ppos), ARGS(filp, buf, size, ppos)); +FULL_PROXY_FUNC(mmap, int, filp, + PROTO(struct file *filp, struct vm_area_struct *vma), + ARGS(filp, vma)); + FULL_PROXY_FUNC(unlocked_ioctl, long, filp, PROTO(struct file *filp, unsigned int cmd, unsigned long arg), ARGS(filp, cmd, arg)); @@ -224,6 +228,8 @@ static void __full_proxy_fops_init(struct file_operations *proxy_fops, proxy_fops->write = full_proxy_write; if (real_fops->poll) proxy_fops->poll = full_proxy_poll; + if (real_fops->mmap) + proxy_fops->mmap = full_proxy_mmap; if (real_fops->unlocked_ioctl) proxy_fops->unlocked_ioctl = full_proxy_unlocked_ioctl; }
Add proxy function for the mmap file_operations hook under the full_proxy_fops structure. This is useful for providing a custom mmap routine in a driver's debugfs implementation. Cc: Nicolai Stange <nicstange@gmail.com> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> --- fs/debugfs/file.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.9.0