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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux