Re: [PATCH 1/1] soc: qcom: geni: Provide parameter error checking

[Lee Jones]

On Tue, 03 Sep 2019, Bjorn Andersson wrote:

> On Tue 03 Sep 06:50 PDT 2019, Lee Jones wrote:
> 
> > When booting with ACPI, the Geni Serial Engine is not set as the I2C/SPI
> > parent and thus, the wrapper (parent device) is unassigned.  This causes
> > the kernel to crash with a null dereference error.
> > 
> 
> Now I see what you did in 8bc529b25354; i.e. stubbed all the other calls
> between the SE and wrapper.
> 
> Do you think it would be possible to resolve the _DEP link to QGP[01]
> somehow?

I looked at QGP{0,1}, but did not see it represented in the current
Device Tree implementation and thus failed to identify it.  Do you
know what it is?  Does it have a driver in Linux already?

> For the clocks workarounds this could be resolved by us
> representing that relationship using device_link and just rely on
> pm_runtime to propagate the clock state.

That is not allowed when booting ACPI.  The Clock/Regulator frameworks
are not to be used in this use-case, hence why all of the calls to
these frameworks are "stubbed out".  If we wanted to properly
implement power management, we would have to create a driver/subsystem
similar to the "Windows-compatible System Power Management Controller"
(PEP).  Without documentation for the PEP, this would be an impossible
task.  A request for the aforementioned documentation has been put in
to Lenovo/Qualcomm.  Hopefully something appears soon.

> For the DMA operation, iiuc it's the wrapper that implements the DMA
> engine involved, but I'm guessing the main reason for mapping buffers on
> the wrapper is so that it ends up being associated with the iommu
> context of the wrapper.

Judging by the code alone, the wrapper doesn't sound like it does much
at all.  It seems to only have a single (version) register (at least
that is the only register that's used).  The only registers it
reads/writes are those of the calling device, whether that be I2C, SPI
or UART.

Device Tree represents the wrapper's relationship with the I2C (and
SPI/UART) Serial Engine (SE) devices as parent-child ones, with the
wrapper being the parent and SE the child.  Whether this is a true
representation of the hardware or just a tactic used for convenience
is not clear, but the same representation does not exist in ACPI.

In the current Linux implementation, the buffer belongs to the SE
(obtained by the child (e.g. I2C) SE by fetching the parent's
(wrapper's) device data using the standard platform helpers) but the
register-set used to control the DMA transactions belong to the SE
devices.

> Are the SMMU contexts at all represented in the ACPI world and if so do
> you know how the wrapper vs SEs are bound to contexts? Can we map on
> se->dev when wrapper is NULL (or perhaps always?)?

Yes, the SMMU devices are represented in ACPI (MMU0) and (MMU1).  They
share the same register addresses as the SMMU devices located in
arch/arm64/boot/dts/qcom/sdm845.dtsi.

With this simple parameter checking patch, the SE falls back to using
FIFO mode to transmit data and continues to work flawlessly.  IMHO
this should be applied in the first instance, as it fixes a real (null
dereference) bug which currently resides in the Mainline kernel.

Moving forward we can try to come up with a suitable plan to implement
DMA in the ACPI use-case - but again, this is feature adding work
which should be carried out against -next, where as this patch needs
to go in via the current -rcs ASAP.

> > Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
> > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> > ---
> > Since we are already at -rc7 this patch should be processed ASAP - thank you.
> > 
> > drivers/soc/qcom/qcom-geni-se.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> > index d5cf953b4337..7d622ea1274e 100644
> > --- a/drivers/soc/qcom/qcom-geni-se.c
> > +++ b/drivers/soc/qcom/qcom-geni-se.c
> > @@ -630,6 +630,9 @@ int geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len,
> >  	struct geni_wrapper *wrapper = se->wrapper;
> >  	u32 val;
> >  
> > +	if (!wrapper)
> > +		return -EINVAL;
> > +
> >  	*iova = dma_map_single(wrapper->dev, buf, len, DMA_TO_DEVICE);
> >  	if (dma_mapping_error(wrapper->dev, *iova))
> >  		return -EIO;
> > @@ -663,6 +666,9 @@ int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len,
> >  	struct geni_wrapper *wrapper = se->wrapper;
> >  	u32 val;
> >  
> > +	if (!wrapper)
> > +		return -EINVAL;
> > +
> >  	*iova = dma_map_single(wrapper->dev, buf, len, DMA_FROM_DEVICE);
> >  	if (dma_mapping_error(wrapper->dev, *iova))
> >  		return -EIO;

-- 
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog