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

[Variksla]

> On Apr 21, 2017, at 10:27 AM, Mark Brown <broonie@xxxxxxxxxx> 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.