| Message ID | 20220303161724.3324948-1-michael@walle.cc |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: at91: use dma safe buffers | expand |
Am 03.03.22 um 17:17 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. > > Cc: stable@vger.kernel.org > Signed-off-by: Michael Walle <michael@walle.cc> > --- > > I'm not sure if or which Fixes: tag I should add to this patch. The issue > seems to be since a very long time, but nobody seem to have triggered it. > FWIW, I'm using the sff,sfp driver, which triggers this. > > drivers/i2c/busses/i2c-at91-master.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c > index b0eae94909f4..a7a22fedbaba 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; Maybe call your variable differently. DMA-buf is an inter driver buffer sharing frame we use for GPU acceleration and V4L. It doesn't cause any technical issues, but the maintainer regex now triggers on that. So you are CCing people not related to this code in any way. Regards, Christian. > > dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num); > > @@ -703,7 +704,18 @@ 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:
Hi Christian, > Maybe call your variable differently. DMA-buf is an inter driver buffer > sharing frame we use for GPU acceleration and V4L. > > It doesn't cause any technical issues, but the maintainer regex now triggers > on that. So you are CCing people not related to this code in any way. Frankly, I think the 'dma_buf' regex is a bit too generic. 'dma_buf' seems like a reasonable name to me if some subsystem has to deal with different buffers which can be DMA or non-DMA, like I2C. If you git-grep the tree, you will find it in quite some places. We could now think of renaming the variable to 'dmabuf' but this is a strange and kind of arbitrary rule to remember IMO. I wonder if you'd miss a lot of patches if we remove 'dma_buf' from the regex and keep 'dma_fence' and 'dma_resv'? Or extend it to 'dma_buf_' or 'struct dma_buf'? All the best, Wolfram
Am 04.03.22 um 09:04 schrieb Wolfram Sang: > Hi Christian, > >> Maybe call your variable differently. DMA-buf is an inter driver buffer >> sharing frame we use for GPU acceleration and V4L. >> >> It doesn't cause any technical issues, but the maintainer regex now triggers >> on that. So you are CCing people not related to this code in any way. > Frankly, I think the 'dma_buf' regex is a bit too generic. 'dma_buf' > seems like a reasonable name to me if some subsystem has to deal with > different buffers which can be DMA or non-DMA, like I2C. If you git-grep > the tree, you will find it in quite some places. > > We could now think of renaming the variable to 'dmabuf' but this is > a strange and kind of arbitrary rule to remember IMO. > > I wonder if you'd miss a lot of patches if we remove 'dma_buf' from the > regex and keep 'dma_fence' and 'dma_resv'? Or extend it to 'dma_buf_' or > 'struct dma_buf'? Yeah, I'm already considering something similar for a while. I'm getting quite a bunch of unrelated mails because the regex is not the best. On the other hand the framework is used in a lot of drivers and I do want to be notified when they mess with their interfaces. Going to take a another look at that when I have time. Thanks, Christian. > > All the best, > > Wolfram >
> I'm getting quite a bunch of unrelated mails because the regex is not the > best. I can imagine! > On the other hand the framework is used in a lot of drivers and I do want to > be notified when they mess with their interfaces. Sure thing. I am convinced the regex can be improved to ensure that to a high degree. I think it is also less work for you than asking people to rename their variable all the time :) > Going to take a another look at that when I have time. Thank you!
Am 04.03.22 um 09:43 schrieb Wolfram Sang: >> I'm getting quite a bunch of unrelated mails because the regex is not the >> best. > I can imagine! > >> On the other hand the framework is used in a lot of drivers and I do want to >> be notified when they mess with their interfaces. > Sure thing. I am convinced the regex can be improved to ensure that to a > high degree. I think it is also less work for you than asking people to > rename their variable all the time :) Well not all the time. It's just that you absolutely hit the nail on the head with the name. The local variable for the DMA-buf handle is just nearly always named dma_buf or dmabuf in the drivers. Christian. > >> Going to take a another look at that when I have time. > Thank you! >
Hi all, Am 2022-03-03 17:17, 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. > > Cc: stable@vger.kernel.org > Signed-off-by: Michael Walle <michael@walle.cc> Any news here? > --- > > I'm not sure if or which Fixes: tag I should add to this patch. The > issue > seems to be since a very long time, but nobody seem to have triggered > it. > FWIW, I'm using the sff,sfp driver, which triggers this. -michael
On 03.03.2022 18:17, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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. > > Cc: stable@vger.kernel.org > Signed-off-by: Michael Walle <michael@walle.cc> Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > --- > > I'm not sure if or which Fixes: tag I should add to this patch. The issue > seems to be since a very long time, but nobody seem to have triggered it. > FWIW, I'm using the sff,sfp driver, which triggers this. I think it should be: Fixes: 60937b2cdbf9 ("i2c: at91: add dma support") > + if (dev->use_dma) { > + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); If you want, you could just dev->buf = i2c_get_dma_safe... > + if (!dma_buf) { > + ret = -ENOMEM; > + goto out; > + } > + dev->buf = dma_buf; Thanks!
Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com: > On 03.03.2022 18:17, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> 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. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Michael Walle <michael@walle.cc> > > Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Thanks! >> I'm not sure if or which Fixes: tag I should add to this patch. The >> issue >> seems to be since a very long time, but nobody seem to have triggered >> it. >> FWIW, I'm using the sff,sfp driver, which triggers this. > > I think it should be: > Fixes: 60937b2cdbf9 ("i2c: at91: add dma support") > >> + if (dev->use_dma) { >> + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); > > If you want, you could just dev->buf = i2c_get_dma_safe... But where is the error handling in that case? dev->buf will be NULL, which is eventually passed to dma_map_single(). Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf() call anyway, because dev->buf will be modified during processing. -michael
On 05.04.2022 12:38, Michael Walle wrote: > Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com: >>> + if (dev->use_dma) { >>> + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); >> >> If you want, you could just dev->buf = i2c_get_dma_safe... > > But where is the error handling in that case? dev->buf will > be NULL, which is eventually passed to dma_map_single(). > > Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf() > call anyway, because dev->buf will be modified during > processing. You still: if (!dev->buf) { ret = -ENOMEM; goto out; } So, at91_do_twi_transfer()/dma_map_single() will not be called. Best regards, Codrin
Am 2022-04-05 12:02, schrieb Codrin.Ciubotariu@microchip.com: > On 05.04.2022 12:38, Michael Walle wrote: >> Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com: >>>> + if (dev->use_dma) { >>>> + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); >>> >>> If you want, you could just dev->buf = i2c_get_dma_safe... >> >> But where is the error handling in that case? dev->buf will >> be NULL, which is eventually passed to dma_map_single(). >> >> Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf() >> call anyway, because dev->buf will be modified during >> processing. > > You still: > if (!dev->buf) { > ret = -ENOMEM; > goto out; > } > > So, at91_do_twi_transfer()/dma_map_single() will not be called. Ahh, I misunderstood you. Yes, but as I said, I need the dma_buf temporary variable anyway, because dev->buf is modified, eg. see at91_twi_read_data_dma_callback(). -michael
On 05.04.2022 14:09, Michael Walle wrote: > Am 2022-04-05 12:02, schrieb Codrin.Ciubotariu@microchip.com: >> On 05.04.2022 12:38, Michael Walle wrote: >>> Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com: >>>>> + if (dev->use_dma) { >>>>> + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); >>>> >>>> If you want, you could just dev->buf = i2c_get_dma_safe... >>> >>> But where is the error handling in that case? dev->buf will >>> be NULL, which is eventually passed to dma_map_single(). >>> >>> Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf() >>> call anyway, because dev->buf will be modified during >>> processing. >> >> You still: >> if (!dev->buf) { >> ret = -ENOMEM; >> goto out; >> } >> >> So, at91_do_twi_transfer()/dma_map_single() will not be called. > > Ahh, I misunderstood you. Yes, but as I said, I need the dma_buf > temporary variable anyway, because dev->buf is modified, eg. see > at91_twi_read_data_dma_callback(). at91_twi_read_data_dma_callback() is called as callback if dma_async_issue_pending(dma->chan_rx) is called. dma_async_issue_pending(dma->chan_rx) is called on at91_twi_read_data_dma(), which is called in at91_do_twi_transfer(), which we decided above to skip in case of error.
Am 2022-04-05 15:58, schrieb Codrin.Ciubotariu@microchip.com: > On 05.04.2022 14:09, Michael Walle wrote: >> Am 2022-04-05 12:02, schrieb Codrin.Ciubotariu@microchip.com: >>> On 05.04.2022 12:38, Michael Walle wrote: >>>> Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com: >>>>>> + if (dev->use_dma) { >>>>>> + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); >>>>> >>>>> If you want, you could just dev->buf = i2c_get_dma_safe... >>>> >>>> But where is the error handling in that case? dev->buf will >>>> be NULL, which is eventually passed to dma_map_single(). >>>> >>>> Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf() >>>> call anyway, because dev->buf will be modified during >>>> processing. >>> >>> You still: >>> if (!dev->buf) { >>> ret = -ENOMEM; >>> goto out; >>> } >>> >>> So, at91_do_twi_transfer()/dma_map_single() will not be called. >> >> Ahh, I misunderstood you. Yes, but as I said, I need the dma_buf >> temporary variable anyway, because dev->buf is modified, eg. see >> at91_twi_read_data_dma_callback(). > at91_twi_read_data_dma_callback() is called as callback if > dma_async_issue_pending(dma->chan_rx) is called. > dma_async_issue_pending(dma->chan_rx) is called on > at91_twi_read_data_dma(), which is called in at91_do_twi_transfer(), > which we decided above to skip in case of error. It is not about errors, you need the exact same pointer you got from i2c_get_dma_safe_msg_buf() to be passed to i2c_put_dma_safe_msg_buf(). And because (in some cases, it isn't really obvious) the dev->buf will be advanced a few bytes, I cannot pass dev->buf to i2c_put_dma_safe_msg_buf(). -michael
On 05.04.2022 17:08, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know > the content is safe > > Am 2022-04-05 15:58, schrieb Codrin.Ciubotariu@microchip.com: >> On 05.04.2022 14:09, Michael Walle wrote: >>> Am 2022-04-05 12:02, schrieb Codrin.Ciubotariu@microchip.com: >>>> On 05.04.2022 12:38, Michael Walle wrote: >>>>> Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com: >>>>>>> + if (dev->use_dma) { >>>>>>> + dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1); >>>>>> >>>>>> If you want, you could just dev->buf = i2c_get_dma_safe... >>>>> >>>>> But where is the error handling in that case? dev->buf will >>>>> be NULL, which is eventually passed to dma_map_single(). >>>>> >>>>> Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf() >>>>> call anyway, because dev->buf will be modified during >>>>> processing. >>>> >>>> You still: >>>> if (!dev->buf) { >>>> ret = -ENOMEM; >>>> goto out; >>>> } >>>> >>>> So, at91_do_twi_transfer()/dma_map_single() will not be called. >>> >>> Ahh, I misunderstood you. Yes, but as I said, I need the dma_buf >>> temporary variable anyway, because dev->buf is modified, eg. see >>> at91_twi_read_data_dma_callback(). >> at91_twi_read_data_dma_callback() is called as callback if >> dma_async_issue_pending(dma->chan_rx) is called. >> dma_async_issue_pending(dma->chan_rx) is called on >> at91_twi_read_data_dma(), which is called in at91_do_twi_transfer(), >> which we decided above to skip in case of error. > > It is not about errors, you need the exact same pointer you > got from i2c_get_dma_safe_msg_buf() to be passed to > i2c_put_dma_safe_msg_buf(). And because (in some cases, it > isn't really obvious) the dev->buf will be advanced a few > bytes, I cannot pass dev->buf to i2c_put_dma_safe_msg_buf(). You are right, when dev->use_dma && (dev->buf_len <= AT91_I2C_DMA_THRESHOLD) got it. Thanks! Best regards, Codrin
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c index b0eae94909f4..a7a22fedbaba 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,18 @@ 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. Cc: stable@vger.kernel.org Signed-off-by: Michael Walle <michael@walle.cc> --- I'm not sure if or which Fixes: tag I should add to this patch. The issue seems to be since a very long time, but nobody seem to have triggered it. FWIW, I'm using the sff,sfp driver, which triggers this. drivers/i2c/busses/i2c-at91-master.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)