Message-ID: <ZjJ/S1bawYkfs1I4@Asurada-Nvidia>
Date: Wed, 1 May 2024 10:43:39 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <will@...nel.org>, <robin.murphy@....com>, <joro@...tes.org>,
	<thierry.reding@...il.com>, <vdumpa@...dia.com>, <jonathanh@...dia.com>,
	<linux-kernel@...r.kernel.org>, <iommu@...ts.linux.dev>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v6 5/6] iommu/arm-smmu-v3: Add in-kernel support for
 NVIDIA Tegra241 (Grace) CMDQV

On Wed, May 01, 2024 at 10:00:42AM -0300, Jason Gunthorpe wrote:
> On Tue, Apr 30, 2024 at 11:08:55AM -0700, Nicolin Chen wrote:
> > On Tue, Apr 30, 2024 at 01:35:45PM -0300, Jason Gunthorpe wrote:
> > > On Mon, Apr 29, 2024 at 09:43:48PM -0700, Nicolin Chen wrote:
> > > Really this all seems like alot of overkill to make a little bit of
> > > shorthand. It is not so wordy just to type it out:
> > > 
> > >   readl(vintf->base + TEGRA241_VINTF_CONFIG) 
> > 
> > vintf_readl(vintf, CONFIG) is much shorter. Doing so reduced the
> > line breaks at quite a lot places, so overall the driver looks a
> > lot cleaner to me.
> 
> We don't have the strict 80 column limit now, it would be fine to go a
> few extra to avoid the breaks.
> 
> Certainly preferred to these readability damaging macros.

I think only very few folks will look up for the register macros
that are actually damaged, and these MMIO macros still look very
straightforward that we will always know the correct ->base and
register offset pair will be used.

There are four sets of ->base in the driver. Sometimes they can
be so dazzling that a wrong pair might be accidentally used. Yet
such a stupid mistake can be really difficult to catch it out --
I had a hard time earlier to sort it out..

> > I can probably change these logging helpers to inline functions.
> 
> Just call the normal logging functions directly.
.
> Just call the normal logging functions, there are so few callers
> typing out the VCMDQ%u is not going to be so bad

There're a few more in the part 2. And it's actually a bit wordy
doing "VINTF%u: LVCDMQ%u/VCMDQ%u" at every place..

> > It would be unrolled to three mostly identical inline functions:
> > 	tegra241_cmdqv_write_config(cmdqv, regval)
> > 	tegra241_vintf_write_config(vintf, regval)
> > 	tegra241_vcmdq_write_config(vcmdq, regval)
> 
> Expand the parameters in the caller:
> 
> __do_write_config(owner->base, &owner->status, _CMDQV_EN, TEGRA241_CMDQ_CONFIG,
>                   TEGRA241_CMDQ_STATUS, _CMDQ_ENABLED)

Yea, I just did something like that. _CMDQV_EN and _CMDQ_ENABLED
always equal to BIT(0), so I also move them to the common place.

> > > > +#define cmdqv_write_config(_regval) \
> > > > +	tegra241_cmdqv_write_config(cmdqv, CMDQV, _regval)
> > > > +#define vintf_write_config(_regval) \
> > > > +	tegra241_cmdqv_write_config(vintf, VINTF, _regval)
> > > > +#define vcmdq_write_config(_regval) \
> > > > +	tegra241_cmdqv_write_config(vcmdq, VCMDQ, _regval)
> > > 
> > > More hidden access to stack values
> > 
> > Btw, any reason for forbidding this practice? It will break the
> > build if something goes wrong, which seems to be pretty easy to
> > catch.
> 
> It is the kernel consensus not to do that. function-like-macros should
> act like functions and not reach into some other stack frame. It makes
> it very hard to follow the calling function if you can't follow where
> the references are.

I see.

> > > > +	/* Use SMMU CMDQ if vintfs[0] is uninitialized */
> > > > +	if (!FIELD_GET(VINTF_ENABLED, atomic_read(&vintf->status)))
> > > > +		return &smmu->cmdq;
> > > > +
> > > > +	/* Use SMMU CMDQ if vintfs[0] has error status */
> > > > +	if (FIELD_GET(VINTF_STATUS, atomic_read(&vintf->status)))
> > > > +		return &smmu->cmdq;
> > > 
> > > Why atomic_read? The unlocked interaction with
> > > tegra241_cmdqv_handle_vintf0_error() doesn't seem especially sane IMHO
> > 
> > Race between this get_cmdq() and the isr. Any alternative practice?
> 
> It doesn't fix any real race, I'm not sure what this is supposed to be
> doing. The cmdq becomes broken and you get an ISR, so before the ISR
> it will still post but get stuck, during the ISR it will avoid
> posting, and after it will go back to posting?
> 
> Why? Just always post to the Q and let the ISR fix it?

Yes, we could do so. I was thinking of the worst case by giving
the guest OS a chance to continue (though in a slower mode), if
something unrecoverable happens to the VINTF/VCMDQ part.

> > > > +	if (tegra241_cmdqv_probe(cmdqv)) {
> > > > +		if (cmdqv->irq > 0)
> > > > +			devm_free_irq(smmu->dev, cmdqv->irq, cmdqv);
> > > > +		devm_iounmap(smmu->dev, cmdqv->base);
> > > > +		devm_kfree(smmu->dev, cmdqv);
> > > > +		return NULL;
> > > 
> > > Oh. Please don't use devm at all in this code then, it is not attached
> > > to a probed driver with the proper scope, devm isn't going to work in
> > > sensible way.
> > 
> > Mind elaborating "it is not"? This function is called by
> > arm_smmu_device_acpi_probe and arm_smmu_device_probe.
> 
> Normal devm usage will unwind the devm allocations when probe fails.
> 
> That doesn't happen here, you open coded the unwind above, and then
> you have open coded freeing in another place anyhow.
> 
> So just don't use it. There is no value if the places where it should
> work automatically are not functioning.

I thought devm could work when rmmod too, not only when the probe
fails..

In that case, should we be concerned about the "rmmod arm-smmu-v3"?
There is no callback function for that yet, so memory on the cmdqv
side wouldn't be free-ed.

Thanks
Nicolin

Powered by blists - more mailing lists