From: mzoran@crowfest.net (Michael Zoran)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] staging: vc04_services: tie up loose ends with dma_map_sg conversion
Date: Fri, 28 Oct 2016 08:36:34 -0700 [thread overview]
Message-ID: <1477668994.12378.1.camel@crowfest.net> (raw)
In-Reply-To: <20161028153124.GA29906@kroah.com>
On Fri, 2016-10-28 at 11:31 -0400, Greg KH wrote:
> 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
Point #1 and #2 would be very easy to seperate. Point #3 and #4 are
essentually a redo of two major functions and are where most of the
changes are.
Would making #1 and #2 independent but combining #3 and #4 sufficient?
next prev parent reply other threads:[~2016-10-28 15:36 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-28 15:16 [PATCH] staging: vc04_services: tie up loose ends with dma_map_sg conversion Michael Zoran
2016-10-28 15:31 ` Greg KH
2016-10-28 15:36 ` Michael Zoran [this message]
2016-10-28 15:42 ` Greg KH
2016-10-28 16:02 ` Michael Zoran
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1477668994.12378.1.camel@crowfest.net \
--to=mzoran@crowfest.net \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).