Message-ID: <CANNG1HV0Ai9oeUr55uk+wAf456vDn1qiYmeXYc3=rkpqhdoM4g@mail.gmail.com>
Date:   Thu, 21 Mar 2019 11:21:20 +0530
From:   Vaishali Thakkar <vaishali.thakkar@...aro.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        rafael@...nel.org, Bjorn Andersson <bjorn.andersson@...aro.org>,
        Vinod Koul <vkoul@...nel.org>
Subject: Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

On Thu, 14 Mar 2019 at 21:28, Stephen Boyd <swboyd@...omium.org> wrote:
>
> Quoting Vaishali Thakkar (2019-03-14 04:25:16)
> > On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@...omium.org> 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.
>

Yes, it's true that this approach is better than introducing new endian
functions in debugfs core but we should also keep in mind that this is
applicable only for 4 use cases. For other usecases, we want to print
string and hex values. So, I would either need new debugfs core
functions for the same. I tried introducing debugfs_create_str for string
values but we're ending up with introducing bunch of other helpers in
the core as simple_attr_read expects integer values. Similarly, for hex
values , I can't use debugfs_create_u32 as defined attributes in the
core has unsigned int as a specifier, will need to introduce some extra
helpers over there again.

Also, in case of keeping all other cases as it is, it'll look quite
asymmetric to use debugfs u32 function in init and using local macros
for other cases. I can have DEBUGFS_UINT_ADD like wrapper
macro for debugfs_create_u32 but again not sure if doing
all of this looks better than what we have at the moment as just having
3 local macros covering our all cases without having lot of duplicated
code.

 Let me know if about your opinion on the same. Thanks.

Powered by blists - more mailing lists