Message ID | 20220303161724.3324948-1-michael@walle.cc (mailing list archive) |
---|---|
State | Not Applicable |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1nPo9T-00752O-3A; Thu, 03 Mar 2022 16:18:04 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234958AbiCCQSp (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 3 Mar 2022 11:18:45 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34514 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234921AbiCCQS1 (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 3 Mar 2022 11:18:27 -0500 Received: from ssl.serverraum.org (ssl.serverraum.org [IPv6:2a01:4f8:151:8464::1:2]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AC01FFD26; Thu, 3 Mar 2022 08:17:40 -0800 (PST) Received: from mwalle01.kontron.local. (unknown [213.135.10.150]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ssl.serverraum.org (Postfix) with ESMTPSA id 49F902223B; Thu, 3 Mar 2022 17:17:29 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=walle.cc; s=mail2016061301; t=1646324250; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=PFcCtGt63rBp0kucxIHsntg/HdnJkcA4oD5l63d9a3E=; b=JbU6lT9gA0vzBY0dz3e94kJ/OGyeZJZSJbYWL+9fPmX7eKB6GJwz6P+MsJyn2SN0XoS1vF hr9Jc10NWzX5jilTfcOPebKpTC9S9c47WGfEPHwdX5E+uvK6La+F+gZp2x7b7mgH2aBxBX 95jxKtw4oDFeZ1hQ1aEF6AlUXePndvw= From: Michael Walle <michael@walle.cc> To: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>, Nicolas Ferre <nicolas.ferre@microchip.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Claudiu Beznea <claudiu.beznea@microchip.com>, Sumit Semwal <sumit.semwal@linaro.org>, =?utf-8?q?Christian_K=C3=B6nig?= <christian.koenig@amd.com> Cc: linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, Michael Walle <michael@walle.cc>, stable@vger.kernel.org Subject: [PATCH] i2c: at91: use dma safe buffers Date: Thu, 3 Mar 2022 17:17:24 +0100 Message-Id: <20220303161724.3324948-1-michael@walle.cc> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_NONE=-0.0001 autolearn=ham autolearn_force=no |
Series |
i2c: at91: use dma safe buffers
|
|
Commit Message
Michael Walle
March 3, 2022, 4:17 p.m. UTC
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(+)
Comments
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: