Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions

[Lina Iyer]

On Fri, May 11 2018 at 14:14 -0600, Doug Anderson wrote:
> Hi,
>
> On Fri, May 11, 2018 at 8:06 AM, Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote:
> > > As I've said I haven't reviewed RPMh in any amount of detail and so
> > > perhaps I don't understand something.
> > >
> > > OK, I dug a little more and coded up something for you.  Basically
> > > you're doing a whole bunch of iteration / extra work here to try to
> > > come up with a way to associate an extra bit of data with each "struct
> > > rsc_drv".  Rather than that, just add an extra field into "struct
> > > rsc_drv".  Problem solved.
> > >
> > > See http://crosreview.com/1054646 for what I mean.
> > I tried to avoid such pointer references and keep it object oriented
> > with this approach. I agree that we run through a list of 2 (at the max)
> > RSC to get the drv* from the rpmh_ctrlr. It is not going to be
> > expensive.
>
> Even if you wanted to keep things "object oriented" then IMHO your
> code still should change.  Sure, it's not computationally expensive to
> iterate through this list, but it adds an extra level of complexity
> that doesn't seem justified.
>
> If you _really_ needed an abstraction barrier then at least add a
> "void *client_data" to "struct rsc_drv.c".  At least you'd get rid of
> the ugly global list and store your pointer directly in the correct
> structure rather than creating an external entity.  Now it becomes
> 100% obvious that there is exactly 1 "struct rpmh_ctrlr" for each
> controller.  ...but IMO there's enough intertwining between "rpmh.c"
> and "rpmh-rsc.c" that it would just be a waste because now you'd need
> to do extra memory allocation and freeing.  ...and if you just
> allocated the pointer in get_rpmh_ctrlr() it would also seem non-ideal
> because this one-time allocation (that affects _all_ RPMh clients)
> happens whenever one client happens to do the first access.  This is
> one-time init so it makes sense to do it at init time.
>
> I say that there's intertwining between "rpmh.c" and "rpmh-rsc.c"
> because both C files call directly into one another and have intimate
> knowledge of how the other works.  They aren't really separate things.
> Specifically I see that "rpmh-rsc" directly calls into "rpmh.c" when
> it calls rpmh_tx_done(), and of coruse "rpmh-rsc.c" directly calls
> into "rpmh.c".
>
>
> OK, so I've been browsing through the source code more so I can be a
> little more informed here.  As far as I can tell "rpmh.c"'s goal is:
>
> 1. Put a simpler API atop "rpmh-rsc.c".  ...but why not just put that
> API directly into "rpmh-rsc.c" anyway?  If there was someone that
> needed the more complex API then having a "simpler" wrapper makes
> sense, but that's not the case, is it?
>
> 2. Add caching atop "rpmh-rsc"
>
>
> I'll respond to some of your other patches too, but I think that the
> amount of code for caching is not very much.  I don't see the benefit
> of trying to split the code into two files.  Put them into one and
> then delete all the extra code you needed just the try to maintain
> some abstraction.
>
>
> > Another things this helps with is that, if the driver is not a child of
> > the RSC nodes in DT, then the drvdata of the parent would not a RSC node
> > and accessing that would result in a crash. This offers a cleaner exit
> > path for the error case.
>
> Why would the driver not be a child of the RSC nodes?  That's kinda
> like saying "if you try to instantiate an i2c device as a platform
> device then its probe will crash".  Yeah, it will.  Doctor, it hurts
> if I poke myself in my eye with this sharp stick, do you have any
> medicine that can help fix that?
>
>
> > > > > I'll try to dig into this more so I could just be confused, but in
> > > > > general it seems really odd to have a spinlock and something called a
> > > > > "cache" at this level.  If we need some sort of mutual exclusion or
> > > > > caching it seems like it should be stored in memory directly
> > > > > associated with the RPMh device, not some external global.
> > > > The idea behind the locking is not to avoid the race between rpmh.c and
> > > > rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are
> > > > probed following the probe of the controller. And init is not that we are
> > > > worried about.
> > > > The condition here is to prevent the rpmh_rsc[] from being modified
> > > > concurrently by drivers.
> > >
> > >
> > > OK, I see the point of the locking now, but not the list still.
> > > Sounds like Matthias agrees with me that the list isn't useful.  Seems
> > > like you should squash my patch at http://crosreview.com/1042883 into
> > > yours.
> > I saw your approach. I am okay with it for your tree,
>
> I'm not okay with it for the Chrome OS tree.  We need to match
> upstream, not fork for style reasons.  I'd rather take a driver that I
> think it overly complex but matches upstream than a private driver.
>
>
> > my approach comes
> > out of experiences in qcom platforms and how things tend to shape up in
> > the future. I would want you to consider my reasoning as well, before we
> > go forward.
>
> I suppose we can get advice from others who have worked in qcom
> platforms and see what they think.  My opinions come out of years of
> experience working with Linux drivers, so I guess we both have some
> experience under our belts that we're trying to leverage.
Doug, I am sorry it came out the wrong way. I meant to say, knowing
qualcomm platforms, as it has been in the past, we need this
flexibility. Things change with hardware variants just a wee bit that it
doesn't warrant a new interface, just a new driver or part of it to be
re-written. Keeping code separate out like this helps us maintain the
driver better.

Thanks for your reviews. I will try to address your comments before my
vacation, but I doubt I will get to all of it.

Thanks,
Lina