linux-kernel - Re: [PATCH] staging: vc04_services: tie up loose ends with dma_map

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]

Message-ID: <20161028153124.GA29906@kroah.com>
Date:   Fri, 28 Oct 2016 11:31:24 -0400
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Michael Zoran <mzoran@...wfest.net>
Cc:     eric@...olt.net, swarren@...dotorg.org, lee@...nel.org,
        daniels@...labora.com, noralf@...nnes.org, popcornmix@...il.com,
        linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: vc04_services: tie up loose ends with
 dma_map_sg conversion

On Fri, Oct 28, 2016 at 08:16:51AM -0700, Michael Zoran wrote:
> The conversion to dma_map_sg left a few loose ends.  This change
> ties up those loose ends.
> 
> 1. Settings the DMA mask is mandatory on 64 bit even though it
> is optional on 32 bit.  Set the mask so that DMA space is always
> in the lower 32 bit range of data on both platforms.
> 
> 2. The scatterlist was not completely initialized correctly.
> Initialize the list with sg_init_table so that DMA works correctly
> if scatterlist debugging is enabled in the build configuration.
> 
> 3. The error paths in create_pagelist were not consistent.  Make
> them all consistent by calling a helper function called
> cleanup_pagelistinfo to cleanup regardless of what state the pagelist
> is in.
> 
> 4. create_pagelist and free_pagelist had a very large amount of
> duplication in computing offsets into a single allocation of memory
> in the DMA area.  Introduce a new structure called the pagelistinfo
> that is appened to the end of the allocation to store necessary
> information to prevent duplication of code and make cleanup on errors
> easier.
> 
> When combined with a fix for vchiq_copy_from_user which is not
> included at this time, both functional and pings tests of vchiq_test
> now pass in both 32 bit and 64 bit modes.
> 
> Even though this cleanup could have been broken down to chunks,
> all the changes are to a single file and submitting it as a single
> related change should make reviewing the diff much easier then if it
> were submitted piecemeal.

No, it's harder.  A patch should only do one type of thing, this patch
has to be reviewed thinking of 4 different things all at once, making it
much more difficult to do so.

We write patches to be read easily, and make them easy to review.  We
don't write them in a way to be easy for the developer to create :)

Can you please break this up into a patch series?

thanks,

greg k-h