| Message ID | 20190905075213.13260-2-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: soc: qcom: Provide option to disable DMA | expand |
[ 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
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
> 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. > 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 > -- > 2.17.1 >
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
On Thu, Sep 05, 2019 at 10:28:16AM +0100, Lee Jones wrote: > 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. I still don't see why this basic ACPI enabling code (not touching DMA but only clocks and pinctrl) causes and additional handling for DMA. Am I overlooking something obvious?
On Thu, 05 Sep 2019, Wolfram Sang wrote: > On Thu, Sep 05, 2019 at 10:28:16AM +0100, Lee Jones wrote: > > 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. > > I still don't see why this basic ACPI enabling code (not touching DMA > but only clocks and pinctrl) causes and additional handling for DMA. Am > I overlooking something obvious? Please ignore, I'm discussing with another patch in mind.
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
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(-)