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

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

[Lee Jones]

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.

Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
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;
-- 
2.17.1

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

[Bjorn Andersson]

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? 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.

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.

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?)?

Regards,
Bjorn

> Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> 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;
> -- 
> 2.17.1
> 

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@linaro.org>
> > ---
> > 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

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

[Bjorn Andersson]

On Wed 04 Sep 01:45 PDT 2019, Lee Jones wrote:

> 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?
> 

QGP0 is the same hardware block as &qupv3_id_0, but apparently both are
only representing a smaller part - and different ones.

But conceptually both represents the wrapper...

> > 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.
> 

I see, so the PEP states needs to be parsed and associated with each
device and we would use pm_runtime to toggle between the states and
device_links to ensure that _DEP nodes are powered in appropriate order.

That seems reasonable and straight forward and the reliance on
pm_runtime will make the DT case cleaner as well.

> > 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.
> 

Yeah, I saw this as well. If all the SEs where the wrappers iommu domain
things should work fine by mapping it on the se->dev, regardless of the
device's being linked together.

The remaining relationship to the wrapper would then be reduced to the
read of the version to check for 1.0 or 1.1 hardware in the SPI driver,
which can be replaced by the assumption that we're on 1.1.

> > 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.
> 

Right but this only describes the IOMMU devices, I don't see any
information about how individual client devices relates to the various
IOMMU contexts.

> 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.
> 

Per the current driver design the wrapper device is the parent of the
SE, I should have seen that 8bc529b25354 was the beginning of a game of
whac-a-mole circumventing this design. Sorry for not spotting this
earlier.

But if this is the one whack left to get the thing to boot then I think
we should merge it.

> 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.
> 

Sounds good.

Regards,
Bjorn

> > > Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > > 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

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

[Lee Jones]

On Wed, 04 Sep 2019, Bjorn Andersson wrote:

> On Wed 04 Sep 01:45 PDT 2019, Lee Jones wrote:
> 
> > 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?
> 
> QGP0 is the same hardware block as &qupv3_id_0, but apparently both are
> only representing a smaller part - and different ones.
> 
> But conceptually both represents the wrapper...

... which doesn't actually do anything in the Linux implementation.

It only has one register. :)

> > > 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.
> > 
> 
> I see, so the PEP states needs to be parsed and associated with each
> device and we would use pm_runtime to toggle between the states and
> device_links to ensure that _DEP nodes are powered in appropriate order.
> 
> That seems reasonable and straight forward and the reliance on
> pm_runtime will make the DT case cleaner as well.

Essentially yes.  The issue is translating the ACPI tables into
actions to be taken by the Linux Power Management APIs.  Again, we've
requested documentation.  Now, we wait ...

> > > 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.
> > 
> 
> Yeah, I saw this as well. If all the SEs where the wrappers iommu domain
> things should work fine by mapping it on the se->dev, regardless of the
> device's being linked together.

This is my assumption too.

> The remaining relationship to the wrapper would then be reduced to the
> read of the version to check for 1.0 or 1.1 hardware in the SPI driver,
> which can be replaced by the assumption that we're on 1.1.

Also correct.  You would be left with a huge duplication of code
across each of the SEs however.

> > > 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.
> 
> Right but this only describes the IOMMU devices, I don't see any
> information about how individual client devices relates to the various
> IOMMU contexts.

I see some _DEPs which detail the MMU{0,1}, but that's about it.

> > 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.
> > 
> 
> Per the current driver design the wrapper device is the parent of the
> SE, I should have seen that 8bc529b25354 was the beginning of a game of
> whac-a-mole circumventing this design. Sorry for not spotting this
> earlier.

Right, but that doesn't mean that the current driver design is
correct.  ACPI, which is in theory a description of the hardware
doesn't seem to think so.  It looks more like we do this in Linux as a
convenience function to link the devices.  Instead this 'parent' seems
to be represented as a very small register space at the end of the SE
banks.

> But if this is the one whack left to get the thing to boot then I think
> we should merge it.

Amazing, thank you!

Do you know how we go about getting this merged?  We only potentially
have 0.5 weeks (1.5 weeks if there is an -rc8 [doubtful]), so we need
to move fast.  Would you be prepared to send it to Linus for -fixes?
I'd do it myself, but this is a little out of my remit.

Nothing heard from Andy for a very long time.

