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

From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Lee Jones <lee.jones@linaro.org>
Cc: agross@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 1/1] soc: qcom: geni: Provide parameter error checking
Date: Wed, 4 Sep 2019 13:26:56 -0700	[thread overview]
Message-ID: <20190904202656.GB580@tuxbook-pro> (raw)
In-Reply-To: <20190904200130.GT26880@dell>

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