diff mbox series

[2/2] i2c: qcom-geni: Provide an option to disable DMA processing

Message ID 20190905075213.13260-2-lee.jones@linaro.org
State New
Headers show
Series None | expand

Commit Message

Lee Jones Sept. 5, 2019, 7:52 a.m. UTC 
We have a production-level laptop (Lenovo Yoga C630) which is exhibiting
a rather horrific bug.  When I2C HID devices are being scanned for at
boot-time the QCom Geni based I2C (Serial Engine) attempts to use DMA.
When it does, the laptop reboots and the user never sees the OS.

The beautiful thing about this approach is that, *if* the Geni SE DMA
ever starts working, we can remove the C code and any old properties
left in older DTs just become NOOP.  Older kernels with newer DTs (less
of a priority) *still* will not work - but they do not work now anyway.

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

Reviewed-by: Vinod Koul <vkoul@kernel.org>

---
 drivers/i2c/busses/i2c-qcom-geni.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Marc Gonzalez Sept. 5, 2019, 8:18 a.m. UTC | #1 
[ Trimming recipients list for idle chat ]

On 05/09/2019 09:52, Lee Jones wrote:

> We have a production-level laptop (Lenovo Yoga C630) which is exhibiting

> a rather horrific bug.  When I2C HID devices are being scanned for at

> boot-time the QCom Geni based I2C (Serial Engine) attempts to use DMA.

> When it does, the laptop reboots and the user never sees the OS.

> 

> The beautiful thing about this approach is that, *if* the Geni SE DMA

> ever starts working, we can remove the C code and any old properties

> left in older DTs just become NOOP.  Older kernels with newer DTs (less

> of a priority) *still* will not work - but they do not work now anyway.

> 

> Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")

> Signed-off-by: Lee Jones <lee.jones@linaro.org>

> Reviewed-by: Vinod Koul <vkoul@kernel.org>

> ---

>  drivers/i2c/busses/i2c-qcom-geni.c | 14 ++++++++++----

>  1 file changed, 10 insertions(+), 4 deletions(-)

> 

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c

> index a89bfce5388e..8822dea82980 100644

> --- a/drivers/i2c/busses/i2c-qcom-geni.c

> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

> @@ -353,13 +353,16 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)

>  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

>  				u32 m_param)

>  {

> +	struct device_node *np = gi2c->se.dev->of_node;

>  	dma_addr_t rx_dma;

>  	unsigned long time_left;

> -	void *dma_buf;

> +	void *dma_buf = NULL;

>  	struct geni_se *se = &gi2c->se;

>  	size_t len = msg->len;

>  

> -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> +	if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))

> +		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> +

>  	if (dma_buf)

>  		geni_se_select_mode(se, GENI_SE_DMA);

>  	else

> @@ -392,13 +395,16 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

>  static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

>  				u32 m_param)

>  {

> +	struct device_node *np = gi2c->se.dev->of_node;

>  	dma_addr_t tx_dma;

>  	unsigned long time_left;

> -	void *dma_buf;

> +	void *dma_buf = NULL;

>  	struct geni_se *se = &gi2c->se;

>  	size_t len = msg->len;

>  

> -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> +	if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))

> +		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> +

>  	if (dma_buf)

>  		geni_se_select_mode(se, GENI_SE_DMA);

>  	else

> 


Would it make sense to factorize the DT lookup within a helper?
(For example; not compile-tested; not sure it's worth it)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index a89bfce5388e..1489181f60fe 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -350,6 +350,14 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
 		dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");
 }
 
+static void *get_dma_buf(struct geni_se *se, struct i2c_msg *msg)
+{
+	if (of_property_read_bool(se->dev->of_node, "qcom,geni-se-no-dma"))
+		return NULL;
+
+	return i2c_get_dma_safe_msg_buf(msg, 32);
+}
+
 static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 				u32 m_param)
 {
@@ -359,7 +367,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	struct geni_se *se = &gi2c->se;
 	size_t len = msg->len;
 
-	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+	dma_buf = get_dma_buf(se, msg);
 	if (dma_buf)
 		geni_se_select_mode(se, GENI_SE_DMA);
 	else
@@ -398,7 +406,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	struct geni_se *se = &gi2c->se;
 	size_t len = msg->len;
 
-	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+	dma_buf = get_dma_buf(se, msg);
 	if (dma_buf)
 		geni_se_select_mode(se, GENI_SE_DMA);
 	else
Lee Jones Sept. 5, 2019, 8:36 a.m. UTC | #2 
On Thu, 05 Sep 2019, Marc Gonzalez wrote:

> [ Trimming recipients list for idle chat ]

> 

> On 05/09/2019 09:52, Lee Jones wrote:

> 

> > We have a production-level laptop (Lenovo Yoga C630) which is exhibiting

> > a rather horrific bug.  When I2C HID devices are being scanned for at

> > boot-time the QCom Geni based I2C (Serial Engine) attempts to use DMA.

> > When it does, the laptop reboots and the user never sees the OS.

> > 

> > The beautiful thing about this approach is that, *if* the Geni SE DMA

> > ever starts working, we can remove the C code and any old properties

> > left in older DTs just become NOOP.  Older kernels with newer DTs (less

> > of a priority) *still* will not work - but they do not work now anyway.

> > 

> > Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")

> > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > Reviewed-by: Vinod Koul <vkoul@kernel.org>

> > ---

> >  drivers/i2c/busses/i2c-qcom-geni.c | 14 ++++++++++----

> >  1 file changed, 10 insertions(+), 4 deletions(-)

> > 

> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c

> > index a89bfce5388e..8822dea82980 100644

> > --- a/drivers/i2c/busses/i2c-qcom-geni.c

> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c

> > @@ -353,13 +353,16 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)

> >  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

> >  				u32 m_param)

> >  {

> > +	struct device_node *np = gi2c->se.dev->of_node;

> >  	dma_addr_t rx_dma;

> >  	unsigned long time_left;

> > -	void *dma_buf;

> > +	void *dma_buf = NULL;

> >  	struct geni_se *se = &gi2c->se;

> >  	size_t len = msg->len;

> >  

> > -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> > +	if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))

> > +		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> > +

> >  	if (dma_buf)

> >  		geni_se_select_mode(se, GENI_SE_DMA);

> >  	else

> > @@ -392,13 +395,16 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

> >  static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

> >  				u32 m_param)

> >  {

> > +	struct device_node *np = gi2c->se.dev->of_node;

> >  	dma_addr_t tx_dma;

> >  	unsigned long time_left;

> > -	void *dma_buf;

> > +	void *dma_buf = NULL;

> >  	struct geni_se *se = &gi2c->se;

> >  	size_t len = msg->len;

> >  

> > -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> > +	if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))

> > +		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> > +

> >  	if (dma_buf)

> >  		geni_se_select_mode(se, GENI_SE_DMA);

> >  	else

> > 

> 

> Would it make sense to factorize the DT lookup within a helper?

> (For example; not compile-tested; not sure it's worth it)


Possibly, but the semantics end up the same.

If you think it's cleaner, perhaps submit your version a fix-up to the
original (this one).  Seeing as we're already carrying Reviewed-bys
and time is very limited to have this fixed.

I would also like to see the helper in your version prefixed, so it
would be:

  geni_i2c_get_dma_buf()

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c

> index a89bfce5388e..1489181f60fe 100644

> --- a/drivers/i2c/busses/i2c-qcom-geni.c

> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

> @@ -350,6 +350,14 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)

>  		dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");

>  }

>  

> +static void *get_dma_buf(struct geni_se *se, struct i2c_msg *msg)

> +{

> +	if (of_property_read_bool(se->dev->of_node, "qcom,geni-se-no-dma"))

> +		return NULL;

> +

> +	return i2c_get_dma_safe_msg_buf(msg, 32);

> +}

