From: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>
Subject: Re: [PATCH for v3.18] media: use sg = sg_next(sg) instead of sg++
Date: Mon, 24 Nov 2014 16:51:16 -0200 [thread overview]
Message-ID: <20141124165116.4641ef9e@recife.lan> (raw)
In-Reply-To: <54737AFC.3080801@xs4all.nl>
Em Mon, 24 Nov 2014 19:37:48 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> On 11/24/2014 07:24 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 21 Nov 2014 10:20:56 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> >> Several drivers (mostly copy-and-paste) still used sg++ instead of
> >> sg = sg_next(sg). Fix them since sg++ won't work if contiguous scatter
> >> entries where combined into one larger entry.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> Cc: stable@vger.kernel.org # for v3.7 and up
> >> ---
> >> drivers/media/pci/bt8xx/bttv-risc.c | 12 ++++++------
> >> drivers/media/pci/cx23885/cx23885-core.c | 6 +++---
> >> drivers/media/pci/cx25821/cx25821-core.c | 12 ++++++------
> >> drivers/media/pci/cx88/cx88-core.c | 6 +++---
> >> drivers/media/pci/ivtv/ivtv-udma.c | 2 +-
> >> 5 files changed, 19 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/media/pci/bt8xx/bttv-risc.c b/drivers/media/pci/bt8xx/bttv-risc.c
> >> index 82cc47d..4d3f05a 100644
> >> --- a/drivers/media/pci/bt8xx/bttv-risc.c
> >> +++ b/drivers/media/pci/bt8xx/bttv-risc.c
> >> @@ -84,7 +84,7 @@ bttv_risc_packed(struct bttv *btv, struct btcx_riscmem *risc,
> >> continue;
> >> while (offset && offset >= sg_dma_len(sg)) {
> >> offset -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >> if (bpl <= sg_dma_len(sg)-offset) {
> >> /* fits into current chunk */
> >> @@ -100,13 +100,13 @@ bttv_risc_packed(struct bttv *btv, struct btcx_riscmem *risc,
> >> *(rp++)=cpu_to_le32(sg_dma_address(sg)+offset);
> >> todo -= (sg_dma_len(sg)-offset);
> >> offset = 0;
> >> - sg++;
> >> + sg = sg_next(sg);
> >> while (todo > sg_dma_len(sg)) {
> >> *(rp++)=cpu_to_le32(BT848_RISC_WRITE|
> >> sg_dma_len(sg));
> >> *(rp++)=cpu_to_le32(sg_dma_address(sg));
> >> todo -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >> *(rp++)=cpu_to_le32(BT848_RISC_WRITE|BT848_RISC_EOL|
> >> todo);
> >> @@ -187,15 +187,15 @@ bttv_risc_planar(struct bttv *btv, struct btcx_riscmem *risc,
> >> /* go to next sg entry if needed */
> >> while (yoffset && yoffset >= sg_dma_len(ysg)) {
> >> yoffset -= sg_dma_len(ysg);
> >> - ysg++;
> >> + ysg = sg_next(ysg);
> >> }
> >> while (uoffset && uoffset >= sg_dma_len(usg)) {
> >> uoffset -= sg_dma_len(usg);
> >> - usg++;
> >> + usg = sg_next(usg);
> >> }
> >> while (voffset && voffset >= sg_dma_len(vsg)) {
> >> voffset -= sg_dma_len(vsg);
> >> - vsg++;
> >> + vsg = sg_next(vsg);
> >> }
> >>
> >> /* calculate max number of bytes we can write */
> >> diff --git a/drivers/media/pci/cx23885/cx23885-core.c b/drivers/media/pci/cx23885/cx23885-core.c
> >> index 331edda..3bd386c 100644
> >> --- a/drivers/media/pci/cx23885/cx23885-core.c
> >> +++ b/drivers/media/pci/cx23885/cx23885-core.c
> >> @@ -1078,7 +1078,7 @@ static __le32 *cx23885_risc_field(__le32 *rp, struct scatterlist *sglist,
> >> for (line = 0; line < lines; line++) {
> >> while (offset && offset >= sg_dma_len(sg)) {
> >> offset -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >>
> >> if (lpi && line > 0 && !(line % lpi))
> >> @@ -1101,14 +1101,14 @@ static __le32 *cx23885_risc_field(__le32 *rp, struct scatterlist *sglist,
> >> *(rp++) = cpu_to_le32(0); /* bits 63-32 */
> >> todo -= (sg_dma_len(sg)-offset);
> >> offset = 0;
> >> - sg++;
> >> + sg = sg_next(sg);
> >> while (todo > sg_dma_len(sg)) {
> >> *(rp++) = cpu_to_le32(RISC_WRITE|
> >> sg_dma_len(sg));
> >> *(rp++) = cpu_to_le32(sg_dma_address(sg));
> >> *(rp++) = cpu_to_le32(0); /* bits 63-32 */
> >> todo -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >> *(rp++) = cpu_to_le32(RISC_WRITE|RISC_EOL|todo);
> >> *(rp++) = cpu_to_le32(sg_dma_address(sg));
> >> diff --git a/drivers/media/pci/cx25821/cx25821-core.c b/drivers/media/pci/cx25821/cx25821-core.c
> >> index e81173c..389fffd 100644
> >> --- a/drivers/media/pci/cx25821/cx25821-core.c
> >> +++ b/drivers/media/pci/cx25821/cx25821-core.c
> >> @@ -996,7 +996,7 @@ static __le32 *cx25821_risc_field(__le32 * rp, struct scatterlist *sglist,
> >> for (line = 0; line < lines; line++) {
> >> while (offset && offset >= sg_dma_len(sg)) {
> >> offset -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >> if (bpl <= sg_dma_len(sg) - offset) {
> >> /* fits into current chunk */
> >> @@ -1014,14 +1014,14 @@ static __le32 *cx25821_risc_field(__le32 * rp, struct scatterlist *sglist,
> >> *(rp++) = cpu_to_le32(0); /* bits 63-32 */
> >> todo -= (sg_dma_len(sg) - offset);
> >> offset = 0;
> >> - sg++;
> >> + sg = sg_next(sg);
> >> while (todo > sg_dma_len(sg)) {
> >> *(rp++) = cpu_to_le32(RISC_WRITE |
> >> sg_dma_len(sg));
> >> *(rp++) = cpu_to_le32(sg_dma_address(sg));
> >> *(rp++) = cpu_to_le32(0); /* bits 63-32 */
> >> todo -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >> *(rp++) = cpu_to_le32(RISC_WRITE | RISC_EOL | todo);
> >> *(rp++) = cpu_to_le32(sg_dma_address(sg));
> >> @@ -1101,7 +1101,7 @@ static __le32 *cx25821_risc_field_audio(__le32 * rp, struct scatterlist *sglist,
> >> for (line = 0; line < lines; line++) {
> >> while (offset && offset >= sg_dma_len(sg)) {
> >> offset -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >>
> >> if (lpi && line > 0 && !(line % lpi))
> >> @@ -1125,14 +1125,14 @@ static __le32 *cx25821_risc_field_audio(__le32 * rp, struct scatterlist *sglist,
> >> *(rp++) = cpu_to_le32(0); /* bits 63-32 */
> >> todo -= (sg_dma_len(sg) - offset);
> >> offset = 0;
> >> - sg++;
> >> + sg = sg_next(sg);
> >> while (todo > sg_dma_len(sg)) {
> >> *(rp++) = cpu_to_le32(RISC_WRITE |
> >> sg_dma_len(sg));
> >> *(rp++) = cpu_to_le32(sg_dma_address(sg));
> >> *(rp++) = cpu_to_le32(0); /* bits 63-32 */
> >> todo -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >> *(rp++) = cpu_to_le32(RISC_WRITE | RISC_EOL | todo);
> >> *(rp++) = cpu_to_le32(sg_dma_address(sg));
> >> diff --git a/drivers/media/pci/cx88/cx88-core.c b/drivers/media/pci/cx88/cx88-core.c
> >> index 9fa4acb..dee177e 100644
> >> --- a/drivers/media/pci/cx88/cx88-core.c
> >> +++ b/drivers/media/pci/cx88/cx88-core.c
> >> @@ -95,7 +95,7 @@ static __le32* cx88_risc_field(__le32 *rp, struct scatterlist *sglist,
> >> for (line = 0; line < lines; line++) {
> >> while (offset && offset >= sg_dma_len(sg)) {
> >> offset -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >> if (lpi && line>0 && !(line % lpi))
> >> sol = RISC_SOL | RISC_IRQ1 | RISC_CNT_INC;
> >> @@ -114,13 +114,13 @@ static __le32* cx88_risc_field(__le32 *rp, struct scatterlist *sglist,
> >> *(rp++)=cpu_to_le32(sg_dma_address(sg)+offset);
> >> todo -= (sg_dma_len(sg)-offset);
> >> offset = 0;
> >> - sg++;
> >> + sg = sg_next(sg);
> >> while (todo > sg_dma_len(sg)) {
> >> *(rp++)=cpu_to_le32(RISC_WRITE|
> >> sg_dma_len(sg));
> >> *(rp++)=cpu_to_le32(sg_dma_address(sg));
> >> todo -= sg_dma_len(sg);
> >> - sg++;
> >> + sg = sg_next(sg);
> >> }
> >> *(rp++)=cpu_to_le32(RISC_WRITE|RISC_EOL|todo);
> >> *(rp++)=cpu_to_le32(sg_dma_address(sg));
> >> diff --git a/drivers/media/pci/ivtv/ivtv-udma.c b/drivers/media/pci/ivtv/ivtv-udma.c
> >> index 7338cb2..bee2329 100644
> >> --- a/drivers/media/pci/ivtv/ivtv-udma.c
> >> +++ b/drivers/media/pci/ivtv/ivtv-udma.c
> >> @@ -76,7 +76,7 @@ void ivtv_udma_fill_sg_array (struct ivtv_user_dma *dma, u32 buffer_offset, u32
> >> int i;
> >> struct scatterlist *sg;
> >>
> >> - for (i = 0, sg = dma->SGlist; i < dma->SG_length; i++, sg++) {
> >> + for (i = 0, sg = dma->SGlist; i < dma->SG_length; i++, sg = sg_next(sg)) {
> >
> > This hunk seems awkward, at least on my eyes.
> >
> > As you've pointed at the description, S/G can be grouped into bigger
> > segments. So, the maximum number of S/G can actually be less than
> > dma->SGlist.
>
> That's correct, and dma->SG_length is equal to that number. SG_length is assigned
> with the result of dma_map_sg() which returns "the number of bus address segments
> mapped (this may be shorter than <nents> passed in if some elements of the
> scatter/gather list are physically or virtually adjacent and an IOMMU maps them
> with a single entry)."
>
> So the patch is correct.
OK.
Btw, you added this patch at a pull request for Kernel 3.19. Shouldn't it
be, instead, be sent to 3.18 and c/c stable?
>
> Regards,
>
> Hans
>
> >
> > The proper fix here seems to convert it into a while that will stop
> > if the accumulated size matches the transfer size, just like the loops
> > at the other similar drivers.
> >
> >> dma->SGarray[i].size = cpu_to_le32(sg_dma_len(sg));
> >> dma->SGarray[i].src = cpu_to_le32(sg_dma_address(sg));
> >> dma->SGarray[i].dst = cpu_to_le32(buffer_offset);
>
next prev parent reply other threads:[~2014-11-24 18:51 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-21 9:20 [PATCH for v3.18] media: use sg = sg_next(sg) instead of sg++ Hans Verkuil
2014-11-21 10:05 ` Hans Verkuil
2014-11-21 10:50 ` Hans Verkuil
2014-11-24 18:24 ` Mauro Carvalho Chehab
2014-11-24 18:37 ` Hans Verkuil
2014-11-24 18:51 ` Mauro Carvalho Chehab [this message]
2014-11-24 19:16 ` Hans Verkuil
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=20141124165116.4641ef9e@recife.lan \
--to=mchehab@osg.samsung.com \
--cc=hverkuil@xs4all.nl \
--cc=linux-media@vger.kernel.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