Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation

[David Miller]

From: Oliver Neukum <oneukum@xxxxxxx>
Date: Tue, 13 Aug 2013 17:17:10 +0200

> On Tue, 2013-08-13 at 20:32 +0800, hayeswang wrote:
>>  Oliver Neukum [mailto:oneukum@xxxxxxx] 
>> > Sent: Tuesday, August 13, 2013 4:49 PM
>> > To: Hayeswang
>> > Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 
>> > linux-usb@xxxxxxxxxxxxxxx
>> > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/