lkml.org 
[lkml]   [2013]   [Aug]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH net-next 1/3] net/usb/r8152: support aggregation
From
From: Oliver Neukum <oneukum@suse.de>
Date: Tue, 13 Aug 2013 17:17:10 +0200

> On Tue, 2013-08-13 at 20:32 +0800, hayeswang wrote:
>> Oliver Neukum [mailto:oneukum@suse.de]
>> > Sent: Tuesday, August 13, 2013 4:49 PM
>> > To: Hayeswang
>> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> > linux-usb@vger.kernel.org
>> > Subject: Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
>> >
>> [...]
>> > > + len_used = 0;
>> > > + rx_desc = agg->head;
>> > > + rx_data = agg->head;
>> > > + smp_wmb();
>> > > + pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
>> > > + len_used += sizeof(struct rx_desc) + pkt_len;
>> > > +
>> > > + while (urb->actual_length >= len_used) {
>> > > + if (pkt_len < ETH_ZLEN)
>> > > + break;
>> > > +
>> > > + pkt_len -= 4; /* CRC */
>> > > + rx_data += sizeof(struct rx_desc);
>> > > +
>> > > + skb = netdev_alloc_skb_ip_align(netdev,
>> > > pkt_len);
>> > > + if (!skb) {
>> > > + stats->rx_dropped++;
>> > > + break;
>> > > + }
>> > > + memcpy(skb->data, rx_data, pkt_len);
>> > > + skb_put(skb, pkt_len);
>> > > + skb->protocol = eth_type_trans(skb, netdev);
>> > > + netif_rx(skb);
>> > > + stats->rx_packets++;
>> > > + stats->rx_bytes += pkt_len;
>> > > +
>> > > + rx_data = rx_agg_align(rx_data +
>> > pkt_len + 4);
>> > > + rx_desc = (struct rx_desc *)rx_data;
>> > > + smp_wmb();
>> >
>> > Against what is the memory barrier?
>>
>> Excuse me. I don't understand your question. Do you mean the function should not
>> be used here?
>
> I don't understand what problem the function is supposed to fix. As long
> as I don't understand it I cannot say for sure whether it is correct.
> There seems no obvious reason for a memory barrier, but there may be a
> hidden reason I don't see.

Hayes, when Oliver asks you "Against what is the memory barrier?" he is asking
you which memory operations you are trying to order.

You do not explain this in your commit message, nor do you explain it with a
suitable comment. This is not acceptable.

It is absolutely critical, that any time you add a memory barrier, you add a
comment above the new memory barrier explaining exactly what the barrier is
trying to achieve.

In fact, this is required by our coding standards.


\
 
 \ /
  Last update: 2013-08-14 02:01    [W:0.114 / U:0.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site