Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Martin Sperl]

> On 09.05.2015, at 00:13, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Fri, May 08, 2015 at 10:02:44PM +0200, Martin Sperl wrote:
> 
>> Based on the findings by Noralf, the following solves the issue:
> 
> Can you please post this upstream as a proper submission so I can review
> and apply it (I'm not going to look at it properly right now, it's just
> gone 11pm, but at first glance it doesn't ring any alarm bells)?
> 
>> Note that I am not sure about the locking, so I left unmap outside the
>> lock for now.
> 
> It needs to be, you can't do anything big like unmapping in a
> non-sleeping context.

Well, right now it is not in a locking context but outside, so
I wonder if it is really needed, but then for spi_finalize_current_message
it is not specified if it can also get run from interrupt context or not.

I will do something quick now, but I guess we should revisit the
SPI_MASTER_MUST_RX/TX situation as there are some side-effects
that may not be intended or performance friendly like:

* dummy_RX/TX get allocated/released even when can_dma returns false
  should only run this “mapping if necessary. I guess that it is mostly
  usefull for DMA transfers - PIO drivers typically implement this via
  a simple if when tx/rx_buf is NULL.
* dummy_tx/rx get allocated/released for every spi_message, which is
  wasting CPU
* we allocate spi_transfer->len bytes to this dummy buffer and with a
  spi-transfer of 16MB this would try to allocate another 16MB - and possibly
  fail...
  Here maybe a fixed allocated (and mapped) buffer of master->max_dma_len
  could be helpfull that would get reused multiple times in the scatter/gather
  list to minimize the memory footprint.
  Ideally there would be support for this inside dma-engine in the form of
  a flag when calling dmaengine_prep_slave_sg to indicate that data comes
  from /dev/zero or goes to /dev/null, as some silicon dma-engines
  (like bcm2835) supports such types of transfers in hardware.

Martin--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Martin Sperl]

Forwarded to linux-spi as well for documentation, was only sent to linux-rpi-kernel.
Apologies.


> On 08.05.2015, at 22:02, Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
> 
> Hi!
> 
> It seems as if there is a potential race inside spi_finalize_current_message
> in this section of drivers/spi/spi.c
> 
> void spi_finalize_current_message(struct spi_master *master)
> {
>        struct spi_message *mesg;
>        unsigned long flags;
>        int ret;
> 
>        spin_lock_irqsave(&master->queue_lock, flags);
>        mesg = master->cur_msg;
>        master->cur_msg = NULL;
> 
>        queue_kthread_work(&master->kworker, &master->pump_messages);
>        spin_unlock_irqrestore(&master->queue_lock, flags);
> 
>        spi_unmap_msg(master, mesg);
> ...
> 
> The kernel thread gets scheduled before spi_unmap_msg is getting called
> this is obviously not an issue when master->dummy_rx and master->dummy_tx
> are NULL, but if a driver sets SPI_MASTER_MUST_RX or SPI_MASTER_MUST_TX,
> then master->dummy_rx and master->dummy_tx may get freed inside the
> pump thread _before_ the memory is unmapped inside finalize.
> 
> This can result in:
> [   83.774518] page:db7ba030 count:0 mapcount:0 mapping:  (null) index:0x0
> [   83.783143] flags: 0x200(arch_1)
> [   83.791861] page dumped because: PAGE_FLAGS_CHECK_AT_PREP flag set
> [   83.803147] bad because of flags:
> [   83.808397] flags: 0x200(arch_1)
> ...
> [   83.813703] [<c012a2d4>] (__kmalloc_track_caller) from [<c01050d8>] (krealloc+0x60/0xb8)
> [   83.813734] [<c01050d8>] (krealloc) from [<c03cce08>] (__spi_pump_messages+0x70c/0x79c)
> [   83.813764] [<c03cce08>] (__spi_pump_messages) from [<c03cd0b4>] (__spi_sync+0x21c/0x22c)
> [   83.813789] [<c03cd0b4>] (__spi_sync) from [<c03cd100>] (spi_sync+0x1c/0x20)
> [   83.813871] [<c03cd100>] (spi_sync) from [<bf0341c4>] (fbtft_write_spi+0x90/0x108 [fbtft])
> 
> Surprisingly Noralf saw this only in a downstream 4.0 kernel but I have not
> experienced such a situation in 4.1-rc2 (and after being able to reproduce
> on downstream 4.0 I still have not been able to trigger such a situation in
> 4.1-rc2).
> 
> Based on the findings by Noralf, the following solves the issue:
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 57a1950..1e662a0 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1089,6 +1089,8 @@ void spi_finalize_current_message(struct spi_master *master)
> 	unsigned long flags;
> 	int ret;
> 
> +	spi_unmap_msg(master, master->cur_msg);
> +
> 	spin_lock_irqsave(&master->queue_lock, flags);
> 	mesg = master->cur_msg;
> 	master->cur_msg = NULL;
> @@ -1096,8 +1098,6 @@ void spi_finalize_current_message(struct spi_master *master)
> 	queue_kthread_work(&master->kworker, &master->pump_messages);
> 	spin_unlock_irqrestore(&master->queue_lock, flags);
> 
> -	spi_unmap_msg(master, mesg);
> -
> 	if (master->cur_msg_prepared && master->unprepare_message) {
> 		ret = master->unprepare_message(master, mesg);
> 		if (ret) {
> 
> 
> Note that I am not sure about the locking, so I left unmap outside the
> lock for now.
> 
> Not knowing the semantics of prepare_message it may be possible that move
> may also need to get applied to the master->unprepare_message() call.
> 
> 
> Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: fix race freeing dummy_tx/rx before it is unmapped

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Fix a race (with some kernel configurations) where a queued
master->pump_messages runs and frees dummy_tx/rx before
spi_unmap_msg is running (or is finished).

This results in the following messages:
  BUG: Bad page state in process
  page:db7ba030 count:0 mapcount:0 mapping:  (null) index:0x0
  flags: 0x200(arch_1)
  page dumped because: PAGE_FLAGS_CHECK_AT_PREP flag set
  ...

Reported-by: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
Suggested-by: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
Tested-by: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Note that I am not 100% sure if the spinlock is really needed to read
cur_msg, but as it was there I left it as is and just moved the
scheduling and assignments down after sg_unmap and unprepare_message.

Noralf also sugested removing the first locking and my testing shows
that I was unable to trigger any issues with locking removed for the
assignemnt of mesg but there still may be a possibilty...

Also note that if you leave cur_message = NULL assignement on top, then
there is another race were other drivers submitting spi_messages and thus
triggering spi_pump while we still are cleaning up the old message.
This is because pump_message stops if it finds cur_message to be still
asigned.

Tested with the following devices on the same bus and all active:
* 2x mcp2515
* 1x enc28j60
* 1x fb_st7735r

Communication on reporting/testing by Noralf can get reviewed at:
https://github.com/raspberrypi/linux/issues/959#issuecomment-100391599 and
https://github.com/msperl/spi-bcm2835/issues/13#issuecomment-87210385

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 50910d8..d35c1a1 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -988,9 +988,6 @@ void spi_finalize_current_message(struct spi_master *master)
 
 	spin_lock_irqsave(&master->queue_lock, flags);
 	mesg = master->cur_msg;
-	master->cur_msg = NULL;
-
-	queue_kthread_work(&master->kworker, &master->pump_messages);
 	spin_unlock_irqrestore(&master->queue_lock, flags);
 
 	spi_unmap_msg(master, mesg);
@@ -1003,9 +1000,13 @@ void spi_finalize_current_message(struct spi_master *master)
 		}
 	}
 
-	trace_spi_message_done(mesg);
-
+	spin_lock_irqsave(&master->queue_lock, flags);
+	master->cur_msg = NULL;
 	master->cur_msg_prepared = false;
+	queue_kthread_work(&master->kworker, &master->pump_messages);
+	spin_unlock_irqrestore(&master->queue_lock, flags);
+
+	trace_spi_message_done(mesg);
 
 	mesg->state = NULL;
 	if (mesg->complete)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2242 bytes --]

On Sat, May 09, 2015 at 08:40:12AM +0200, Martin Sperl wrote:
> > On 09.05.2015, at 00:13, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Fri, May 08, 2015 at 10:02:44PM +0200, Martin Sperl wrote:

> >> Note that I am not sure about the locking, so I left unmap outside the
> >> lock for now.

> > It needs to be, you can't do anything big like unmapping in a
> > non-sleeping context.

> Well, right now it is not in a locking context but outside, so
> I wonder if it is really needed, but then for spi_finalize_current_message
> it is not specified if it can also get run from interrupt context or not.

I'm having a hard time following the above, sorry.  What is "it" here?

Ideally we do want to be able to finalize in any context, though as
you've seen that's not true for all feature sets.  It's fixable if
someone has a need though.

> * dummy_RX/TX get allocated/released even when can_dma returns false
>   should only run this “mapping if necessary. I guess that it is mostly
>   usefull for DMA transfers - PIO drivers typically implement this via
>   a simple if when tx/rx_buf is NULL.

No, it's supposed to be usable for PIO too - it can keep logic simpler.

> * dummy_tx/rx get allocated/released for every spi_message, which is
>   wasting CPU

Right, ideally we want to push that logic out so that the free happens
when the controller goes idle.

> * we allocate spi_transfer->len bytes to this dummy buffer and with a
>   spi-transfer of 16MB this would try to allocate another 16MB - and possibly
>   fail...
>   Here maybe a fixed allocated (and mapped) buffer of master->max_dma_len
>   could be helpfull that would get reused multiple times in the scatter/gather
>   list to minimize the memory footprint.
>   Ideally there would be support for this inside dma-engine in the form of
>   a flag when calling dmaengine_prep_slave_sg to indicate that data comes
>   from /dev/zero or goes to /dev/null, as some silicon dma-engines
>   (like bcm2835) supports such types of transfers in hardware.

Indeed, just having a page that gets reused would be better.  I'm hoping
that someone who needs this will get around to optimizing it at some
point.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Martin Sperl]

> On 11.05.2015, at 20:00, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
>> * dummy_RX/TX get allocated/released even when can_dma returns false
>>  should only run this “mapping if necessary. I guess that it is mostly
>>  usefull for DMA transfers - PIO drivers typically implement this via
>>  a simple if when tx/rx_buf is NULL.
> 
> No, it's supposed to be usable for PIO too - it can keep logic simpler.
but only slightly - it is a simple
  (xfer->tx_buf) ? xfer->tx_buf[i] : 0
so I do not see a huge difference.

> Indeed, just having a page that gets reused would be better.  I'm hoping
> that someone who needs this will get around to optimizing it at some
> point.

I could possibly look into that, but it goes against the point you
made above about PIO.
Either we allocate the memory completely and make it work for PIO.
Or we only create a scatter/gather list of the same page and leave
tx_buf == NULL.

So unless we have a separate flag to say we only need it for DMA,
then we have to keep the the current logic with allocation or we break
other drivers.

Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

On Mon, May 11, 2015 at 09:27:35PM +0200, Martin Sperl wrote:
> > On 11.05.2015, at 20:00, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > No, it's supposed to be usable for PIO too - it can keep logic simpler.

> but only slightly - it is a simple
>   (xfer->tx_buf) ? xfer->tx_buf[i] : 0
> so I do not see a huge difference.

People can make the logic more complex than that.

> > Indeed, just having a page that gets reused would be better.  I'm hoping
> > that someone who needs this will get around to optimizing it at some
> > point.

> I could possibly look into that, but it goes against the point you
> made above about PIO.
> Either we allocate the memory completely and make it work for PIO.
> Or we only create a scatter/gather list of the same page and leave
> tx_buf == NULL.

We can tell if it's a DMA transfer - if it's a DMA transfer then using
anything except the SG list is a bit dodgy anyway since you're not
really supposed to have the CPU look at anything that's mapped for the
device (though it's generally fine if the device never actually DMAs).

> So unless we have a separate flag to say we only need it for DMA,
> then we have to keep the the current logic with allocation or we break
> other drivers.

We essentially have that; anything looking at a DMA mapped transfer had
better cope.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Martin Sperl]

On 2015-05-12 12:20, Mark Brown wrote:
> We can tell if it's a DMA transfer - if it's a DMA transfer then using
> anything except the SG list is a bit dodgy anyway since you're not
> really supposed to have the CPU look at anything that's mapped for the
> device (though it's generally fine if the device never actually DMAs).
>
>> So unless we have a separate flag to say we only need it for DMA,
>> then we have to keep the the current logic with allocation or we break
>> other drivers.
>
> We essentially have that; anything looking at a DMA mapped transfer had
> better cope.

I believe you miss my point:
a) with SPI_MASTER_MUST_ we will always allocate the buffer - we do not
    take the result of can_dma into account when calculating the size.
    This means we will allocate memory that is potentially too big for
    what we really need (and it can even fail because it is too big
    for the system to handle)
b) a driver in PIO mode can run without SPI_MASTER_MUST_XX
    but when can_dma returns true it needs a scatter-gather-list and
    in that case it needs SPI_MASTER_MUST_ to be set, but the allocation
    is not really needed - just a scatter/gather list that does the
    transfer - there is no means for the driver to state this
    requirement clearly.

That is why I was asking for an additional flag SPI_MASTER_MUST_DMAONLY
to state that requirement.

Then we can extend the if in spi_map_msg to:
     if ((master->flags & (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX)) &&
         (! (master->flags & SPI_MASTER_MUST_DMAONLY)) {

and we save on the allocation when not needed.

And inside __spi_map_msg we can just add some code to create the simple
scatter-gather mapping using a single preallocated page - so something
along the lines (assuming that either tx_buf or rx_buf must be set):
     if ((!xfer->tx_buf) && (master->flags & SPI_MASTER_MUST_TX)) {
         ret = spi_map_null(master, tx_dev,
                            &xfer->tx_sg, &xfer->rx_sg,
			   DMA_TO_DEVICE);
         if (ret != 0) {
              spi_unmap_buf(master, rx_dev,
                            &xfer->rx_sg, DMA_FROM_DEVICE);
              return ret;
         }
     }
     if ((!xfer->rx_buf) && (master->flags & SPI_MASTER_MUST_RX)) {
         ret = spi_map_null(master, rx_dev,
                            &xfer->rx_sg, &xfer->tx_sg,
			   DMA_FROM_DEVICE);
         if (ret != 0) {
              spi_unmap_buf(master, tx_dev,
                            &xfer->tx_sg, DMA_TO_DEVICE);
              return ret;
         }
     }

Similarly we could also define 2 flags: SPI_MASTER_MUST_RX_SG and
SPI_MASTER_MUST_TX_SG and be open for more different options for
drivers.

One potentially could merge the functionality of spi_map_msg and 
__spi_map_message into one to achive something similar without the
flag, but then this is a subtil change in the functionality which
may break existing drivers with different expectations about what
happens.

I see this last approach as the one with the highest risk of
breaking existing drivers.

So the extra flag seems to be to be a "safer" approach and it also
states more explicitly the needs of the driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

On Tue, May 12, 2015 at 02:07:21PM +0200, Martin Sperl wrote:
> On 2015-05-12 12:20, Mark Brown wrote:

> >We essentially have that; anything looking at a DMA mapped transfer had
> >better cope.

> I believe you miss my point:

No, I don't.

> a) with SPI_MASTER_MUST_ we will always allocate the buffer - we do not

In the current implementation, we can always change that in the future.

> And inside __spi_map_msg we can just add some code to create the simple
> scatter-gather mapping using a single preallocated page - so something
> along the lines (assuming that either tx_buf or rx_buf must be set):

Yes, to repeat what I said earlier I'm hoping someone with a need for
this will implement it.

> So the extra flag seems to be to be a "safer" approach and it also
> states more explicitly the needs of the driver.

I think it's easier to either have drivers handle scatterlists or not
rely on the flags for PIO and work harder if the code is looking
complicated.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Lee Jones]

> Forwarded to linux-spi as well for documentation, was only sent to linux-rpi-kernel.
> Apologies.

FYI: You should always Cc LKML (and LAKML if ARM related) for upstream
discussions.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On Wed, May 13, 2015 at 08:52:28AM +0100, Lee Jones wrote:
> > Forwarded to linux-spi as well for documentation, was only sent to linux-rpi-kernel.
> > Apologies.

> FYI: You should always Cc LKML (and LAKML if ARM related) for upstream
> discussions.

If there's a more specific upstream list it's OK to not copy one of the
generic lists (and adding too many of them can cause problems with CC
limits), it's just CCing downstream only lists like the RPi kernel list
that's an issue.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: race in spi_finalize_current_message starting queue_kthread_work before the message is unmapped

[Martin Sperl]

> On 12.05.2015, at 18:50, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
>> And inside __spi_map_msg we can just add some code to create the simple
>> scatter-gather mapping using a single preallocated page - so something
>> along the lines (assuming that either tx_buf or rx_buf must be set):
> 
> Yes, to repeat what I said earlier I'm hoping someone with a need for
> this will implement it.

Actually I was asking these questions to have some guidelines what is
acceptable and what is not for the implementation

So, I got a patch now that works with all my test cases (tft-framebuffer
device as well as enc28j60).

Submitting it shortly for review.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: SPI_MASTER_MUST_* with scatter-gather only option and avoiding realloc

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Rewrite of spi_map_msg and spi_map_buf so that for SPI_MASTER_MUST_*:
* short transfers are handled by a page-sized buffer instead of
  reallocating and freeing memory (if smaller than PAGE_SIZE)
* with an extra set of flags allows to ONLY create a scatter/gather list
  that reuses the same page for all the transfers
  The scatter list produced is a match of the corresponding template
  scatter list (so tx-sg is the template for the rx-sg and vice versa)

It also fixes the insufficient cleanup in case __spi_map_msg returns
an error.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |    2 +-
 drivers/spi/spi.c         |  174 ++++++++++++++++++++++++++++++++++++++++-----
 include/linux/spi/spi.h   |   10 ++-
 3 files changed, 165 insertions(+), 21 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index ac1760e..ac74456 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -463,7 +463,7 @@ void bcm2835_dma_init(struct spi_master *master, struct device *dev)
 	master->can_dma = bcm2835_spi_can_dma;
 	master->max_dma_len = 65535; /* limitation by BCM2835_SPI_DLEN */
 	/* need to do TX AND RX DMA, so we need dummy buffers */
-	master->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
+	master->flags = SPI_MASTER_MUST_RX_SG | SPI_MASTER_MUST_TX_SG;
 
 	return;
 
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d35c1a1..c85cf58 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -471,6 +471,73 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
 }
 
 #ifdef CONFIG_HAS_DMA
+static int spi_map_null(struct spi_master *master, struct device *dev,
+			struct sg_table *sgt,
+			struct sg_table *sgt_template,
+			void *buf,
+			enum dma_data_direction dir)
+{
+	struct page *vm_page;
+	unsigned long page_offset;
+	int sgs = 0;
+	int i, j, ret;
+	ssize_t len, l;
+
+	if (is_vmalloc_addr(buf)) {
+		vm_page = vmalloc_to_page(buf);
+		if (!vm_page) {
+			sg_free_table(sgt);
+			return -ENOMEM;
+		}
+		page_offset = offset_in_page(buf);
+	} else {
+		vm_page = NULL;
+		page_offset = 0;
+	}
+
+	/* count the number of sgs we will need walking the template */
+	for (i = 0; i < sgt_template->nents; i++) {
+		len = sg_dma_len(&sgt_template->sgl[i]);
+		while (len) {
+			len -= min_t(size_t, len, PAGE_SIZE);
+			sgs++;
+		}
+	}
+
+	/* now allocate it */
+	ret = sg_alloc_table(sgt, sgs, GFP_KERNEL);
+	if (ret)
+		return ret;
+
+	/* and iterate over the template to fill our own table */
+	for (i = 0, j = 0; i < sgt_template->nents; i++) {
+		len = sg_dma_len(&sgt_template->sgl[i]);
+		/* split into multiple transfers if needed */
+		while (len) {
+			l = min_t(size_t, len, PAGE_SIZE);
+			if (vm_page)
+				sg_set_page(&sgt->sgl[i], vm_page,
+					    l, page_offset);
+			else
+				sg_set_buf(&sgt->sgl[j], buf, l);
+			len -= l;
+			j++;
+		}
+	}
+
+	ret = dma_map_sg(dev, sgt->sgl, sgt->nents, dir);
+	if (!ret)
+		ret = -ENOMEM;
+	if (ret < 0) {
+		sg_free_table(sgt);
+		return ret;
+	}
+
+	sgt->nents = ret;
+
+	return 0;
+}
+
 static int spi_map_buf(struct spi_master *master, struct device *dev,
 		       struct sg_table *sgt, void *buf, size_t len,
 		       enum dma_data_direction dir)
@@ -564,6 +631,37 @@ static int __spi_map_msg(struct spi_master *master, struct spi_message *msg)
 				return ret;
 			}
 		}
+
+		/*
+		 * handle the SPI_MASTER_MUST_*_SG
+		 * note that the situation with tx_buf and rx_buf both NULL
+		 * is checked and handled inside spi_transfer_one_message
+		 */
+		if ((!xfer->tx_buf) && (xfer->rx_buf) &&
+		    (master->flags & SPI_MASTER_MUST_TX_SG)) {
+			ret = spi_map_null(master, tx_dev,
+					   &xfer->tx_sg, &xfer->rx_sg,
+					   master->page_tx,
+					   DMA_TO_DEVICE);
+			if (ret != 0) {
+				spi_unmap_buf(master, rx_dev, &xfer->rx_sg,
+					      DMA_FROM_DEVICE);
+				return ret;
+			}
+		}
+
+		if ((!xfer->rx_buf) && (xfer->tx_buf) &&
+		    (master->flags & SPI_MASTER_MUST_RX_SG)) {
+			ret = spi_map_null(master, rx_dev,
+					   &xfer->rx_sg, &xfer->tx_sg,
+					   master->page_rx,
+					   DMA_FROM_DEVICE);
+			if (ret != 0) {
+				spi_unmap_buf(master, tx_dev, &xfer->tx_sg,
+					      DMA_TO_DEVICE);
+				return ret;
+			}
+		}
 	}
 
 	master->cur_msg_mapped = true;
@@ -587,9 +685,11 @@ static int spi_unmap_msg(struct spi_master *master, struct spi_message *msg)
 		 * Restore the original value of tx_buf or rx_buf if they are
 		 * NULL.
 		 */
-		if (xfer->tx_buf == master->dummy_tx)
+		if ((xfer->tx_buf == master->dummy_tx) ||
+		    (xfer->tx_buf == master->page_tx))
 			xfer->tx_buf = NULL;
-		if (xfer->rx_buf == master->dummy_rx)
+		if ((xfer->rx_buf == master->dummy_rx) ||
+		    (xfer->rx_buf == master->page_rx))
 			xfer->rx_buf = NULL;
 
 		if (!master->can_dma(master, msg->spi, xfer))