> +

>  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

>  				u32 m_param)

>  {

> @@ -359,7 +367,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

>  	struct geni_se *se = &gi2c->se;

>  	size_t len = msg->len;

>  

> -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> +	dma_buf = get_dma_buf(se, msg);

>  	if (dma_buf)

>  		geni_se_select_mode(se, GENI_SE_DMA);

>  	else

> @@ -398,7 +406,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

>  	struct geni_se *se = &gi2c->se;

>  	size_t len = msg->len;

>  

> -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> +	dma_buf = get_dma_buf(se, msg);

>  	if (dma_buf)

>  		geni_se_select_mode(se, GENI_SE_DMA);

>  	else


-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones Sept. 5, 2019, 9:28 a.m. UTC | #3 
On Thu, 05 Sep 2019, Wolfram Sang wrote:

> 

> > Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")

> 

> Are you sure? From visual inspection, I don't see a correlation between

> this commit and the fix here.


This patch should have been part of the commit, or at the very least,
part of the set, alluded to above.  Unfortunately, I was carrying
Bjorn's hack which simply returned early from geni_se_rx_dma_prep()
with an error, so it masked the issue.

> > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > Reviewed-by: Vinod Koul <vkoul@kernel.org>

> > ---

> >  drivers/i2c/busses/i2c-qcom-geni.c | 14 ++++++++++----

> >  1 file changed, 10 insertions(+), 4 deletions(-)

> > 

> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c

> > index a89bfce5388e..8822dea82980 100644

> > --- a/drivers/i2c/busses/i2c-qcom-geni.c

> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c

> > @@ -353,13 +353,16 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)

> >  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

> >  				u32 m_param)

> >  {

> > +	struct device_node *np = gi2c->se.dev->of_node;

> >  	dma_addr_t rx_dma;

> >  	unsigned long time_left;

> > -	void *dma_buf;

> > +	void *dma_buf = NULL;

> >  	struct geni_se *se = &gi2c->se;

> >  	size_t len = msg->len;

> >  

> > -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> > +	if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))

> > +		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> > +

> >  	if (dma_buf)

> >  		geni_se_select_mode(se, GENI_SE_DMA);

> >  	else

> > @@ -392,13 +395,16 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

> >  static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,

> >  				u32 m_param)

> >  {

> > +	struct device_node *np = gi2c->se.dev->of_node;

> >  	dma_addr_t tx_dma;

> >  	unsigned long time_left;

> > -	void *dma_buf;

> > +	void *dma_buf = NULL;

> >  	struct geni_se *se = &gi2c->se;

> >  	size_t len = msg->len;

> >  

> > -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> > +	if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))

> > +		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);

> > +

> >  	if (dma_buf)

> >  		geni_se_select_mode(se, GENI_SE_DMA);

> >  	else




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

Patch

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index a89bfce5388e..8822dea82980 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -353,13 +353,16 @@  static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
 static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 				u32 m_param)
 {
+	struct device_node *np = gi2c->se.dev->of_node;
 	dma_addr_t rx_dma;
 	unsigned long time_left;
-	void *dma_buf;
+	void *dma_buf = NULL;
 	struct geni_se *se = &gi2c->se;
 	size_t len = msg->len;
 
-	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+	if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))
+		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+
 	if (dma_buf)
 		geni_se_select_mode(se, GENI_SE_DMA);
 	else
@@ -392,13 +395,16 @@  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 				u32 m_param)
 {
+	struct device_node *np = gi2c->se.dev->of_node;
 	dma_addr_t tx_dma;
 	unsigned long time_left;
-	void *dma_buf;
+	void *dma_buf = NULL;
 	struct geni_se *se = &gi2c->se;
 	size_t len = msg->len;
 
-	dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+	if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))
+		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+
 	if (dma_buf)
 		geni_se_select_mode(se, GENI_SE_DMA);
 	else