From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752084AbdCAOax (ORCPT ); Wed, 1 Mar 2017 09:30:53 -0500 Received: from smtpout.microchip.com ([198.175.253.82]:35050 "EHLO email.microchip.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751431AbdCAOar (ORCPT ); Wed, 1 Mar 2017 09:30:47 -0500 Subject: Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support To: Boris Brezillon References: <20170227120839.16545-1-vigneshr@ti.com> <20170227120839.16545-3-vigneshr@ti.com> <8f999a27-c3ce-2650-452c-b21c3e44989d@ti.com> <8a2c9b3b-dd5f-fca7-fa5c-690e5bed949f@atmel.com> <20170301152855.0cfcdf0f@bbrezillon> CC: Vignesh R , Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , , , , Frode Isaksen , , Mark Brown From: Cyrille Pitchen Message-ID: Date: Wed, 1 Mar 2017 15:30:34 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170301152855.0cfcdf0f@bbrezillon> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: =?utf-8?B?SDRzSUFBQUFBQUFBQytOZ0ZtcGdrK0pJclNoSkxjcEx6RkZpNDJMaDR2Smgw?= =?utf-8?B?Ulc4dlMzQ1lOWDgrYmRZTEE2OFdNaGlNZlhoRXphTEl4ZldNbHRNWERtWjJl?= =?utf-8?B?THBwWCtzRnBkM3pXR3oyTjIwak4xaTlwSitGb3ZHanpmWkxZN3V1Y2RzTVhu?= =?utf-8?B?bkcwYUwvMmMvc0R2d2VMeS8wY3J1OFdUVFJVYVBuYlB1c250c1hxSGxzV2xW?= =?utf-8?B?SjV2SDVpWDFIamZuRlhvY3Y3R2R5ZVB6SnJrQXppald6THlrL0lvRTFveVhM?= =?utf-8?B?VC9ZQ3o2cVZxeDl0NHk5Z2ZHK1hCY2pGNGVRd0hKR2liV3IyaGk3R0RrNWhB?= =?utf-8?B?VWlKZjY5ZnN3RVlvc0lXRWgwUER6S0JsSDBoMGxpenQ0L2pDQU9zMEFMczhT?= =?utf-8?B?QmQyZFlRYXJZQkF3bDNqNDRDbWJ6Q3RoSVRMOXdETXhtRVZDUjZObTBnQm5F?= =?utf-8?B?RmhXSWtKai9kQlVUUkkyZ3hNbVpUMWhBYkU2ZzNvTi9UckYzTVhJQURkV1VX?= =?utf-8?B?TDlMSHlUTUxDQXYwYngxTmxpcmtJQ2F4TUtXRldDMmhFQ2d4UHBmUDVrZ2JD?= =?utf-8?B?ZUo1K3R1c2tIWWRoS0hwMTlraDdBZEpMb1cvR2FHcVhuNitDS1VyUzJ4L2RV?= =?utf-8?B?K1ZnaGJSMkxid1g0V0NOdFdZcytNaVZBejNTVWVQRm9PWmZ0S3pIcllBRlVU?= =?utf-8?B?SmRHMlpEUGJCRWJKV1VnK21JVnc5U3drVnk5Z1pGN0ZLTzNzNGFjYkhLYnJH?= =?utf-8?B?dUhzWVdDdWw1dWNVYUNibTVpWnA1ZWNuN3VKRVpJV2NuWXdybGtjY1loUmtv?= =?utf-8?B?TkpTWlIzOTZwdEVVSjhTZmtwbFJtSnhSbnhSYVU1cWNXSEdHVTRPSlFrZURm?= =?utf-8?B?Y0JNb0pGcVdtcDFha1plWUFFeFJNbW9tRDh4Q2pCQWVQa2dqdkpaQWEzdUtD?= =?utf-8?B?eE56aXpIU0kvQ2xHU1NseDNrODNnQklDSUltTTBqeTQza3VNb2xMQ3ZDa1hn?= =?utf-8?B?WEk4QmFsRnVaa2xFUEZiakdJY0Q1azRIak1Kc2VUbDU2VktBWjNLQUFRR2pL?= =?utf-8?B?OFl4VGtZbFlSNWhXK0J0R1RtbGNDdGVRVjBBUlBRQlM5VXRvSmNVSktJa0pK?= =?utf-8?B?cVlIUy82aE84NmQvU0Iwc3FYdWtjTjUrWEwzdFZuT0dTeGozdDhLbThqWG95?= =?utf-8?B?QzJZWTcwa3NsMDdUZWhieU1YNzZIWmI4WWpYemlMQnB0d3cvRzkvSjJucXRi?= =?utf-8?B?OFhjc05mVFJYSWt2eXZ2cXJhU0xkek1ZT3YrOXUwbjlWZkxiN09zdFh4UnFL?= =?utf-8?B?cVl2WmpWYjExUFE1WHZ5UmVQN3pHVTd1bGE1L0p6eWxkZUhaV3N4YTlYbEVx?= =?utf-8?B?SGlTcXhGR2NrR21veEZ4VW5BZ0FLVEU3V2t3TUFBQT09?= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Le 01/03/2017 à 15:28, Boris Brezillon a écrit : > On Wed, 1 Mar 2017 15:21:24 +0100 > Cyrille Pitchen wrote: > >> + Mark >> >> Le 01/03/2017 à 12:46, Vignesh R a écrit : >>> >>> >>> On Wednesday 01 March 2017 04:13 PM, Cyrille Pitchen wrote: >>>> Le 01/03/2017 à 05:54, Vignesh R a écrit : >>>>> >>>>> >>>>> On Wednesday 01 March 2017 03:11 AM, Richard Weinberger wrote: >>>>>> Vignesh, >>>>>> >>>>>> Am 27.02.2017 um 13:08 schrieb Vignesh R: >>>>>>> Many SPI controller drivers use DMA to read/write from m25p80 compatible >>>>>>> flashes. Therefore enable bounce buffers support provided by spi-nor >>>>>>> framework to take care of handling vmalloc'd buffers which may not be >>>>>>> DMA'able. >>>>>>> >>>>>>> Signed-off-by: Vignesh R >>>>>>> --- >>>>>>> drivers/mtd/devices/m25p80.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c >>>>>>> index c4df3b1bded0..d05acf22eadf 100644 >>>>>>> --- a/drivers/mtd/devices/m25p80.c >>>>>>> +++ b/drivers/mtd/devices/m25p80.c >>>>>>> @@ -241,6 +241,7 @@ static int m25p_probe(struct spi_device *spi) >>>>>>> else >>>>>>> flash_name = spi->modalias; >>>>>>> >>>>>>> + nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>>> >>>>>> Isn't there a better way to detect whether a bounce buffer is needed or not? >>>>> >>>> >>>> I agree with Richard: the bounce buffer should be enabled only if needed >>>> by the SPI controller. >>>> >>>>> Yes, I can poke the spi->master struct to see of dma channels are >>>>> populated and request SNOR_F_USE_BOUNCE_BUFFER accordingly: >>>>> >>>>> - nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>> + if (spi->master->dma_tx || spi->master->dma_rx) >>>>> + nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>> + >>>>> >>>> >>>> However I don't agree with this solution: master->dma_{tx|rx} can be set >>>> for SPI controllers which already rely on spi_map_msg() to handle >>>> vmalloc'ed memory during DMA transfers. >>>> Such SPI controllers don't need the spi-nor bounce buffer. >>>> >>>> spi_map_msg() can build a scatter-gather list from vmalloc'ed buffer >>>> then map this sg list with dma_map_sg(). AFAIK, It is safe to do so for >>>> architectures using PIPT caches since the possible cache aliases issue >>>> present for VIPT or VIVT caches is always avoided for PIPT caches. >>>> >>>> For instance, the drivers/spi/spi-atmel.c driver relies on spi_map_sg() >>>> to be called from the SPI sub-system to handle vmalloc'ed buffers and >>>> both master->dma_tx and master->dma_rx are set by the this driver. >>>> >>>> >>>> By the way, Is there any case where the same physical page is actually >>>> mapped into two different virtual addresses for the buffers allocated by >>>> the MTD sub-system? Because for a long time now I wonder whether the >>>> cache aliases issue is a real or only theoretical issue but I have no >>>> answer to that question. >>>> >>> >>> I have atleast one evidence of VIVT aliasing causing problem. Please see >>> this thread on DMA issues with davinci-spi driver >>> https://www.spinics.net/lists/arm-kernel/msg563420.html >>> https://www.spinics.net/lists/arm-kernel/msg563445.html >>> >>>> Then my next question: is spi_map_msg() enough in every case, even with >>>> VIPT or VIVT caches? >>>> >>> >>> Not really, I am debugging another issue with UBIFS on DRA74 EVM (ARM >>> cortex-a15) wherein pages allocated by vmalloc are in highmem region >>> that are not addressable using 32 bit addresses and is backed by LPAE. >>> So, a 32 bit DMA cannot access these buffers at all. >>> When dma_map_sg() is called to map these pages by spi_map_buf() the >>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of >>> dma_map_sg() call). This results in random crashes as DMA starts >>> accessing random memory during SPI read. >>> >>> IMO, there may be more undiscovered caveat with using dma_map_sg() for >>> non kmalloc'd buffers and its better that spi-nor starts handling these >>> buffers instead of relying on spi_map_msg() and working around every >>> time something pops up. >>> >> >> Both Frode and you confirmed that the alias issue does occur at least >> with VIVT caches, hence we can't rely on spi_map_msg() in that case. >> So I agree with you: adding a bounce buffer in spi-nor seems to be a >> good solution at least till some rework is done in the ubifs layer, as >> proposed by Boris, to replace vmalloc'ed buffers by kmalloc'ed memory. > > We should keep it even after reworking UBI/UBIFS, because UBI is just > one user of MTD, and other users might pass vmalloc-ed or kmap-ed bufs. > > I'm fine with that :) From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cyrille Pitchen Subject: Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support Date: Wed, 1 Mar 2017 15:30:34 +0100 Message-ID: References: <20170227120839.16545-1-vigneshr@ti.com> <20170227120839.16545-3-vigneshr@ti.com> <8f999a27-c3ce-2650-452c-b21c3e44989d@ti.com> <8a2c9b3b-dd5f-fca7-fa5c-690e5bed949f@atmel.com> <20170301152855.0cfcdf0f@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Cc: Vignesh R , Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , , , , Frode Isaksen , , Mark Brown To: Boris Brezillon Return-path: In-Reply-To: <20170301152855.0cfcdf0f@bbrezillon> Sender: linux-spi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Le 01/03/2017 à 15:28, Boris Brezillon a écrit : > On Wed, 1 Mar 2017 15:21:24 +0100 > Cyrille Pitchen wrote: > >> + Mark >> >> Le 01/03/2017 à 12:46, Vignesh R a écrit : >>> >>> >>> On Wednesday 01 March 2017 04:13 PM, Cyrille Pitchen wrote: >>>> Le 01/03/2017 à 05:54, Vignesh R a écrit : >>>>> >>>>> >>>>> On Wednesday 01 March 2017 03:11 AM, Richard Weinberger wrote: >>>>>> Vignesh, >>>>>> >>>>>> Am 27.02.2017 um 13:08 schrieb Vignesh R: >>>>>>> Many SPI controller drivers use DMA to read/write from m25p80 compatible >>>>>>> flashes. Therefore enable bounce buffers support provided by spi-nor >>>>>>> framework to take care of handling vmalloc'd buffers which may not be >>>>>>> DMA'able. >>>>>>> >>>>>>> Signed-off-by: Vignesh R >>>>>>> --- >>>>>>> drivers/mtd/devices/m25p80.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c >>>>>>> index c4df3b1bded0..d05acf22eadf 100644 >>>>>>> --- a/drivers/mtd/devices/m25p80.c >>>>>>> +++ b/drivers/mtd/devices/m25p80.c >>>>>>> @@ -241,6 +241,7 @@ static int m25p_probe(struct spi_device *spi) >>>>>>> else >>>>>>> flash_name = spi->modalias; >>>>>>> >>>>>>> + nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>>> >>>>>> Isn't there a better way to detect whether a bounce buffer is needed or not? >>>>> >>>> >>>> I agree with Richard: the bounce buffer should be enabled only if needed >>>> by the SPI controller. >>>> >>>>> Yes, I can poke the spi->master struct to see of dma channels are >>>>> populated and request SNOR_F_USE_BOUNCE_BUFFER accordingly: >>>>> >>>>> - nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>> + if (spi->master->dma_tx || spi->master->dma_rx) >>>>> + nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>> + >>>>> >>>> >>>> However I don't agree with this solution: master->dma_{tx|rx} can be set >>>> for SPI controllers which already rely on spi_map_msg() to handle >>>> vmalloc'ed memory during DMA transfers. >>>> Such SPI controllers don't need the spi-nor bounce buffer. >>>> >>>> spi_map_msg() can build a scatter-gather list from vmalloc'ed buffer >>>> then map this sg list with dma_map_sg(). AFAIK, It is safe to do so for >>>> architectures using PIPT caches since the possible cache aliases issue >>>> present for VIPT or VIVT caches is always avoided for PIPT caches. >>>> >>>> For instance, the drivers/spi/spi-atmel.c driver relies on spi_map_sg() >>>> to be called from the SPI sub-system to handle vmalloc'ed buffers and >>>> both master->dma_tx and master->dma_rx are set by the this driver. >>>> >>>> >>>> By the way, Is there any case where the same physical page is actually >>>> mapped into two different virtual addresses for the buffers allocated by >>>> the MTD sub-system? Because for a long time now I wonder whether the >>>> cache aliases issue is a real or only theoretical issue but I have no >>>> answer to that question. >>>> >>> >>> I have atleast one evidence of VIVT aliasing causing problem. Please see >>> this thread on DMA issues with davinci-spi driver >>> https://www.spinics.net/lists/arm-kernel/msg563420.html >>> https://www.spinics.net/lists/arm-kernel/msg563445.html >>> >>>> Then my next question: is spi_map_msg() enough in every case, even with >>>> VIPT or VIVT caches? >>>> >>> >>> Not really, I am debugging another issue with UBIFS on DRA74 EVM (ARM >>> cortex-a15) wherein pages allocated by vmalloc are in highmem region >>> that are not addressable using 32 bit addresses and is backed by LPAE. >>> So, a 32 bit DMA cannot access these buffers at all. >>> When dma_map_sg() is called to map these pages by spi_map_buf() the >>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of >>> dma_map_sg() call). This results in random crashes as DMA starts >>> accessing random memory during SPI read. >>> >>> IMO, there may be more undiscovered caveat with using dma_map_sg() for >>> non kmalloc'd buffers and its better that spi-nor starts handling these >>> buffers instead of relying on spi_map_msg() and working around every >>> time something pops up. >>> >> >> Both Frode and you confirmed that the alias issue does occur at least >> with VIVT caches, hence we can't rely on spi_map_msg() in that case. >> So I agree with you: adding a bounce buffer in spi-nor seems to be a >> good solution at least till some rework is done in the ubifs layer, as >> proposed by Boris, to replace vmalloc'ed buffers by kmalloc'ed memory. > > We should keep it even after reworking UBI/UBIFS, because UBI is just > one user of MTD, and other users might pass vmalloc-ed or kmap-ed bufs. > > I'm fine with that :) -- 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cyrille Pitchen Subject: Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support Date: Wed, 1 Mar 2017 15:30:34 +0100 Message-ID: References: <20170227120839.16545-1-vigneshr@ti.com> <20170227120839.16545-3-vigneshr@ti.com> <8f999a27-c3ce-2650-452c-b21c3e44989d@ti.com> <8a2c9b3b-dd5f-fca7-fa5c-690e5bed949f@atmel.com> <20170301152855.0cfcdf0f@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20170301152855.0cfcdf0f@bbrezillon> Sender: linux-spi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Boris Brezillon Cc: Vignesh R , Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Frode Isaksen , linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Brown List-Id: linux-omap@vger.kernel.org Le 01/03/2017 à 15:28, Boris Brezillon a écrit : > On Wed, 1 Mar 2017 15:21:24 +0100 > Cyrille Pitchen wrote: > >> + Mark >> >> Le 01/03/2017 à 12:46, Vignesh R a écrit : >>> >>> >>> On Wednesday 01 March 2017 04:13 PM, Cyrille Pitchen wrote: >>>> Le 01/03/2017 à 05:54, Vignesh R a écrit : >>>>> >>>>> >>>>> On Wednesday 01 March 2017 03:11 AM, Richard Weinberger wrote: >>>>>> Vignesh, >>>>>> >>>>>> Am 27.02.2017 um 13:08 schrieb Vignesh R: >>>>>>> Many SPI controller drivers use DMA to read/write from m25p80 compatible >>>>>>> flashes. Therefore enable bounce buffers support provided by spi-nor >>>>>>> framework to take care of handling vmalloc'd buffers which may not be >>>>>>> DMA'able. >>>>>>> >>>>>>> Signed-off-by: Vignesh R >>>>>>> --- >>>>>>> drivers/mtd/devices/m25p80.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c >>>>>>> index c4df3b1bded0..d05acf22eadf 100644 >>>>>>> --- a/drivers/mtd/devices/m25p80.c >>>>>>> +++ b/drivers/mtd/devices/m25p80.c >>>>>>> @@ -241,6 +241,7 @@ static int m25p_probe(struct spi_device *spi) >>>>>>> else >>>>>>> flash_name = spi->modalias; >>>>>>> >>>>>>> + nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>>> >>>>>> Isn't there a better way to detect whether a bounce buffer is needed or not? >>>>> >>>> >>>> I agree with Richard: the bounce buffer should be enabled only if needed >>>> by the SPI controller. >>>> >>>>> Yes, I can poke the spi->master struct to see of dma channels are >>>>> populated and request SNOR_F_USE_BOUNCE_BUFFER accordingly: >>>>> >>>>> - nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>> + if (spi->master->dma_tx || spi->master->dma_rx) >>>>> + nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>> + >>>>> >>>> >>>> However I don't agree with this solution: master->dma_{tx|rx} can be set >>>> for SPI controllers which already rely on spi_map_msg() to handle >>>> vmalloc'ed memory during DMA transfers. >>>> Such SPI controllers don't need the spi-nor bounce buffer. >>>> >>>> spi_map_msg() can build a scatter-gather list from vmalloc'ed buffer >>>> then map this sg list with dma_map_sg(). AFAIK, It is safe to do so for >>>> architectures using PIPT caches since the possible cache aliases issue >>>> present for VIPT or VIVT caches is always avoided for PIPT caches. >>>> >>>> For instance, the drivers/spi/spi-atmel.c driver relies on spi_map_sg() >>>> to be called from the SPI sub-system to handle vmalloc'ed buffers and >>>> both master->dma_tx and master->dma_rx are set by the this driver. >>>> >>>> >>>> By the way, Is there any case where the same physical page is actually >>>> mapped into two different virtual addresses for the buffers allocated by >>>> the MTD sub-system? Because for a long time now I wonder whether the >>>> cache aliases issue is a real or only theoretical issue but I have no >>>> answer to that question. >>>> >>> >>> I have atleast one evidence of VIVT aliasing causing problem. Please see >>> this thread on DMA issues with davinci-spi driver >>> https://www.spinics.net/lists/arm-kernel/msg563420.html >>> https://www.spinics.net/lists/arm-kernel/msg563445.html >>> >>>> Then my next question: is spi_map_msg() enough in every case, even with >>>> VIPT or VIVT caches? >>>> >>> >>> Not really, I am debugging another issue with UBIFS on DRA74 EVM (ARM >>> cortex-a15) wherein pages allocated by vmalloc are in highmem region >>> that are not addressable using 32 bit addresses and is backed by LPAE. >>> So, a 32 bit DMA cannot access these buffers at all. >>> When dma_map_sg() is called to map these pages by spi_map_buf() the >>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of >>> dma_map_sg() call). This results in random crashes as DMA starts >>> accessing random memory during SPI read. >>> >>> IMO, there may be more undiscovered caveat with using dma_map_sg() for >>> non kmalloc'd buffers and its better that spi-nor starts handling these >>> buffers instead of relying on spi_map_msg() and working around every >>> time something pops up. >>> >> >> Both Frode and you confirmed that the alias issue does occur at least >> with VIVT caches, hence we can't rely on spi_map_msg() in that case. >> So I agree with you: adding a bounce buffer in spi-nor seems to be a >> good solution at least till some rework is done in the ubifs layer, as >> proposed by Boris, to replace vmalloc'ed buffers by kmalloc'ed memory. > > We should keep it even after reworking UBI/UBIFS, because UBI is just > one user of MTD, and other users might pass vmalloc-ed or kmap-ed bufs. > > I'm fine with that :) -- 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