@@ -618,8 +718,9 @@ static inline int spi_unmap_msg(struct spi_master *master,
 static int spi_map_msg(struct spi_master *master, struct spi_message *msg)
 {
 	struct spi_transfer *xfer;
-	void *tmp;
+	void *tmp_tx, *tmp_rx;
 	unsigned int max_tx, max_rx;
+	int ret;
 
 	if (master->flags & (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX)) {
 		max_tx = 0;
@@ -635,34 +736,53 @@ static int spi_map_msg(struct spi_master *master, struct spi_message *msg)
 		}
 
 		if (max_tx) {
-			tmp = krealloc(master->dummy_tx, max_tx,
-				       GFP_KERNEL | GFP_DMA);
-			if (!tmp)
-				return -ENOMEM;
-			master->dummy_tx = tmp;
-			memset(tmp, 0, max_tx);
+			if (max_tx > PAGE_SIZE) {
+				tmp_tx = krealloc(master->dummy_tx, max_tx,
+						  GFP_KERNEL | GFP_DMA);
+				if (!tmp_tx)
+					return -ENOMEM;
+				master->dummy_tx = tmp_tx;
+				memset(tmp_tx, 0, max_tx);
+			} else {
+				tmp_tx = master->page_tx;
+			}
+		} else {
+			tmp_tx = NULL;
 		}
 
-		if (max_rx) {
-			tmp = krealloc(master->dummy_rx, max_rx,
-				       GFP_KERNEL | GFP_DMA);
-			if (!tmp)
-				return -ENOMEM;
-			master->dummy_rx = tmp;
+		if (max_rx)  {
+			if (max_rx > PAGE_SIZE) {
+				tmp_rx = krealloc(master->dummy_rx, max_rx,
+						  GFP_KERNEL | GFP_DMA);
+				if (!tmp_rx)
+					return -ENOMEM;
+				master->dummy_rx = tmp_rx;
+			} else {
+				tmp_rx = master->page_rx;
+			}
+		} else {
+			tmp_tx = NULL;
 		}
 
 		if (max_tx || max_rx) {
 			list_for_each_entry(xfer, &msg->transfers,
 					    transfer_list) {
 				if (!xfer->tx_buf)
-					xfer->tx_buf = master->dummy_tx;
+					xfer->tx_buf = tmp_tx;
 				if (!xfer->rx_buf)
-					xfer->rx_buf = master->dummy_rx;
+					xfer->rx_buf = tmp_rx;
 			}
 		}
 	}
 
-	return __spi_map_msg(master, msg);
+	/* if we fail we need to undo the parial mappings
+	 * and fix up the modified rx_buf/tx_buf
+	 */
+	ret = __spi_map_msg(master, msg);
+	if (ret)
+		spi_unmap_msg(master, msg);
+
+	return ret;
 }
 
 /*
@@ -1555,6 +1675,24 @@ int spi_register_master(struct spi_master *master)
 	if (!master->max_dma_len)
 		master->max_dma_len = INT_MAX;
 
+	/* we need to set max_dma_len to PAGESIZE for MUST_XX_SG */
+	if (master->flags & (SPI_MASTER_MUST_RX_SG | SPI_MASTER_MUST_TX_SG))
+		master->max_dma_len = min_t(size_t,
+					    master->max_dma_len, PAGE_SIZE);
+	/* and allocate some buffers for dma */
+	if (master->flags & (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_RX_SG)) {
+		master->page_rx = devm_kmalloc(&master->dev,
+					       PAGE_SIZE, GFP_DMA);
+		if (!master->page_rx)
+			return -ENOMEM;
+	}
+	if (master->flags & (SPI_MASTER_MUST_TX | SPI_MASTER_MUST_TX_SG)) {
+		master->page_tx = devm_kzalloc(&master->dev,
+					       PAGE_SIZE, GFP_DMA);
+		if (!master->page_tx)
+			return -ENOMEM;
+	}
+
 	/* register the device, then userspace will see it.
 	 * registration fails if the bus ID is in use.
 	 */
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index d673072..1f440ff 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -353,8 +353,10 @@ struct spi_master {
 #define SPI_MASTER_HALF_DUPLEX	BIT(0)		/* can't do full duplex */
 #define SPI_MASTER_NO_RX	BIT(1)		/* can't do buffer read */
 #define SPI_MASTER_NO_TX	BIT(2)		/* can't do buffer write */
-#define SPI_MASTER_MUST_RX      BIT(3)		/* requires rx */
-#define SPI_MASTER_MUST_TX      BIT(4)		/* requires tx */
+#define SPI_MASTER_MUST_RX      BIT(3)		/* requires rx_buf allocated */
+#define SPI_MASTER_MUST_TX      BIT(4)		/* requires tx_buf allocated */
+#define SPI_MASTER_MUST_RX_SG   BIT(5)		/* requires rx sg list */
+#define SPI_MASTER_MUST_TX_SG   BIT(6)		/* requires tx sg list */
 
 	/* lock and mutex for SPI bus locking */
 	spinlock_t		bus_lock_spinlock;
@@ -459,6 +461,10 @@ struct spi_master {
 	/* dummy data for full duplex devices */
 	void			*dummy_rx;
 	void			*dummy_tx;
+
+	/* pages for dma-transfers */
+	void			*page_rx;
+	void			*page_tx;
 };
 
 static inline void *spi_master_get_devdata(struct spi_master *master)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: SPI_MASTER_MUST_* with scatter-gather only option and avoiding realloc

[Martin Sperl]

> On 14.05.2015, at 11:58, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> 
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> Rewrite of spi_map_msg and spi_map_buf so that for SPI_MASTER_MUST_*:
> * short transfers are handled by a page-sized buffer instead of
>  reallocating and freeing memory (if smaller than PAGE_SIZE)
> * with an extra set of flags allows to ONLY create a scatter/gather list
>  that reuses the same page for all the transfers
>  The scatter list produced is a match of the corresponding template
>  scatter list (so tx-sg is the template for the rx-sg and vice versa)
> 
> It also fixes the insufficient cleanup in case __spi_map_msg returns
> an error.
> 
> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> ---

Forgot to mention:

Tested on a raspberry pi with the following devices:
* 2x mcp2515
* 1x mmc_spi (with dma)
* 1x enc28j60 (with dma)
* 1x fb_st7735r (with dma)

The fbtft-device played all of the BigBuckBunny movie without any issues.
The spi-ethernet card was able to transfer 64MB without hickups
the fsck of the attached sd card worked flawlessly.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: SPI_MASTER_MUST_* with scatter-gather only option and avoiding realloc

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4335 bytes --]

On Thu, May 14, 2015 at 09:58:44AM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> Rewrite of spi_map_msg and spi_map_buf so that for SPI_MASTER_MUST_*:

This isn't just rewriting the internals, this is adding a completely new
API with separate code, fixing a bug, optimising the existing API and
adding a user of the new API.  I'd have expected all of that to be
called out in the changelog and for it to be split out into several
patches - especially the bug fix since that should go to Linus' tree and
stable.

Like I say I'm not entirely convinced we need the extra flags over just
using can_dma().

> It also fixes the insufficient cleanup in case __spi_map_msg returns
> an error.

This should be a separate patch.

> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index ac1760e..ac74456 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -463,7 +463,7 @@ void bcm2835_dma_init(struct spi_master *master, struct device *dev)
>  	master->can_dma = bcm2835_spi_can_dma;
>  	master->max_dma_len = 65535; /* limitation by BCM2835_SPI_DLEN */
>  	/* need to do TX AND RX DMA, so we need dummy buffers */
> -	master->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
> +	master->flags = SPI_MASTER_MUST_RX_SG | SPI_MASTER_MUST_TX_SG;
>  
>  	return;
>  

This is updating a specific driver to use the new API you're adding,
this should be in a separate patch.

>  #ifdef CONFIG_HAS_DMA
> +static int spi_map_null(struct spi_master *master, struct device *dev,
> +			struct sg_table *sgt,
> +			struct sg_table *sgt_template,
> +			void *buf,
> +			enum dma_data_direction dir)
> +{

It's not obvious to me what this is supposed to do and it looks awfully
like it's duplicating the existing DMA mapping code (but if that's the
case the existing code should be using this not duplicating it so I
guess not).  I think it's supposed to be for creating a scatterlist that
repeatedly reused the same dummy page but there's this template thing,
it's getting a buffer passed in and...

> +	if (is_vmalloc_addr(buf)) {
> +		vm_page = vmalloc_to_page(buf);
> +		if (!vm_page) {
> +			sg_free_table(sgt);
> +			return -ENOMEM;
> +		}
> +		page_offset = offset_in_page(buf);
> +	} else {
> +		vm_page = NULL;
> +		page_offset = 0;
> +	}

...this is really weird for that case.  

I'd have expected to be allocating a scatterlist that transfers a
specified length to/from a normal page sized kmalloc() buffer in which
case we don't need to worry about vmalloc() or this template stuff.

> +		/*
> +		 * handle the SPI_MASTER_MUST_*_SG
> +		 * note that the situation with tx_buf and rx_buf both NULL
> +		 * is checked and handled inside spi_transfer_one_message
> +		 */
> +		if ((!xfer->tx_buf) && (xfer->rx_buf) &&
> +		    (master->flags & SPI_MASTER_MUST_TX_SG)) {
> +			ret = spi_map_null(master, tx_dev,
> +					   &xfer->tx_sg, &xfer->rx_sg,
> +					   master->page_tx,
> +					   DMA_TO_DEVICE);
> +			if (ret != 0) {
> +				spi_unmap_buf(master, rx_dev, &xfer->rx_sg,
> +					      DMA_FROM_DEVICE);
> +				return ret;
> +			}
> +		}

Why does the transmit handling use a scatterlist allocated for receive
(and vice versa)?  This goes back to the lack of clarity about what
spi_map_null() is doing.

>  		if (max_tx) {
> -			tmp = krealloc(master->dummy_tx, max_tx,
> -				       GFP_KERNEL | GFP_DMA);
> -			if (!tmp)
> -				return -ENOMEM;
> -			master->dummy_tx = tmp;
> -			memset(tmp, 0, max_tx);
> +			if (max_tx > PAGE_SIZE) {
> +				tmp_tx = krealloc(master->dummy_tx, max_tx,
> +						  GFP_KERNEL | GFP_DMA);
> +				if (!tmp_tx)
> +					return -ENOMEM;
> +				master->dummy_tx = tmp_tx;
> +				memset(tmp_tx, 0, max_tx);
> +			} else {
> +				tmp_tx = master->page_tx;
> +			}

This idea is fine but should be a separate patch.

I'd expect it should be about as cheap to change the realloc to be the
max of max_tx and PAGE_SIZE (the code was written on the basis that we'd
basically get that behaviour from the allocator anyway most of the
time though IIRC it will work in chunks smaller than a page) but this
does save us the memset() on the transmit path which is handy.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: SPI_MASTER_MUST_* with scatter-gather only option and avoiding realloc

[Martin Sperl]

> On 19.05.2015, at 14:46, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> Like I say I'm not entirely convinced we need the extra flags over just
> using can_dma().
If you can tell me that:
	(master->flags & (SPI_MASTER_MUST_TX | SPI_MASTER_MUST_RX)) &&
	master->can_dma && master->can_dma(...) 
means that we do NOT need real memory allocated in the first place but
we only expect a scatter list to be created 
and memory allocated in all other cases then that be it.

I am just not convinced that this is necessarily true for all
existing (or future) drivers.

During my discovery questions you mentioned that for some drivers
it may not be as simple as
  tx = (xfer->tx_buf) ? xfer->tx_buf[i] : 0;

So this “extension” of the API is to avoid the potential risk of
breaking other drivers by making the above assumption (with
additional calls to can_dma, which then we should start to cache)

> 
>> It also fixes the insufficient cleanup in case __spi_map_msg returns
>> an error.
> 
> This should be a separate patch.

The problem is that this came up while developing the patch, so
I left it in the patch, assuming there would be some feedback
that requires another version of the patch...

> This is updating a specific driver to use the new API you're adding,
> this should be in a separate patch.

At one occasion I got scolded for separating things out into different
patches (in that case api from implemetation), now the other way around,
but we can do that...

> 
>> #ifdef CONFIG_HAS_DMA
>> +static int spi_map_null(struct spi_master *master, struct device *dev,
>> +			struct sg_table *sgt,
>> +			struct sg_table *sgt_template,
>> +			void *buf,
>> +			enum dma_data_direction dir)
>> +{
> 
> It's not obvious to me what this is supposed to do and it looks awfully
> like it's duplicating the existing DMA mapping code (but if that's the
> case the existing code should be using this not duplicating it so I
> guess not).  I think it's supposed to be for creating a scatterlist that
> repeatedly reused the same dummy page but there's this template thing,
> it's getting a buffer passed in and…

OK, so the idea is that this situation ONLY happens when we have either
tx_buf or rx_buf for which we need to map the other transfer from a
“dummy/null” page.

So maybe instead of spi_map_null it should get called spi_map_dummy.

The template is there to create an (almost) identical scatter lists for
both rx and tx (modulo PAGE_SIZE). So if we have a tx-scatter list of:
4, 8, 4103 bytes, and we need to do a dummy transfer for rx, then the
(basic) identical pattern of 4, 8, 4096, 7 would get created.

This kind avoids some restrictions that I have seen with the spi-bcm2835
hardware.

As for replication - the code is similar in some aspects,
but still serves a different purpose (especially with the templating).

Adding all that into one would mean actually merging tx and rx cases
into one piece of code, and I did not want to go that far.
Keep what is working.

Also if we would do that then we should also add additional features
like creating a list of scatter lists for some cases, when:
* an individual dma transfer to the HW can only be < 65536 bytes in
  length (the spi HW got a counter)
* or when individual entries in the scatter list are not a multiple
  of 4
we would need to add multiple sg lists and handle each one separately.
(again examples from spi-bcm2835)

This would be a much bigger change.

> 
>> +	if (is_vmalloc_addr(buf)) {
>> +		vm_page = vmalloc_to_page(buf);
>> +		if (!vm_page) {
>> +			sg_free_table(sgt);
>> +			return -ENOMEM;
>> +		}
>> +		page_offset = offset_in_page(buf);
>> +	} else {
>> +		vm_page = NULL;
>> +		page_offset = 0;
>> +	}
> 
> ...this is really weird for that case.  
> 
> I'd have expected to be allocating a scatterlist that transfers a
> specified length to/from a normal page sized kmalloc() buffer in which
> case we don't need to worry about vmalloc() or this template stuff.

vm_page actually serves as a flag what call should get used a little
further down - sg_set_page (in case of not null) or sg_set_buf.

This is there as I am not 100% sure that the allocation used as
buffer is ALWAYS of the same identical type on _all_ platforms
in all .config variants (and I want to avoid breaking things...

If you can tell me that it is always of one type (even when used
early during kernel initialization), then we can remove that.

> Why does the transmit handling use a scatterlist allocated for receive
> (and vice versa)?  This goes back to the lack of clarity about what
> spi_map_null() is doing.

See above about “templating"

> 
>> 		if (max_tx) {
>> -			tmp = krealloc(master->dummy_tx, max_tx,
>> -				       GFP_KERNEL | GFP_DMA);
>> -			if (!tmp)
>> -				return -ENOMEM;
>> -			master->dummy_tx = tmp;
>> -			memset(tmp, 0, max_tx);
>> +			if (max_tx > PAGE_SIZE) {
>> +				tmp_tx = krealloc(master->dummy_tx, max_tx,
>> +						  GFP_KERNEL | GFP_DMA);
>> +				if (!tmp_tx)
>> +					return -ENOMEM;
>> +				master->dummy_tx = tmp_tx;
>> +				memset(tmp_tx, 0, max_tx);
>> +			} else {
>> +				tmp_tx = master->page_tx;
>> +			}
> 
> This idea is fine but should be a separate patch.
> 
> I'd expect it should be about as cheap to change the realloc to be the
> max of max_tx and PAGE_SIZE (the code was written on the basis that we'd
> basically get that behaviour from the allocator anyway most of the
> time though IIRC it will work in chunks smaller than a page) but this
> does save us the memset() on the transmit path which is handy.

Well - this comes mostly from the fact that we now have a PAGE that
is cleaned already (and we need it for mapping anyway), so 
just make use of it as long as the size is small enough, which
probably happens in most cases.

Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: SPI_MASTER_MUST_* with scatter-gather only option and avoiding realloc

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2362 bytes --]

On Tue, May 19, 2015 at 04:17:33PM +0200, Martin Sperl wrote:
> > On 19.05.2015, at 14:46, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

Quite a few process issues here which are rather annoying so I'm just
going to ignore the technical content for now.

> > Like I say I'm not entirely convinced we need the extra flags over just
> > using can_dma().

> If you can tell me that:
> 	(master->flags & (SPI_MASTER_MUST_TX | SPI_MASTER_MUST_RX)) &&
> 	master->can_dma && master->can_dma(...) 
> means that we do NOT need real memory allocated in the first place but
> we only expect a scatter list to be created 
> and memory allocated in all other cases then that be it.

I replied to your last mail on this in the other thread.

> >> It also fixes the insufficient cleanup in case __spi_map_msg returns
> >> an error.

> > This should be a separate patch.

> The problem is that this came up while developing the patch, so
> I left it in the patch, assuming there would be some feedback
> that requires another version of the patch...

There is no barrier to creating a separate patch for unrelated
improvments you happen to notice while working on an issue, it is
something that is well supported by the tooling.  Not doing this
guarantees that a resubmission will be required, means that reviewers
have to review the same code multiple times (which takes their time) and
means that it takes longer for other people to get the benefit of the
change.

> > This is updating a specific driver to use the new API you're adding,
> > this should be in a separate patch.

> At one occasion I got scolded for separating things out into different
> patches (in that case api from implemetation), now the other way around,
> but we can do that...

One patch per change as covered in SubmittingPatches.  Prototypes for a
function with no implementation are not a separate change since they
can't be used without the implementation also being added.  Changing a
driver is a separate change to adding an API since people can use the
API change even if they don't use that driver.

Not doing this causes extra work for people, it makes review harder,
causes resubmissions, and if the randomly mixed changes do end up
getting applied somehow then it makes things like bisection and
backporting harder.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: SPI_MASTER_MUST_* with scatter-gather only option and avoiding realloc

[Martin Sperl]

> On 22.05.2015, at 13:34, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> Quite a few process issues here which are rather annoying so I'm just
> going to ignore the technical content for now.
> 
Accepting those - thanks for the clarifications.

> 
>> If you can tell me that:
>> 	(master->flags & (SPI_MASTER_MUST_TX | SPI_MASTER_MUST_RX)) &&
>> 	master->can_dma && master->can_dma(...) 
>> means that we do NOT need real memory allocated in the first place but
>> we only expect a scatter list to be created 
>> and memory allocated in all other cases then that be it.
> 
> I replied to your last mail on this in the other thread.
Well - I will take that as a yes.

With the logic based only on can_dma we still would be allocating memory
for those cases where can_dma returns false - even if the driver only
requires the scatter/gather list but would not require memory for those
rx/tx_buf == NULL cases (which is the case for the spi-bcm2835).

The extra flags solve that requirement cleanly.

An alternative would be that we provide a spi_map_dummy function
that the driver could call explicitly if it just needs the scatter_gather
list for null-buffers and leave the master->flags = 0.

One other reasons was that with this logic we need to run can_dma several
times in the process at different stages which is a waste of CPU-resources
(especially as the compiler can not inline those function pointer calls).

To reduce that overhead, would it be acceptable if we add a field to
spi_transfer (and possibly to spi_message) - say bool can_dma -  that is
filled during the loop over all the transfers in __spi_validate?

Essentially adding something like this to __spi_validate:
	message->can_dma = false;
	list_for_each_entry(xfer, &message->transfers, transfer_list) {
		/* calculate can_dma once and cache it */
		xfer->can_dma = master->can_dma ? master->can_dma(xfer) : false;
		/* and set it also in the message itself */
		message->can_dma |= xfer->can_dma
	}


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: add missing cleanup in spi_map_msg on error

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

applies against for-next

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d35c1a1..a73d9e4 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -620,6 +620,7 @@ static int spi_map_msg(struct spi_master *master, struct spi_message *msg)
 	struct spi_transfer *xfer;
 	void *tmp;
 	unsigned int max_tx, max_rx;
+	int ret;
 
 	if (master->flags & (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX)) {
 		max_tx = 0;
@@ -662,7 +663,11 @@ static int spi_map_msg(struct spi_master *master, struct spi_message *msg)
 		}
 	}
 
-	return __spi_map_msg(master, msg);
+	ret = __spi_map_msg(master, msg);
+	if (ret)
+		spi_unmap_msg(master, msg);
+
+	return ret;
 }
 
 /*
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2] spi: add missing cleanup in spi_map_msg on error

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Changelog:
[V1 -> V2]: for the cleanup to work propperly cur_msg_mapped needs to be
            set, as spi_unmap_msg checks for this condition and will not
            clean up otherwise (which happened in V1)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d35c1a1..647a8bb 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -552,6 +552,7 @@ static int __spi_map_msg(struct spi_master *master, struct spi_message *msg)
 					  DMA_TO_DEVICE);
 			if (ret != 0)
 				return ret;
+			master->cur_msg_mapped = true;
 		}
 
 		if (xfer->rx_buf != NULL) {
@@ -563,11 +564,10 @@ static int __spi_map_msg(struct spi_master *master, struct spi_message *msg)
 					      DMA_TO_DEVICE);
 				return ret;
 			}
+			master->cur_msg_mapped = true;
 		}
 	}
 
-	master->cur_msg_mapped = true;
-
 	return 0;
 }
 
@@ -620,6 +620,7 @@ static int spi_map_msg(struct spi_master *master, struct spi_message *msg)
 	struct spi_transfer *xfer;
 	void *tmp;
 	unsigned int max_tx, max_rx;
+	int ret;
 
 	if (master->flags & (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX)) {
 		max_tx = 0;
@@ -662,7 +663,11 @@ static int spi_map_msg(struct spi_master *master, struct spi_message *msg)
 		}
 	}
 
-	return __spi_map_msg(master, msg);
+	ret = __spi_map_msg(master, msg);
+	if (ret)
+		spi_unmap_msg(master, msg);
+
+	return ret;
 }
 
 /*
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] dma map single page multiple times in sg_list

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

This patch series implements mapping a single page multiple times
in the sg_list when rx/tx_buf == NULL.

The 1st patch makes this apply to all spi_transfers for which
can_dma returns true, but this means it applies to all spi-masters
with DMA support.

To avoid this there is an api extension in the 3rd patch that allows
drivers to state the requirement in spi_master.flags.

spi-bcm2835 is converted to the new api in patches 2 and 4.

Patch 3 and 4 are optional and reduce impact on drivers that do
not need this feature by extending the API.

All patches apply against for-next, but note that there will be a conflict
if the patch "spi: add missing cleanup in spi_map_msg on error" is
applied already.

Martin Sperl (4):
  spi: dma map a single page multiple times in sg_list for rx/tx_buf ==
    NULL
  spi: bcm2835: no longer requires SPI_MASTER_MUST_RX/TX
  spi: add SPI_MASTER_MUST_*_SG to avoid unnecessary mapping of null
    pages in sg_lists for masters that do not need it
  spi: bcm2835: set SPI_MASTER_MUST_RX_SG/TX_SG flags

 drivers/spi/spi-bcm2835.c |    4 ++--
 drivers/spi/spi.c         |   40 +++++++++++++++++++++++++++++++++-------
 include/linux/spi/spi.h   |    6 ++++++
 3 files changed, 41 insertions(+), 9 deletions(-)

-- 
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] spi: dma map a single page multiple times in sg_list for rx/tx_buf == NULL

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

This does not require SPI_MASTER_MUST_RX/TX to be set any longer
in a spi_master and by this avoids unnecessary memory allocations.

This impacts all spi_master with can_dma support when tx/rx_buf == NULL.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi.c       |   70 ++++++++++++++++++++++++++++++++---------------
 include/linux/spi/spi.h |    4 +++
 2 files changed, 52 insertions(+), 22 deletions(-)

Applies against for-next, but note that there will be a conflict
if the patch "spi: add missing cleanup in spi_map_msg on error" is
applied already.

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d35c1a1..7e1a12c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -471,12 +471,28 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
 }
 
 #ifdef CONFIG_HAS_DMA
+static void *__spi_map_alloc_page(struct spi_master *master,
+				  enum dma_data_direction dir)
+{
+	void **buf = (dir == DMA_TO_DEVICE) ?
+		&master->dummy_page_tx : &master->dummy_page_rx;
+
+	if (!*buf) {
+		*buf = devm_kzalloc(&master->dev,
+				    PAGE_SIZE,
+				    GFP_ATOMIC);
+	}
+
+	return *buf;
+}
+
 static int spi_map_buf(struct spi_master *master, struct device *dev,
 		       struct sg_table *sgt, void *buf, size_t len,
 		       enum dma_data_direction dir)
 {
 	const bool vmalloced_buf = is_vmalloc_addr(buf);
-	const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len;
+	const int desc_len = (vmalloced_buf || (!buf)) ?
+		PAGE_SIZE : master->max_dma_len;
 	const int sgs = DIV_ROUND_UP(len, desc_len);
 	struct page *vm_page;
 	void *sg_buf;
@@ -490,7 +506,7 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 	for (i = 0; i < sgs; i++) {
 		min = min_t(size_t, len, desc_len);
 
-		if (vmalloced_buf) {
+		if (buf && vmalloced_buf) {
 			vm_page = vmalloc_to_page(buf);
 			if (!vm_page) {
 				sg_free_table(sgt);
@@ -499,12 +515,20 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 			sg_set_page(&sgt->sgl[i], vm_page,
 				    min, offset_in_page(buf));
 		} else {
-			sg_buf = buf;
+			if (!buf) {
+				sg_buf = __spi_map_alloc_page(master, dir);
+				if (!sg_buf) {
+					sg_free_table(sgt);
+					return -ENOMEM;
+				}
+			} else {
+				sg_buf = buf;
+			}
 			sg_set_buf(&sgt->sgl[i], sg_buf, min);
 		}
 
-
-		buf += min;
+		if (buf)
+			buf += min;
 		len -= min;
 	}
 
@@ -546,23 +570,25 @@ static int __spi_map_msg(struct spi_master *master, struct spi_message *msg)
 		if (!master->can_dma(master, msg->spi, xfer))
 			continue;
 
-		if (xfer->tx_buf != NULL) {
-			ret = spi_map_buf(master, tx_dev, &xfer->tx_sg,
-					  (void *)xfer->tx_buf, xfer->len,
-					  DMA_TO_DEVICE);
-			if (ret != 0)
-				return ret;
-		}
-
-		if (xfer->rx_buf != NULL) {
-			ret = spi_map_buf(master, rx_dev, &xfer->rx_sg,
-					  xfer->rx_buf, xfer->len,
-					  DMA_FROM_DEVICE);
-			if (ret != 0) {
-				spi_unmap_buf(master, tx_dev, &xfer->tx_sg,
-					      DMA_TO_DEVICE);
-				return ret;
-			}
+		/* potentially add a flag to spi_master
+		 * (SPI_MASTER_MUST_TX_SG) to avoid unnecessary mapping
+		 */
+		ret = spi_map_buf(master, tx_dev, &xfer->tx_sg,
+				  (void *)xfer->tx_buf, xfer->len,
+				  DMA_TO_DEVICE);
+		if (ret != 0)
+			return ret;
+
+		/* potentially add a flag to spi_master
+		 * (SPI_MASTER_MUST_RX_SG) to avoid unnecessary mapping
+		 */
+		ret = spi_map_buf(master, rx_dev, &xfer->rx_sg,
+				  xfer->rx_buf, xfer->len,
+				  DMA_FROM_DEVICE);
+		if (ret != 0) {
+			spi_unmap_buf(master, tx_dev, &xfer->tx_sg,
+				      DMA_TO_DEVICE);
+			return ret;
 		}
 	}
 
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index d673072..9ffa506 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -300,6 +300,8 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @dma_rx: DMA receive channel
  * @dummy_rx: dummy receive buffer for full-duplex devices
  * @dummy_tx: dummy transmit buffer for full-duplex devices
