linux-kernel - Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 3 Jan 2017 18:18:01 +0100
From:   Noralf Trønnes <noralf@...nnes.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask


Den 03.01.2017 14:58, skrev Andy Shevchenko:
> On Tue, 2017-01-03 at 12:51 +0200, Andy Shevchenko wrote:
>> On Mon, 2017-01-02 at 19:14 +0100, Noralf Trønnes wrote:
>>> Den 02.01.2017 12:35, skrev Andy Shevchenko:
>>>> Usually it's not consumer's business to override resources passed
>>>> from
>>>> provider, in particularly DMA coherent mask.
>>>> --- a/drivers/staging/fbtft/fbtft-core.c
>>>> +++ b/drivers/staging/fbtft/fbtft-core.c
>>>> @@ -841,7 +841,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct
>>>> fbtft_display *display,
>>>>    	if (txbuflen > 0) {
>>>>    #ifdef CONFIG_HAS_DMA
>>>>    		if (dma) {
>>>> -			dev->coherent_dma_mask = ~0;
>>> Can we make this conditional like in of_dma_configure():
>>>
>>>            if (!dev->coherent_dma_mask)
>>>                   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>>>
>>> If not, I guess the mask has to be set before adding the spi device
>>> in
>>> fbtft_device.c to keep it from breaking.
>> Good point. I will check this.
> So, I was too fast.
>
> Clearly an SPI slave device does not and *should not* know about DMA
> capabilities of SPI *host* controller. It's completely out of slave's
> business.
>
> The masks and other DMA properties only makes sense for the device which
> *actually does DMA*, and apparently SPI slaves do not suit that category
> (there are might be real SPI slaves with private DMA engines, though
> it's another story).
>
> Thus, the patch from my point of view should be kept in the same form.
>
> Regarding to the kbuild bot warning, would you like me to resend it
> fixed? Whatever the decision, I will wait for more comments and
> hopefully your tags.
>
> P.S. I dunno how it did work before, since DMA mask of slave device
> basically has no effect. Perhaps first argument to allocator should be
> NULL. That's only amendment I can see to this particular patch.
>

I have looked at this more closely and it seems that
spi_message.is_dma_mapped is deprecated. And if the spi master driver
can do dma, then the spi core will dma map the buffer (spi_map_buf()
even does vmalloc'ed buffers), and this is what happens for most dma
capable master drivers it seems. Since kmalloc allocates buffers that
is always dma mappable, we can always use kmalloc() instead of
dmam_alloc_coherent() for the transfer buffer.

In addition cleaning up these would prevent any later confusion:

drivers/staging/fbtft/fbtft.h:

  struct fbtft_par {
          struct {
                  void *buf;
-                dma_addr_t dma;
                  size_t len;
          } txbuf;


drivers/staging/fbtft/fbtft-core.c:

-#include <linux/dma-mapping.h>

-#ifdef CONFIG_HAS_DMA
-static bool dma = true;
-module_param(dma, bool, 0);
-MODULE_PARM_DESC(dma, "Use DMA buffer");
-#endif

         if (par->txbuf.buf)
-               sprintf(text1, ", %zu KiB %sbuffer memory",
-                       par->txbuf.len >> 10, par->txbuf.dma ? "DMA " : "");
+               sprintf(text1, ", %zu KiB buffer memory", par->txbuf.len 
 >> 10);


drivers/staging/fbtft/fbtft-io.c:

  int fbtft_write_spi(struct fbtft_par *par, void *buf, size_t len)
-       if (par->txbuf.dma && buf == par->txbuf.buf) {
-               t.tx_dma = par->txbuf.dma;
-               m.is_dma_mapped = 1;
-       }