Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

[Thierry Reding]

On Wed, Sep 30, 2020 at 07:26:00PM +0300, Dmitry Osipenko wrote:
> 30.09.2020 19:15, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 19:03, Thierry Reding пишет:
> >>> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
> >>>> 30.09.2020 18:23, Thierry Reding пишет:
> >>>>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
> >>>>>> From: Dmitry Osipenko <digetx@xxxxxxxxx>
> >>>>>>
> >>>>>> Multiple Tegra drivers need to retrieve Memory Controller and hence there
> >>>>>> is quite some duplication of the retrieval code among the drivers. Let's
> >>>>>> add a new common helper for the retrieval of the MC.
> >>>>>>
> >>>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> >>>>>> Signed-off-by: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
> >>>>>> ---
> >>>>>>
> >>>>>> Changelog
> >>>>>> v2->v3:
> >>>>>>  * Replaced with Dimtry's devm_tegra_get_memory_controller()
> >>>>>> v1->v2:
> >>>>>>  * N/A
> >>>>>>
> >>>>>>  drivers/memory/tegra/mc.c | 39 +++++++++++++++++++++++++++++++++++++++
> >>>>>>  include/soc/tegra/mc.h    | 17 +++++++++++++++++
> >>>>>>  2 files changed, 56 insertions(+)
> >>>>>
> >>>>> Let's not add this helper, please. If a device needs a reference to the
> >>>>> memory controller, it should have a phandle to the memory controller in
> >>>>> device tree so that it can be looked up explicitly.
> >>>>>
> >>>>> Adding this helper is officially sanctioning that it's okay not to have
> >>>>> that reference and that's a bad idea.
> >>>>
> >>>> And please explain why it's a bad idea, I don't see anything bad here at
> >>>> all.
> >>>
> >>> Well, you said yourself in a recent comment that we should avoid global
> >>> variables. devm_tegra_get_memory_controller() is nothing but a glorified
> >>> global variable.
> >>
> >> This is not a variable, but a common helper function which will remove
> >> the duplicated code and will help to avoid common mistakes like a missed
> >> put_device().
> > 
> > Yeah, you're right: this is actually much worse than a global variable.
> > It's a helper function that needs 50+ lines in order to effectively
> > access a global variable.
> > 
> > You could write this much simpler by doing something like this:
> > 
> > 	static struct tegra_mc *global_mc;
> > 
> > 	int tegra_mc_probe(...)
> > 	{
> > 		...
> > 
> > 		global_mc = mc;
> > 
> > 		...
> > 	}
> > 
> > 	struct tegra_mc *tegra_get_memory_controller(void)
> > 	{
> > 		return global_mc;
> > 	}
> > 
> > The result is *exactly* the same, except that this is actually more
> > honest. Nicolin's patch *pretends* that it isn't using a global variable
> > by wrapping a lot of complicated code around it.
> > 
> > But that doesn't change the fact that this accesses a singleton object
> > without actually being able to tie it to the device in the first place.
> 
> I don't think that the MC driver will stay built-in forever, although
> its modularization is complicated right now. Hence something shall keep
> the reference to the MC device resources while they are in use and this
> patch takes care of doing that.

It looks to me like all the other places where we get a reference to the
MC also keep a reference to the device. That's obviously not going to be
enough once the code is turned into a module. At that point we need to
make sure to also grab a reference to the module. But that's orthogonal
to this discussion.

> Secondly, the Nicolin's patch doesn't pretend on anything, but rather

Yes, the patch does pretend to "look up" the memory controller device,
but in reality it will always return a singleton object, which can just
as easily be achieved by using a global variable.

> brings the already existing duplicated code to a single common place.

Where exactly is that duplicated code? The only places I see where we
get a reference to the memory controller are from the EMC drivers and
they properly look up the MC via the nvidia,memory-controller device
tree property.

But that's not what this new helper does. This code will use the OF
lookup table to find any match and then returns that, completely
ignoring any links established by the device tree.

Thierry

Attachment: signature.asc
Description: PGP signature