+ * @dummy_page_rx: dummy page for full-duplex devices
+ * @dummy_page_tx: dummy page for full-duplex devices
  *
  * Each SPI master controller can communicate with one or more @spi_device
  * children.  These make a small bus, sharing MOSI, MISO and SCK signals
@@ -459,6 +461,8 @@ struct spi_master {
 	/* dummy data for full duplex devices */
 	void			*dummy_rx;
 	void			*dummy_tx;
+	void			*dummy_page_tx;
+	void			*dummy_page_rx;
 };
 
 static inline void *spi_master_get_devdata(struct spi_master *master)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] spi: bcm2835: no longer requires SPI_MASTER_MUST_RX/TX

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 59705ab..bfcc436 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -462,8 +462,6 @@ static void bcm2835_dma_init(struct spi_master *master, struct device *dev)
 	/* all went well, so set can_dma */
 	master->can_dma = bcm2835_spi_can_dma;
 	master->max_dma_len = 65535; /* limitation by BCM2835_SPI_DLEN */
-	/* need to do TX AND RX DMA, so we need dummy buffers */
-	master->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
 
 	return;
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] spi: add flags SPI_MASTER_MUST_*_SG to api

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

This avoid unnecessary mapping of null pages in sg_lists for masters
that do not need a sg_list when rx_buf/tx_buff == NULL avoid.

