| Message ID | 20220407150828.202513-1-michael@walle.cc |
|---|---|
| State | Accepted |
| Commit | 03fbb903c8bf7e53e101e8d9a7b261264317c411 |
| Headers | show |
| Series | [v2] i2c: at91: use dma safe buffers | expand |
Am 2022-04-07 17:08, schrieb Michael Walle: > The supplied buffer might be on the stack and we get the following > error > message: > [ 3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc > memory > > Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region if > necessary. > > Fixes: 60937b2cdbf9 ("i2c: at91: add dma support") > Signed-off-by: Michael Walle <michael@walle.cc> > Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Wolfgang, will you pick this one? -michael
Am 2022-05-04 22:23, schrieb Wolfram Sang:
> BTW, my name is Wolfram.
Sorry :/
-michael
On Thu, Apr 07, 2022 at 05:08:28PM +0200, Michael Walle wrote: > The supplied buffer might be on the stack and we get the following error > message: > [ 3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc memory > > Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region if > necessary. > > Fixes: 60937b2cdbf9 ("i2c: at91: add dma support") > Signed-off-by: Michael Walle <michael@walle.cc> > Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > --- > changes since v1: > - remove extra empty line > - add fixes tag as suggested by Codrin > > drivers/i2c/busses/i2c-at91-master.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c > index b0eae94909f4..5eca3b3bb609 100644 > --- a/drivers/i2c/busses/i2c-at91-master.c > +++ b/drivers/i2c/busses/i2c-at91-master.c > @@ -656,6 +656,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num) > unsigned int_addr_flag = 0; > struct i2c_msg *m_start = msg; > bool is_read; > + u8 *dma_buf; > > dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num); > > @@ -703,7 +704,17 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num) > dev->msg = m_start; > dev->recv_len_abort = false; > > + if (dev->use_dma) { > + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); > + if (!dma_buf) { > + ret = -ENOMEM; > + goto out; > + } > + dev->buf = dma_buf; > + } > + > ret = at91_do_twi_transfer(dev); > + i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret); > > ret = (ret < 0) ? ret : num; > out: > -- > 2.30.2 > > This change as commit 03fbb903c8bf ("i2c: at91: use dma safe buffers") causes the following clang warning: drivers/i2c/busses/i2c-at91-master.c:707:6: error: variable 'dma_buf' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] if (dev->use_dma) { ^~~~~~~~~~~~ drivers/i2c/busses/i2c-at91-master.c:717:27: note: uninitialized use occurs here i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret); ^~~~~~~ drivers/i2c/busses/i2c-at91-master.c:707:2: note: remove the 'if' if its condition is always true if (dev->use_dma) { ^~~~~~~~~~~~~~~~~~ drivers/i2c/busses/i2c-at91-master.c:659:13: note: initialize the variable 'dma_buf' to silence this warning u8 *dma_buf; ^ = NULL 1 error generated. Should this variable be initialized or should the call to i2c_put_dma_safe_msg_buf() be moved into the if statement? Cheers, Nathan
[+ Wolfram] Am 2022-05-05 16:51, schrieb Nathan Chancellor: > On Thu, Apr 07, 2022 at 05:08:28PM +0200, Michael Walle wrote: >> The supplied buffer might be on the stack and we get the following >> error >> message: >> [ 3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc >> memory >> >> Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region >> if >> necessary. >> >> Fixes: 60937b2cdbf9 ("i2c: at91: add dma support") >> Signed-off-by: Michael Walle <michael@walle.cc> >> Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> --- >> changes since v1: >> - remove extra empty line >> - add fixes tag as suggested by Codrin >> >> drivers/i2c/busses/i2c-at91-master.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-at91-master.c >> b/drivers/i2c/busses/i2c-at91-master.c >> index b0eae94909f4..5eca3b3bb609 100644 >> --- a/drivers/i2c/busses/i2c-at91-master.c >> +++ b/drivers/i2c/busses/i2c-at91-master.c >> @@ -656,6 +656,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, >> struct i2c_msg *msg, int num) >> unsigned int_addr_flag = 0; >> struct i2c_msg *m_start = msg; >> bool is_read; >> + u8 *dma_buf; >> >> dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num); >> >> @@ -703,7 +704,17 @@ static int at91_twi_xfer(struct i2c_adapter >> *adap, struct i2c_msg *msg, int num) >> dev->msg = m_start; >> dev->recv_len_abort = false; >> >> + if (dev->use_dma) { >> + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); >> + if (!dma_buf) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + dev->buf = dma_buf; >> + } >> + >> ret = at91_do_twi_transfer(dev); >> + i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret); >> >> ret = (ret < 0) ? ret : num; >> out: >> -- >> 2.30.2 >> >> > > This change as commit 03fbb903c8bf ("i2c: at91: use dma safe buffers") > causes the following clang warning: Ugh.. > drivers/i2c/busses/i2c-at91-master.c:707:6: error: variable 'dma_buf' > is used uninitialized whenever 'if' condition is false > [-Werror,-Wsometimes-uninitialized] > if (dev->use_dma) { > ^~~~~~~~~~~~ > drivers/i2c/busses/i2c-at91-master.c:717:27: note: uninitialized use > occurs here > i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret); > ^~~~~~~ > drivers/i2c/busses/i2c-at91-master.c:707:2: note: remove the 'if' if > its condition is always true > if (dev->use_dma) { > ^~~~~~~~~~~~~~~~~~ > drivers/i2c/busses/i2c-at91-master.c:659:13: note: initialize the > variable 'dma_buf' to silence this warning > u8 *dma_buf; > ^ > = NULL > 1 error generated. > > Should this variable be initialized or should the call to > i2c_put_dma_safe_msg_buf() be moved into the if statement? Initializing dma_buf with NULL should to the trick because i2c_put_dma_safe_msg_buf() is handling NULL gracefully. No need for an extra "if (dma_buf)". Will you take care or should I send a patch? -michael
On Thu, May 05, 2022 at 05:11:30PM +0200, Michael Walle wrote: > [+ Wolfram] > > Am 2022-05-05 16:51, schrieb Nathan Chancellor: > > On Thu, Apr 07, 2022 at 05:08:28PM +0200, Michael Walle wrote: > > > The supplied buffer might be on the stack and we get the following > > > error > > > message: > > > [ 3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc > > > memory > > > > > > Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region > > > if > > > necessary. > > > > > > Fixes: 60937b2cdbf9 ("i2c: at91: add dma support") > > > Signed-off-by: Michael Walle <michael@walle.cc> > > > Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > > > --- > > > changes since v1: > > > - remove extra empty line > > > - add fixes tag as suggested by Codrin > > > > > > drivers/i2c/busses/i2c-at91-master.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/drivers/i2c/busses/i2c-at91-master.c > > > b/drivers/i2c/busses/i2c-at91-master.c > > > index b0eae94909f4..5eca3b3bb609 100644 > > > --- a/drivers/i2c/busses/i2c-at91-master.c > > > +++ b/drivers/i2c/busses/i2c-at91-master.c > > > @@ -656,6 +656,7 @@ static int at91_twi_xfer(struct i2c_adapter > > > *adap, struct i2c_msg *msg, int num) > > > unsigned int_addr_flag = 0; > > > struct i2c_msg *m_start = msg; > > > bool is_read; > > > + u8 *dma_buf; > > > > > > dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num); > > > > > > @@ -703,7 +704,17 @@ static int at91_twi_xfer(struct i2c_adapter > > > *adap, struct i2c_msg *msg, int num) > > > dev->msg = m_start; > > > dev->recv_len_abort = false; > > > > > > + if (dev->use_dma) { > > > + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); > > > + if (!dma_buf) { > > > + ret = -ENOMEM; > > > + goto out; > > > + } > > > + dev->buf = dma_buf; > > > + } > > > + > > > ret = at91_do_twi_transfer(dev); > > > + i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret); > > > > > > ret = (ret < 0) ? ret : num; > > > out: > > > -- > > > 2.30.2 > > > > > > > > > > This change as commit 03fbb903c8bf ("i2c: at91: use dma safe buffers") > > causes the following clang warning: > > Ugh.. > > > drivers/i2c/busses/i2c-at91-master.c:707:6: error: variable 'dma_buf' > > is used uninitialized whenever 'if' condition is false > > [-Werror,-Wsometimes-uninitialized] > > if (dev->use_dma) { > > ^~~~~~~~~~~~ > > drivers/i2c/busses/i2c-at91-master.c:717:27: note: uninitialized use > > occurs here > > i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret); > > ^~~~~~~ > > drivers/i2c/busses/i2c-at91-master.c:707:2: note: remove the 'if' if > > its condition is always true > > if (dev->use_dma) { > > ^~~~~~~~~~~~~~~~~~ > > drivers/i2c/busses/i2c-at91-master.c:659:13: note: initialize the > > variable 'dma_buf' to silence this warning > > u8 *dma_buf; > > ^ > > = NULL > > 1 error generated. > > > > Should this variable be initialized or should the call to > > i2c_put_dma_safe_msg_buf() be moved into the if statement? > > Initializing dma_buf with NULL should to the trick because > i2c_put_dma_safe_msg_buf() is handling NULL gracefully. No > need for an extra "if (dma_buf)". > > Will you take care or should I send a patch? I can send a patch, thank you for the input! Cheers, Nathan
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c index b0eae94909f4..5eca3b3bb609 100644 --- a/drivers/i2c/busses/i2c-at91-master.c +++ b/drivers/i2c/busses/i2c-at91-master.c @@ -656,6 +656,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num) unsigned int_addr_flag = 0; struct i2c_msg *m_start = msg; bool is_read; + u8 *dma_buf; dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num); @@ -703,7 +704,17 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num) dev->msg = m_start; dev->recv_len_abort = false; + if (dev->use_dma) { + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); + if (!dma_buf) { + ret = -ENOMEM; + goto out; + } + dev->buf = dma_buf; + } + ret = at91_do_twi_transfer(dev); + i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret); ret = (ret < 0) ? ret : num; out:
The supplied buffer might be on the stack and we get the following error message: [ 3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc memory Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region if necessary. Fixes: 60937b2cdbf9 ("i2c: at91: add dma support") Signed-off-by: Michael Walle <michael@walle.cc> Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> --- changes since v1: - remove extra empty line - add fixes tag as suggested by Codrin drivers/i2c/busses/i2c-at91-master.c | 11 +++++++++++ 1 file changed, 11 insertions(+)