| Message ID | 20170718102339.28726-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Wolfram, On 2017-07-18 12:23:36 +0200, Wolfram Sang wrote: > One helper checks if DMA is suitable and optionally creates a bounce > buffer, if not. The other function returns the bounce buffer and makes > sure the data is properly copied back to the message. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > Changes since v2: > > * rebased to v4.13-rc1 > * helper functions are not inlined anymore but moved to i2c core > * __must_check has been added to the buffer check helper > * the release function has been renamed to contain 'dma' as well > > drivers/i2c/i2c-core-base.c | 68 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c.h | 5 ++++ > 2 files changed, 73 insertions(+) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index c89dac7fd2e7b7..7326a9d2e4eb69 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -34,6 +34,7 @@ > #include <linux/irqflags.h> > #include <linux/jump_label.h> > #include <linux/kernel.h> > +#include <linux/mm.h> > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/of_device.h> > @@ -44,6 +45,7 @@ > #include <linux/pm_wakeirq.h> > #include <linux/property.h> > #include <linux/rwsem.h> > +#include <linux/sched/task_stack.h> > #include <linux/slab.h> > > #include "i2c-core.h" > @@ -2240,6 +2242,72 @@ void i2c_put_adapter(struct i2c_adapter *adap) > } > EXPORT_SYMBOL(i2c_put_adapter); > > +/** > + * i2c_check_msg_for_dma() - check if a message is suitable for DMA > + * @msg: the message to be checked > + * @threshold: the amount of byte from which using DMA makes sense > + * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this > + * ptr, if needed. The bounce buffer must be freed by the > + * caller using i2c_release_dma_bounce_buf(). > + * > + * Return: -ERANGE if message is smaller than threshold > + * -EFAULT if message buffer is not DMA capable and no bounce buffer > + * was requested > + * -ENOMEM if a bounce buffer could not be created > + * 0 if message is suitable for DMA > + * > + * The return value must be checked. > + * > + * Note: This function should only be called from process context! It uses > + * helper functions which work on the 'current' task. > + */ > +int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold, > + u8 **ptr_for_bounce_buf) > +{ > + if (ptr_for_bounce_buf) > + *ptr_for_bounce_buf = NULL; > + > + if (msg->len < threshold) > + return -ERANGE; > + > + if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) { > + pr_debug("msg buffer to 0x%04x is not DMA safe%s\n", msg->addr, > + ptr_for_bounce_buf ? ", trying bounce buffer" : ""); > + if (ptr_for_bounce_buf) { > + if (msg->flags & I2C_M_RD) > + *ptr_for_bounce_buf = kzalloc(msg->len, GFP_KERNEL); > + else > + *ptr_for_bounce_buf = kmemdup(msg->buf, msg->len, > + GFP_KERNEL); > + if (!*ptr_for_bounce_buf) > + return -ENOMEM; > + } else { > + return -EFAULT; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(i2c_check_msg_for_dma); > + > +/** > + * i2c_release_bounce_buf - copy data back from bounce buffer and release it > + * @msg: the message to be copied back to > + * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma(). > + * May be NULL. > + */ > +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf) > +{ > + if (!bounce_buf) > + return; > + > + if (msg->flags & I2C_M_RD) > + memcpy(msg->buf, bounce_buf, msg->len); > + > + kfree(bounce_buf); > +} > +EXPORT_SYMBOL_GPL(i2c_release_bounce_buf); > + > MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); > MODULE_DESCRIPTION("I2C-Bus main module"); > MODULE_LICENSE("GPL"); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 00ca5b86a753f8..ac02287b6c0d8f 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -766,6 +766,11 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg) > return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0); > } > > +int __must_check i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold, > + u8 **ptr_for_bounce_buf); > + > +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf); > + > int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr); > /** > * module_i2c_driver() - Helper macro for registering a modular I2C driver > -- > 2.11.0 >
On Tue, 18 Jul 2017 12:23:36 +0200 Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > One helper checks if DMA is suitable and optionally creates a bounce > buffer, if not. The other function returns the bounce buffer and makes > sure the data is properly copied back to the message. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> My knowledge of the exact requirements for dma on all platforms isn't all that good. Mostly it's based around the assumption that if one forces a heap allocation and makes sure nothing else is in the cacheline all is good. I like the basic idea of this patch set a lot btw! Jonathan > --- > Changes since v2: > > * rebased to v4.13-rc1 > * helper functions are not inlined anymore but moved to i2c core > * __must_check has been added to the buffer check helper > * the release function has been renamed to contain 'dma' as well > > drivers/i2c/i2c-core-base.c | 68 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c.h | 5 ++++ > 2 files changed, 73 insertions(+) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index c89dac7fd2e7b7..7326a9d2e4eb69 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -34,6 +34,7 @@ > #include <linux/irqflags.h> > #include <linux/jump_label.h> > #include <linux/kernel.h> > +#include <linux/mm.h> > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/of_device.h> > @@ -44,6 +45,7 @@ > #include <linux/pm_wakeirq.h> > #include <linux/property.h> > #include <linux/rwsem.h> > +#include <linux/sched/task_stack.h> > #include <linux/slab.h> > > #include "i2c-core.h" > @@ -2240,6 +2242,72 @@ void i2c_put_adapter(struct i2c_adapter *adap) > } > EXPORT_SYMBOL(i2c_put_adapter); > > +/** > + * i2c_check_msg_for_dma() - check if a message is suitable for DMA > + * @msg: the message to be checked > + * @threshold: the amount of byte from which using DMA makes sense > + * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this > + * ptr, if needed. The bounce buffer must be freed by the > + * caller using i2c_release_dma_bounce_buf(). > + * > + * Return: -ERANGE if message is smaller than threshold > + * -EFAULT if message buffer is not DMA capable and no bounce buffer > + * was requested > + * -ENOMEM if a bounce buffer could not be created > + * 0 if message is suitable for DMA > + * > + * The return value must be checked. > + * > + * Note: This function should only be called from process context! It uses > + * helper functions which work on the 'current' task. > + */ > +int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold, > + u8 **ptr_for_bounce_buf) > +{ > + if (ptr_for_bounce_buf) > + *ptr_for_bounce_buf = NULL; > + > + if (msg->len < threshold) > + return -ERANGE; > + > + if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) { > + pr_debug("msg buffer to 0x%04x is not DMA safe%s\n", msg->addr, > + ptr_for_bounce_buf ? ", trying bounce buffer" : ""); > + if (ptr_for_bounce_buf) { > + if (msg->flags & I2C_M_RD) > + *ptr_for_bounce_buf = kzalloc(msg->len, GFP_KERNEL); > + else > + *ptr_for_bounce_buf = kmemdup(msg->buf, msg->len, > + GFP_KERNEL); > + if (!*ptr_for_bounce_buf) > + return -ENOMEM; > + } else { > + return -EFAULT; > + } > + } I'm trying to get my head around whether this is a sufficient set of conditions for a dma safe buffer and I'm not sure it is. We go to a lot of effort with SPI buffers to ensure they are in their own cache lines. Do we not need to do the same here? The issue would be the classic case of embedding a buffer inside a larger structure which is on the stack. Need the magic of __cacheline_aligned to force it into it's own line for devices with dma-incoherent caches. DMA-API-HOWTO.txt (not read that for a while at it is a rather good at describing this stuff these days!) So that one is easy enough to check by checking if it is cache line aligned, but what do you do to know there is nothing much after it? Not sure there is a way around this short of auditing i2c drivers to change any that do this. > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(i2c_check_msg_for_dma); > + > +/** > + * i2c_release_bounce_buf - copy data back from bounce buffer and release it > + * @msg: the message to be copied back to > + * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma(). > + * May be NULL. > + */ > +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf) > +{ > + if (!bounce_buf) > + return; > + > + if (msg->flags & I2C_M_RD) > + memcpy(msg->buf, bounce_buf, msg->len); > + > + kfree(bounce_buf); > +} > +EXPORT_SYMBOL_GPL(i2c_release_bounce_buf); > + > MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); > MODULE_DESCRIPTION("I2C-Bus main module"); > MODULE_LICENSE("GPL"); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 00ca5b86a753f8..ac02287b6c0d8f 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -766,6 +766,11 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg) > return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0); > } > > +int __must_check i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold, > + u8 **ptr_for_bounce_buf); > + > +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf); > + > int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr); > /** > * module_i2c_driver() - Helper macro for registering a modular I2C driver
Hi Wolfram, On Tue, Jul 18, 2017 at 12:23 PM, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > One helper checks if DMA is suitable and optionally creates a bounce > buffer, if not. The other function returns the bounce buffer and makes > sure the data is properly copied back to the message. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > Changes since v2: > > * rebased to v4.13-rc1 > * helper functions are not inlined anymore but moved to i2c core > * __must_check has been added to the buffer check helper > * the release function has been renamed to contain 'dma' as well Right: drivers/i2c/i2c-core-base.c:2310:15: error: 'i2c_release_bounce_buf' undeclared here (not in a function) EXPORT_SYMBOL_GPL(i2c_release_bounce_buf); > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > +/** > + * i2c_release_bounce_buf - copy data back from bounce buffer and release it ^^^^^^^^^^^^^^^^^^^^^^ > + * @msg: the message to be copied back to > + * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma(). > + * May be NULL. > + */ > +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf) > +{ > + if (!bounce_buf) > + return; > + > + if (msg->flags & I2C_M_RD) > + memcpy(msg->buf, bounce_buf, msg->len); > + > + kfree(bounce_buf); > +} > +EXPORT_SYMBOL_GPL(i2c_release_bounce_buf); ^^^^^^^^^^^^^^^^^^^^^^ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> Right: > > drivers/i2c/i2c-core-base.c:2310:15: error: 'i2c_release_bounce_buf' > undeclared here (not in a function) > EXPORT_SYMBOL_GPL(i2c_release_bounce_buf); Thanks. I am just now working on V4 currently which is a redesign. I'll write more in an hour or so.
Hi Jonathan, > I like the basic idea of this patch set a lot btw! Thanks! > Jonathan Could you delete irrelevant parts of the message, please? I nearly missed your other comment which would have been a great loss! > I'm trying to get my head around whether this is a sufficient set of conditions > for a dma safe buffer and I'm not sure it is. > > We go to a lot of effort with SPI buffers to ensure they are in their own cache > lines. Do we not need to do the same here? The issue would be the > classic case of embedding a buffer inside a larger structure which is on > the stack. Need the magic of __cacheline_aligned to force it into it's > own line for devices with dma-incoherent caches. > DMA-API-HOWTO.txt (not read that for a while at it is a rather good > at describing this stuff these days!) > > So that one is easy enough to check by checking if it is cache line aligned, > but what do you do to know there is nothing much after it? Thank you, really! This patch series addressed all cases I have created, but I also wondered if there are cases left which I missed. Now, also because you mentioned cacheline alignments, the faint idea of "maybe it could be fragile" became a strong "it is too fragile"! lib/dma-debug.c is >40KB for a reason. I just rewrote this series... > Not sure there is a way around this short of auditing i2c drivers to > change any that do this. ... to do something like this, more precisely: an opt-in approach. I introduce a new flag for i2c_msg, namely I2C_M_DMA_SAFE which can opt-in DMA if we know the i2c_msg buffer is DMA safe. I finished a proof-of-concept this evening and pushed it here: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma-rfc-v4 I will test it on HW tomorrow and send it out, then. There are still some bits missing, but for getting opinions, it should be good enough. Thanks and kind regards, Wolfram
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index c89dac7fd2e7b7..7326a9d2e4eb69 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -34,6 +34,7 @@ #include <linux/irqflags.h> #include <linux/jump_label.h> #include <linux/kernel.h> +#include <linux/mm.h> #include <linux/module.h> #include <linux/mutex.h> #include <linux/of_device.h> @@ -44,6 +45,7 @@ #include <linux/pm_wakeirq.h> #include <linux/property.h> #include <linux/rwsem.h> +#include <linux/sched/task_stack.h> #include <linux/slab.h> #include "i2c-core.h" @@ -2240,6 +2242,72 @@ void i2c_put_adapter(struct i2c_adapter *adap) } EXPORT_SYMBOL(i2c_put_adapter); +/** + * i2c_check_msg_for_dma() - check if a message is suitable for DMA + * @msg: the message to be checked + * @threshold: the amount of byte from which using DMA makes sense + * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this + * ptr, if needed. The bounce buffer must be freed by the + * caller using i2c_release_dma_bounce_buf(). + * + * Return: -ERANGE if message is smaller than threshold + * -EFAULT if message buffer is not DMA capable and no bounce buffer + * was requested + * -ENOMEM if a bounce buffer could not be created + * 0 if message is suitable for DMA + * + * The return value must be checked. + * + * Note: This function should only be called from process context! It uses + * helper functions which work on the 'current' task. + */ +int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold, + u8 **ptr_for_bounce_buf) +{ + if (ptr_for_bounce_buf) + *ptr_for_bounce_buf = NULL; + + if (msg->len < threshold) + return -ERANGE; + + if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) { + pr_debug("msg buffer to 0x%04x is not DMA safe%s\n", msg->addr, + ptr_for_bounce_buf ? ", trying bounce buffer" : ""); + if (ptr_for_bounce_buf) { + if (msg->flags & I2C_M_RD) + *ptr_for_bounce_buf = kzalloc(msg->len, GFP_KERNEL); + else + *ptr_for_bounce_buf = kmemdup(msg->buf, msg->len, + GFP_KERNEL); + if (!*ptr_for_bounce_buf) + return -ENOMEM; + } else { + return -EFAULT; + } + } + + return 0; +} +EXPORT_SYMBOL_GPL(i2c_check_msg_for_dma); + +/** + * i2c_release_bounce_buf - copy data back from bounce buffer and release it + * @msg: the message to be copied back to + * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma(). + * May be NULL. + */ +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf) +{ + if (!bounce_buf) + return; + + if (msg->flags & I2C_M_RD) + memcpy(msg->buf, bounce_buf, msg->len); + + kfree(bounce_buf); +} +EXPORT_SYMBOL_GPL(i2c_release_bounce_buf); + MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); MODULE_DESCRIPTION("I2C-Bus main module"); MODULE_LICENSE("GPL"); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 00ca5b86a753f8..ac02287b6c0d8f 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -766,6 +766,11 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg) return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0); } +int __must_check i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold, + u8 **ptr_for_bounce_buf); + +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf); + int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr); /** * module_i2c_driver() - Helper macro for registering a modular I2C driver
One helper checks if DMA is suitable and optionally creates a bounce buffer, if not. The other function returns the bounce buffer and makes sure the data is properly copied back to the message. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Changes since v2: * rebased to v4.13-rc1 * helper functions are not inlined anymore but moved to i2c core * __must_check has been added to the buffer check helper * the release function has been renamed to contain 'dma' as well drivers/i2c/i2c-core-base.c | 68 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 5 ++++ 2 files changed, 73 insertions(+)