> > 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.
> 
> Sounds good.

Great.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Bjorn Andersson]

On Wed 04 Sep 13:01 PDT 2019, Lee Jones wrote:

> On Wed, 04 Sep 2019, Bjorn Andersson wrote:
> 
> > On Wed 04 Sep 01:45 PDT 2019, Lee Jones wrote:
> > 
> > > On Tue, 03 Sep 2019, Bjorn Andersson wrote:
> > > 
> > > > On Tue 03 Sep 06:50 PDT 2019, Lee Jones wrote:
[..]
> > > 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.
> > > 
> > 
> > Per the current driver design the wrapper device is the parent of the
> > SE, I should have seen that 8bc529b25354 was the beginning of a game of
> > whac-a-mole circumventing this design. Sorry for not spotting this
> > earlier.
> 
> Right, but that doesn't mean that the current driver design is
> correct.  ACPI, which is in theory a description of the hardware
> doesn't seem to think so.  It looks more like we do this in Linux as a
> convenience function to link the devices.  Instead this 'parent' seems
> to be represented as a very small register space at the end of the SE
> banks.
> 

There's a larger register window containing one block of common
registers followed by register blocks for each serial engine.

I don't know if we will need more of the common registers in the future,
but for now you at least have the requirement that in order to operate
the SEs you need to clock the wrapper. So the current DT model
represents the hardware and the power/clocking topology.

The fact that you managed to boot the system with just ignoring all
clocks is a surprise to me.

> > But if this is the one whack left to get the thing to boot then I think
> > we should merge it.
> 
> Amazing, thank you!
> 
> Do you know how we go about getting this merged?  We only potentially
> have 0.5 weeks (1.5 weeks if there is an -rc8 [doubtful]), so we need
> to move fast.  Would you be prepared to send it to Linus for -fixes?
> I'd do it myself, but this is a little out of my remit.
> 

The "offending" commit was picked up mid June and no one noticed that it
doesn't work until this week?

Let's slap a Cc: stable@ on it and get it into v5.4-rc1 and it will show
up in v5.3.1.

Regards,
Bjorn

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

[Stephen Boyd]

Quoting Bjorn Andersson (2019-09-04 11:27:32)
> On Wed 04 Sep 01:45 PDT 2019, Lee Jones wrote:
> 
> > 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?
> > 
> 
> QGP0 is the same hardware block as &qupv3_id_0, but apparently both are
> only representing a smaller part - and different ones.
> 
> But conceptually both represents the wrapper...
> 
> > > 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.
> > 
> 
> I see, so the PEP states needs to be parsed and associated with each
> device and we would use pm_runtime to toggle between the states and
> device_links to ensure that _DEP nodes are powered in appropriate order.
> 
> That seems reasonable and straight forward and the reliance on
> pm_runtime will make the DT case cleaner as well.

I think we tried to push for pm_runtime here but it was rejected because
of a missing interconnect framework? See
https://marc.info/?l=devicetree&m=152002327106864&w=2

> 
> > > 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.
> > 
> 
> Yeah, I saw this as well. If all the SEs where the wrappers iommu domain
> things should work fine by mapping it on the se->dev, regardless of the
> device's being linked together.
> 
> The remaining relationship to the wrapper would then be reduced to the
> read of the version to check for 1.0 or 1.1 hardware in the SPI driver,
> which can be replaced by the assumption that we're on 1.1.

Could this be described in the DT compatible for the SE in the future
instead of reading it from the wrapper register space?

> 
> > > 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.
> > 
> 
> Right but this only describes the IOMMU devices, I don't see any
> information about how individual client devices relates to the various
> IOMMU contexts.

The only thread I recall describing this is
https://lkml.kernel.org/r/945b6c00-dde6-6ec7-4577-4cc0d034796b@codeaurora.org

> 
> > 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.
> > 
> 
> Per the current driver design the wrapper device is the parent of the
> SE, I should have seen that 8bc529b25354 was the beginning of a game of
> whac-a-mole circumventing this design. Sorry for not spotting this
> earlier.
> 
> But if this is the one whack left to get the thing to boot then I think
> we should merge it.
> 

Agreed.

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

[Lee Jones]

> > But if this is the one whack left to get the thing to boot then I think
> > we should merge it.
> 
> Agreed.

Thanks Stephen.

Unless you guys scream loudly, I'm going to convert these to Acks.

If you scream softly, I can convert the to Reviewed-bys.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Wed, 04 Sep 2019, Bjorn Andersson wrote:

> On Wed 04 Sep 13:01 PDT 2019, Lee Jones wrote:
> 
> > On Wed, 04 Sep 2019, Bjorn Andersson wrote:
> > 
> > > On Wed 04 Sep 01:45 PDT 2019, Lee Jones wrote:
> > > 
> > > > On Tue, 03 Sep 2019, Bjorn Andersson wrote:
> > > > 
> > > > > On Tue 03 Sep 06:50 PDT 2019, Lee Jones wrote:
> [..]
> > > > 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.
> > > > 
> > > 
> > > Per the current driver design the wrapper device is the parent of the
> > > SE, I should have seen that 8bc529b25354 was the beginning of a game of
> > > whac-a-mole circumventing this design. Sorry for not spotting this
> > > earlier.
> > 
> > Right, but that doesn't mean that the current driver design is
> > correct.  ACPI, which is in theory a description of the hardware
> > doesn't seem to think so.  It looks more like we do this in Linux as a
> > convenience function to link the devices.  Instead this 'parent' seems
> > to be represented as a very small register space at the end of the SE
> > banks.
> 
> There's a larger register window containing one block of common
> registers followed by register blocks for each serial engine.
> 
> I don't know if we will need more of the common registers in the future,
> but for now you at least have the requirement that in order to operate
> the SEs you need to clock the wrapper. So the current DT model
> represents the hardware and the power/clocking topology.
> 
> The fact that you managed to boot the system with just ignoring all
> clocks is a surprise to me.

That is a prerequisite of UEFI/ACPI.  All clocks, registers, phys,
resets, etc must be configured by the firmware.  We should only need
to play with them for Power Management purposes (which requires the
PEP to be enabled).

> > > But if this is the one whack left to get the thing to boot then I think
> > > we should merge it.
> > 
> > Amazing, thank you!
> > 
> > Do you know how we go about getting this merged?  We only potentially
> > have 0.5 weeks (1.5 weeks if there is an -rc8 [doubtful]), so we need
> > to move fast.  Would you be prepared to send it to Linus for -fixes?
> > I'd do it myself, but this is a little out of my remit.
> > 
> 
> The "offending" commit was picked up mid June and no one noticed that it
> doesn't work until this week?

I think you're viewing this from the wrong angle.  The "ACPI
enablement" commit(s) merely prevented the use of the Clock and
Regulator APIs, as per the ACPI guidelines.  See [0].

Right.  Unfortunately, I developed this on top of our DT enablement
patches, which also included your Geni SE DMA and Regulator status
hacks, which meant I missed this and the above until now.

It's only now I've had the chance to attempt to boot raw Mainline that
these came to light, hence the last minute panic to try and fix them.

> Let's slap a Cc: stable@ on it and get it into v5.4-rc1 and it will show
> up in v5.3.1.

We *can* do that, however my fear is that the distros are going to be
releasing their new versions (next month) based on v5.3.  Which would
mean they will not boot on these platforms until they backport these
patches, which might be some months later.

This is the only issue that prevents ACPI from booting, meaning we can
at least make use of the distro installers when they are released.
The patch is very simple and low-risk, so ideally it would go into
-rc7.

[0] Documentation/arm64/arm-acpi.rst

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Stephen Boyd]

Quoting Lee Jones (2019-09-04 23:42:53)
> > > But if this is the one whack left to get the thing to boot then I think
> > > we should merge it.
> > 
> > Agreed.
> 
> Thanks Stephen.
> 
> Unless you guys scream loudly, I'm going to convert these to Acks.
> 
> If you scream softly, I can convert the to Reviewed-bys.
> 

<in a soft scream>

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

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

[Lee Jones]

On Wed, 04 Sep 2019, Stephen Boyd wrote:

> Quoting Lee Jones (2019-09-04 23:42:53)
> > > > But if this is the one whack left to get the thing to boot then I think
> > > > we should merge it.
> > > 
> > > Agreed.
> > 
> > Thanks Stephen.
> > 
> > Unless you guys scream loudly, I'm going to convert these to Acks.
> > 
> > If you scream softly, I can convert the to Reviewed-bys.
> > 
> 
> <in a soft scream>
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Done. :)

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog