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

Groups > linux.kernel > #1628453 > unrolled thread

Re: bug fix for registers debugfs file implementation [RFC]

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: bug fix for registers debugfs file implementation [RFC] Mark Brown <broonie@kernel.org> - 2017-04-21 20:40 +0200
    Re: bug fix for registers debugfs file implementation [RFC] Variksla <variksla@gmail.com> - 2017-04-22 22:30 +0200
      Re: bug fix for registers debugfs file implementation [RFC] Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-04-24 11:10 +0200
        Re: bug fix for registers debugfs file implementation [RFC] Mark Brown <broonie@kernel.org> - 2017-04-24 14:10 +0200

#1628453 — Re: bug fix for registers debugfs file implementation [RFC]

[Multipart message — attachments visible in raw view] — view raw

On Sat, Apr 01, 2017 at 02:13:41AM -0700, noman pouigt wrote:
> Current registers debugfs file implementation doesn't
> handle if the size exceeds 4k. It just dumps 4k of registers.
> Fix this by using the seq_file which already handles
> the file offset logic instead of reinventing the wheel.
> 
> I am wondering if there is any issue is doing below which
> I am not aware of?

If I remember correctly this is done the way it is because seq_file has
to iterate through the entire file to get to the point being read by the
application.  This is a *very* big overhead for some applications (like
monitoring some registers to see what they're doing) on bigger devices,
especially if there's lots of uncached stuff and the reads have to go to
the hardware.  With the current code you can open the file, seek to the
registers you're interested in and read only them.  You'll notice that
the other debug files have been converted over to seq_file as they're
pure software and there's less reason to repeatedly read them.

I'm also very surprised that this is failing for you as I know this code
has been fairly heavily exercised with devices with very large register
maps much larger than 4k and my own testing is mainly doing cats which
seemed to work last time I tried and should be iterating over the 4k
boundary...  what's the actual failure mode?  I'm guessing it's
something if we end a register exactly on a page boundary or something?

Also your patch is completely tab/space mangled so it's not appliable.

> +static void regmap_debugfs_stop(struct seq_file *s, void *v)
> +{
> +}

The need for empty functions is worrying...

[toc] | [next] | [standalone]

#1628880

> On Apr 21, 2017, at 10:27 AM, Mark Brown <broonie@kernel.org> wrote:

Thanks for the response.
> 
>> On Sat, Apr 01, 2017 at 02:13:41AM -0700, noman pouigt wrote:
>> Current registers debugfs file implementation doesn't
>> handle if the size exceeds 4k. It just dumps 4k of registers.
>> Fix this by using the seq_file which already handles
>> the file offset logic instead of reinventing the wheel.
>> 
>> I am wondering if there is any issue is doing below which
>> I am not aware of?
> 
> If I remember correctly this is done the way it is because seq_file has
> to iterate through the entire file to get to the point being read by the
> application.  This is a *very* big overhead for some applications (like
> monitoring some registers to see what they're doing) on bigger devices,

Wondering why would the user space application be monitoring the registers?
> especially if there's lots of uncached stuff and the reads have to go to
> the hardware.  With the current code you can open the file, seek to the
> registers you're interested in and read only them.  You'll notice that
> the other debug files have been converted over to seq_file as they're
> pure software and there's less reason to repeatedly read them.

Yes. I noticed that and that is why I realized that I am doing something wrong.
> 
> I'm also very surprised that this is failing for you as I know this code
> has been fairly heavily exercised with devices with very large register
> maps much larger than 4k and my own testing is mainly doing cats which
> seemed to work last time I tried and should be iterating over the 4k
> boundary...  what's the actual failure mode?  I'm guessing it's

I have integrated Florida codec(wm8281) from cirrus and it has more than 4K registers. I could not dump more than 4K with the existing interface.

Charles, are you able to dump all the registers?

> something if we end a register exactly on a page boundary or something?
> 
> Also your patch is completely tab/space mangled so it's not appliable.
> 
>> +static void regmap_debugfs_stop(struct seq_file *s, void *v)
>> +{
>> +}
> 
> The need for empty functions is worrying...

In several other places in kernel code the usage is similar.

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

#1629322

On Sat, Apr 22, 2017 at 01:28:15PM -0700, Variksla wrote:
> 
> 
> > On Apr 21, 2017, at 10:27 AM, Mark Brown <broonie@kernel.org> wrote:
> 
> Thanks for the response.
> > 
> >> On Sat, Apr 01, 2017 at 02:13:41AM -0700, noman pouigt wrote:
> >> Current registers debugfs file implementation doesn't
> >> handle if the size exceeds 4k. It just dumps 4k of registers.
> >> Fix this by using the seq_file which already handles
> >> the file offset logic instead of reinventing the wheel.
> >> 
> >> I am wondering if there is any issue is doing below which
> >> I am not aware of?
> > 
> > If I remember correctly this is done the way it is because seq_file has
> > to iterate through the entire file to get to the point being read by the
> > application.  This is a *very* big overhead for some applications (like
> > monitoring some registers to see what they're doing) on bigger devices,
> 
> Wondering why would the user space application be monitoring the registers?

We do have tooling that accesses and occasionally monitors
registers using the debugfs files. It is often used by hardware
types while testing/debugging issues. And as Mark points out
speed is quite a priority for us on this front so we would be
very keen to avoid anything that slows down the debugfs interface
to the regmap and the ability to seek and acccess just part of
the file is absolutely vital.

> > especially if there's lots of uncached stuff and the reads have to go to
> > the hardware.  With the current code you can open the file, seek to the
> > registers you're interested in and read only them.  You'll notice that
> > the other debug files have been converted over to seq_file as they're
> > pure software and there's less reason to repeatedly read them.
> 
> Yes. I noticed that and that is why I realized that I am doing something wrong.
> > 
> > I'm also very surprised that this is failing for you as I know this code
> > has been fairly heavily exercised with devices with very large register
> > maps much larger than 4k and my own testing is mainly doing cats which
> > seemed to work last time I tried and should be iterating over the 4k
> > boundary...  what's the actual failure mode?  I'm guessing it's
> 
> I have integrated Florida codec(wm8281) from cirrus and it has more than 4K registers. I could not dump more than 4K with the existing interface.
> 
> Charles, are you able to dump all the registers?
> 

I seem to be able to dump all the registers just fine here (all
500k of them). You say it only dumps 4k worth of registers then
just stops? What version of the kernel are you testing this
on? The test I just ran was on Linus's tree, is this an older
kernel or for-next?

If its an older kernel are you perhaps missing one of these
fixes:

commit 26ee47411ae22caa07d3f3b63ca6d097cba6681b
regmap: debugfs: Fix continued read from registers file

commit 213fa5d9685b985e0c61a8db1883a3abf94b18d7
regmap: debugfs: Fix return from regmap_debugfs_get_dump_start

Thanks,
Charles

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

#1629465

[Multipart message — attachments visible in raw view] — view raw

On Mon, Apr 24, 2017 at 10:02:31AM +0100, Charles Keepax wrote:
> On Sat, Apr 22, 2017 at 01:28:15PM -0700, Variksla wrote:
> > > On Apr 21, 2017, at 10:27 AM, Mark Brown <broonie@kernel.org> wrote:

> > > If I remember correctly this is done the way it is because seq_file has
> > > to iterate through the entire file to get to the point being read by the
> > > application.  This is a *very* big overhead for some applications (like
> > > monitoring some registers to see what they're doing) on bigger devices,

> > Wondering why would the user space application be monitoring the registers?

> We do have tooling that accesses and occasionally monitors
> registers using the debugfs files. It is often used by hardware
> types while testing/debugging issues. And as Mark points out

Even without an actual program explicitly written for it interactive
monitoring of the register map can be done by a person as well which is
just as much of an application (and the major point of the file).

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web