* errors in alignment changes..
@ 2014-12-10 20:52 David Miller
2014-12-10 22:08 ` Sergei Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2014-12-10 20:52 UTC (permalink / raw)
To: mitsuhiro.kimura.kc; +Cc: netdev
I just re-reviewed this change:
commit 4d6a949c62f123569fb355b6ec7f314b76f93735
Author: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
Date: Thu Nov 27 20:34:00 2014 +0900
sh_eth: Fix skb alloc size and alignment adjust rule.
and it has serious problems. Well, actually the code has
always been broken in this area.
+ /* The size of the buffer is a multiple of 16 bytes. */
+ rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
+ dma_map_single(&ndev->dev, skb->data, rxdesc->buffer_length,
+ DMA_FROM_DEVICE);
rxdesc->addr = virt_to_phys(PTR_ALIGN(skb->data, 4));
rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
It doesn't make any sense to call dma_map_single() if you aren't
even going to use the return value. The DMA mapping created is
the whole point of calling this function.
And then later we pass:
dma_sync_single_for_cpu(&ndev->dev, rxdesc->addr,
- mdp->rx_buf_sz,
+ ALIGN(mdp->rx_buf_sz, 16),
DMA_FROM_DEVICE);
rxdesc->addr as the "DMA address", but that must be the return value
from dma_map_single() not what you've actually stored there which is
virt_to_phys() run on the skb->data.
This code must be fixed to:
1) Put the return value from dma_map_single() into a local variable,
and check for mapping errors.
2) On success put that return value into rxdesc->addr
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: errors in alignment changes..
2014-12-10 20:52 errors in alignment changes David Miller
@ 2014-12-10 22:08 ` Sergei Shtylyov
2014-12-11 1:42 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2014-12-10 22:08 UTC (permalink / raw)
To: David Miller, mitsuhiro.kimura.kc; +Cc: netdev
On 12/10/2014 11:52 PM, David Miller wrote:
> I just re-reviewed this change:
> commit 4d6a949c62f123569fb355b6ec7f314b76f93735
> Author: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> Date: Thu Nov 27 20:34:00 2014 +0900
> sh_eth: Fix skb alloc size and alignment adjust rule.
> and it has serious problems. Well, actually the code has
> always been broken in this area.
>
> + /* The size of the buffer is a multiple of 16 bytes. */
> + rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
> + dma_map_single(&ndev->dev, skb->data, rxdesc->buffer_length,
> + DMA_FROM_DEVICE);
> rxdesc->addr = virt_to_phys(PTR_ALIGN(skb->data, 4));
> rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
> It doesn't make any sense to call dma_map_single() if you aren't
> even going to use the return value. The DMA mapping created is
> the whole point of calling this function.
Note that this call is just moved from above, not added by this patch.
> And then later we pass:
> dma_sync_single_for_cpu(&ndev->dev, rxdesc->addr,
> - mdp->rx_buf_sz,
> + ALIGN(mdp->rx_buf_sz, 16),
> DMA_FROM_DEVICE);
> rxdesc->addr as the "DMA address", but that must be the return value
> from dma_map_single() not what you've actually stored there which is
> virt_to_phys() run on the skb->data.
> This code must be fixed to:
> 1) Put the return value from dma_map_single() into a local variable,
I have already requested such change in reply to the patch fixing several
DMA errors at once.
> and check for mapping errors.
This was done by that patch IIRC. It just needs to be properly split and
resubmitted.
> 2) On success put that return value into rxdesc->addr
I guess we can just do:
rxdesc->addr = dma_map_single(...);
WBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: errors in alignment changes..
2014-12-10 22:08 ` Sergei Shtylyov
@ 2014-12-11 1:42 ` David Miller
2014-12-12 4:30 ` Simon Horman
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2014-12-11 1:42 UTC (permalink / raw)
To: sergei.shtylyov; +Cc: mitsuhiro.kimura.kc, netdev
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Thu, 11 Dec 2014 01:08:07 +0300
> I guess we can just do:
>
> rxdesc->addr = dma_map_single(...);
Best not to leave a potentially invalid DMA address in a
receive descriptor the chip can potentially fetch and
look at.
That's why I said to put it into a local variable and
check for errors first.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: errors in alignment changes..
2014-12-11 1:42 ` David Miller
@ 2014-12-12 4:30 ` Simon Horman
2014-12-12 12:00 ` Sergei Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: Simon Horman @ 2014-12-12 4:30 UTC (permalink / raw)
To: David Miller; +Cc: sergei.shtylyov, mitsuhiro.kimura.kc, netdev
On Wed, Dec 10, 2014 at 08:42:15PM -0500, David Miller wrote:
> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Date: Thu, 11 Dec 2014 01:08:07 +0300
>
> > I guess we can just do:
> >
> > rxdesc->addr = dma_map_single(...);
>
> Best not to leave a potentially invalid DMA address in a
> receive descriptor the chip can potentially fetch and
> look at.
>
> That's why I said to put it into a local variable and
> check for errors first.
Hi Dave,
this patch ending up in net is partially my fault.
Sergei, do you have time to address David's concerns in relation to this
patch? If not I would like to suggest reverting it for now.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: errors in alignment changes..
2014-12-12 4:30 ` Simon Horman
@ 2014-12-12 12:00 ` Sergei Shtylyov
2014-12-16 5:33 ` Simon Horman
0 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2014-12-12 12:00 UTC (permalink / raw)
To: Simon Horman, David Miller; +Cc: mitsuhiro.kimura.kc, netdev
Hello.
On 12/12/2014 7:30 AM, Simon Horman wrote:
>> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Date: Thu, 11 Dec 2014 01:08:07 +0300
>>> I guess we can just do:
>>> rxdesc->addr = dma_map_single(...);
>> Best not to leave a potentially invalid DMA address in a
>> receive descriptor the chip can potentially fetch and
>> look at.
>> That's why I said to put it into a local variable and
>> check for errors first.
> Hi Dave,
> this patch ending up in net is partially my fault.
> Sergei, do you have time to address David's concerns in relation to this
> patch?
No, I probably don't.
> If not I would like to suggest reverting it for now.
Why? The patch does what it was intended for. Getting rid of
virt_to_phys() calls is a separate issue, IMO.
WBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: errors in alignment changes..
2014-12-12 12:00 ` Sergei Shtylyov
@ 2014-12-16 5:33 ` Simon Horman
0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2014-12-16 5:33 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: David Miller, mitsuhiro.kimura.kc, netdev
On Fri, Dec 12, 2014 at 03:00:16PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 12/12/2014 7:30 AM, Simon Horman wrote:
>
> >>From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>Date: Thu, 11 Dec 2014 01:08:07 +0300
>
> >>> I guess we can just do:
>
> >>> rxdesc->addr = dma_map_single(...);
>
> >>Best not to leave a potentially invalid DMA address in a
> >>receive descriptor the chip can potentially fetch and
> >>look at.
>
> >>That's why I said to put it into a local variable and
> >>check for errors first.
>
> >Hi Dave,
>
> >this patch ending up in net is partially my fault.
>
> >Sergei, do you have time to address David's concerns in relation to this
> >patch?
>
> No, I probably don't.
>
> >If not I would like to suggest reverting it for now.
>
> Why? The patch does what it was intended for. Getting rid of
> virt_to_phys() calls is a separate issue, IMO.
Thanks, that is fine by me.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-16 5:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-10 20:52 errors in alignment changes David Miller
2014-12-10 22:08 ` Sergei Shtylyov
2014-12-11 1:42 ` David Miller
2014-12-12 4:30 ` Simon Horman
2014-12-12 12:00 ` Sergei Shtylyov
2014-12-16 5:33 ` Simon Horman
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).