Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-03-14 04:25:16)
> On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> >
> > Why can't we use the debugfs_create_u32 function? It would make things
> > clearer if there was either a debugfs_create_le32() function or if the
> > socinfo structure stored in smem was unmarshalled from little endian
> > to the cpu native endian format during probe time and then all the rest
> > of the code can treat it as a native endian u32 values.
> 
> Thanks for the review. I've worked on the next version with all the
> changes you suggested in the patchset but I'm kind of not sure
> what would be the best solution in this case.

Alright, thanks.

> 
> I'm bit skeptical about introducing debugfs_create_le32 as we don't
> really have any endian specific functions in the debugfs core at the
> moment. And if I do that, should I also introduce debugfs_create_be32
> [and to an extent debugfs_create_le{16,64}]? More importantly, would
> it be useful to introduce them in core?

I suppose it's ambiguous if the endianness should be swapped to be CPU
native, or if it should just export them as little endian or big endian
to userspace. I wouldn't introduce any APIs that aren't used, because
it's just dead code. If the code is documented clearly and indicates
what it does then it should be fine. This patch has pretty much already
written the code, so it's just a matter of moving it into debugfs core
now and getting gregkh to approve.

> 
> In the case of converting it to cpu native during probe, I'll need to
> declare an extra struct with u32 being the parsed version for it to be
> correct. Wouldn't it add extra overhead?

Yes it would be some small extra overhead that could be allocated on the
kernel's heap. What's the maximum size? A hundred bytes or so?

I don't see much of a problem with this approach. It simplifies the
patch series because nothing new is introduced in debugfs core and the
endian conversion is done once in one place instead of being scattered
throughout the code. Sounds like a good improvement to me.