Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1473439 > unrolled thread

Re: [PATCH] debugfs: Add proxy function for the mmap file operation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] debugfs: Add proxy function for the mmap file operation Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-08-31 15:10 +0200
    Re: [PATCH] debugfs: Add proxy function for the mmap file operation Liviu Dudau <Liviu.Dudau@arm.com> - 2016-08-31 17:30 +0200
      Re: [PATCH] debugfs: Add proxy function for the mmap file operation Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-09-01 08:20 +0200
        Re: [PATCH] debugfs: Add proxy function for the mmap file operation Liviu Dudau <Liviu.Dudau@arm.com> - 2016-09-01 15:00 +0200
          Re: [PATCH] debugfs: Add proxy function for the mmap file operation Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-09-01 16:50 +0200

#1473439 — Re: [PATCH] debugfs: Add proxy function for the mmap file operation

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...

> 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,

greg k-h

[toc] | [next] | [standalone]

#1473613

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!  /
  ---------------
    ¯\_(ツ)_/¯

[toc] | [prev] | [next] | [standalone]

#1474076

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?

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.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1474387

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!  /
  ---------------
    ¯\_(ツ)_/¯

[toc] | [prev] | [next] | [standalone]

#1474442

On Thu, Sep 01, 2016 at 01:50:39PM +0100, Liviu Dudau wrote:
> 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.

So it's not just debugging?  This is a "real" thing that code will rely
on?  If so, that's not good, don't ever use debugfs for that.

good luck!

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web