Only spi-masters that require a mapped scatter-gather map need to set
this, but they would not need to set SPI_MASTER_MUST_RX/TX.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi.c       |   38 +++++++++++++++++++-------------------
 include/linux/spi/spi.h |    2 ++
 2 files changed, 21 insertions(+), 19 deletions(-)

Note that this is an optional patch to avoid the overhead
for spi_masters that do not need the dummy-page functionality.

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 7e1a12c..9205928 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -570,25 +570,25 @@ static int __spi_map_msg(struct spi_master *master, struct spi_message *msg)
 		if (!master->can_dma(master, msg->spi, xfer))
 			continue;
 
-		/* potentially add a flag to spi_master
-		 * (SPI_MASTER_MUST_TX_SG) to avoid unnecessary mapping
-		 */
-		ret = spi_map_buf(master, tx_dev, &xfer->tx_sg,
-				  (void *)xfer->tx_buf, xfer->len,
-				  DMA_TO_DEVICE);
-		if (ret != 0)
-			return ret;
-
-		/* potentially add a flag to spi_master
-		 * (SPI_MASTER_MUST_RX_SG) to avoid unnecessary mapping
-		 */
-		ret = spi_map_buf(master, rx_dev, &xfer->rx_sg,
-				  xfer->rx_buf, xfer->len,
-				  DMA_FROM_DEVICE);
-		if (ret != 0) {
-			spi_unmap_buf(master, tx_dev, &xfer->tx_sg,
-				      DMA_TO_DEVICE);
-			return ret;
+		if (xfer->tx_buf ||
+		    (master->flags & SPI_MASTER_MUST_TX_SG)) {
+			ret = spi_map_buf(master, tx_dev, &xfer->tx_sg,
+					  (void *)xfer->tx_buf, xfer->len,
+					  DMA_TO_DEVICE);
+			if (ret != 0)
+				return ret;
+		}
+
+		if (xfer->rx_buf ||
+		    (master->flags & SPI_MASTER_MUST_RX_SG)) {
+			ret = spi_map_buf(master, rx_dev, &xfer->rx_sg,
+					  xfer->rx_buf, xfer->len,
+					  DMA_FROM_DEVICE);
+			if (ret != 0) {
+				spi_unmap_buf(master, tx_dev, &xfer->tx_sg,
+					      DMA_TO_DEVICE);
+				return ret;
+			}
 		}
 	}
 
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 9ffa506..8b70419 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -357,6 +357,8 @@ struct spi_master {
 #define SPI_MASTER_NO_TX	BIT(2)		/* can't do buffer write */
 #define SPI_MASTER_MUST_RX      BIT(3)		/* requires rx */
 #define SPI_MASTER_MUST_TX      BIT(4)		/* requires tx */
+#define SPI_MASTER_MUST_RX_SG   BIT(5)		/* requires rx sg_list */
+#define SPI_MASTER_MUST_TX_SG   BIT(6)		/* requires tx sg_list */
 
 	/* lock and mutex for SPI bus locking */
 	spinlock_t		bus_lock_spinlock;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] spi: bcm2835: set SPI_MASTER_MUST_RX_SG/TX_SG flags

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index bfcc436..e98db57 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -462,6 +462,8 @@ static void bcm2835_dma_init(struct spi_master *master, struct device *dev)
 	/* all went well, so set can_dma */
 	master->can_dma = bcm2835_spi_can_dma;
 	master->max_dma_len = 65535; /* limitation by BCM2835_SPI_DLEN */
+	/* need to do TX AND RX DMA, so we need scatter-gather lists */
+	master->flags = SPI_MASTER_MUST_RX_SG | SPI_MASTER_MUST_TX_SG;
 
 	return;
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] spi: add missing cleanup in spi_map_msg on error

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On Mon, May 25, 2015 at 10:10:29AM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Please don't bury patches in reply to existing threads, and especially
don't send streams of multiple new patches in reply to each other at
once.  This makes it harder to spot new patches and confusing what's the
current code intended for review, you end up with a jumbled mess of
possibly related things some of which superceed each other.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]