RE: [PATCH net-next 1/3] net/usb/r8152: support aggregation
David Miller [mailto:da...@davemloft.net] > Sent: Wednesday, August 14, 2013 7:41 AM > To: oneu...@suse.de > Cc: Hayeswang; net...@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation > [...] > > 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. I just want to make sure the rx_desc and rx_data are set correctly before they are used. However, I study some examples and information from internet, and I think that the memory barries is not necessary here. Therefore, I would remove them later. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
From: Oliver Neukum
Date: Tue, 13 Aug 2013 17:17:10 +0200
> On Tue, 2013-08-13 at 20:32 +0800, hayeswang wrote:
>> Oliver Neukum [mailto:oneu...@suse.de]
>> > Sent: Tuesday, August 13, 2013 4:49 PM
>> > To: Hayeswang
>> > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> > linux-...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
On Tue, 2013-08-13 at 20:32 +0800, hayeswang wrote:
> Oliver Neukum [mailto:oneu...@suse.de]
> > Sent: Tuesday, August 13, 2013 4:49 PM
> > To: Hayeswang
> > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-...@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.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/3] net/usb/r8152: support aggregation
Oliver Neukum [mailto:oneu...@suse.de]
> Sent: Tuesday, August 13, 2013 4:49 PM
> To: Hayeswang
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-...@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?
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
On Tue, 2013-08-13 at 15:28 +0800, Hayes Wang wrote:
> +
> +static void rx_bottom(struct r8152 *tp)
> +{
> + struct net_device_stats *stats;
> + struct net_device *netdev;
> + struct rx_agg *agg;
> + struct rx_desc *rx_desc;
> + unsigned long lockflags;
> + struct list_head *cursor, *next;
> + struct sk_buff *skb;
> + struct urb *urb;
> + unsigned pkt_len;
> + int len_used;
> + u8 *rx_data;
> + int ret;
> +
> + netdev = tp->netdev;
> +
> + if (!netif_running(netdev))
> return;
> - dev_kfree_skb_irq(tp->tx_skb);
> - if (!netif_device_present(tp->netdev))
> +
> + stats = rtl8152_get_stats(netdev);
> +
> + spin_lock_irqsave(>rx_lock, lockflags);
> + list_for_each_safe(cursor, next, >rx_done) {
> + list_del_init(cursor);
> + spin_unlock_irqrestore(>rx_lock, lockflags);
> +
> + agg = list_entry(cursor, struct rx_agg, list);
> + urb = agg->urb;
> + if (urb->actual_length < ETH_ZLEN) {
> + ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
> + spin_lock_irqsave(>rx_lock, lockflags);
> + if (ret && ret != -ENODEV) {
> + list_add_tail(>list, next);
> + tasklet_schedule(>tl);
> + }
> + continue;
> + }
> +
> + 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?
> + pkt_len = le32_to_cpu(rx_desc->opts1) &
> RX_LEN_MASK;
> + len_used = (int)(rx_data - (u8 *)agg->head);
> + len_used += sizeof(struct rx_desc) + pkt_len;
> + }
> +
> + ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
> + spin_lock_irqsave(>rx_lock, lockflags);
> + if (ret && ret != -ENODEV) {
> + list_add_tail(>list, next);
> + tasklet_schedule(>tl);
> + }
> + }
> + spin_unlock_irqrestore(>rx_lock, lockflags);
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
On Tue, 2013-08-13 at 15:28 +0800, Hayes Wang wrote:
+
+static void rx_bottom(struct r8152 *tp)
+{
+ struct net_device_stats *stats;
+ struct net_device *netdev;
+ struct rx_agg *agg;
+ struct rx_desc *rx_desc;
+ unsigned long lockflags;
+ struct list_head *cursor, *next;
+ struct sk_buff *skb;
+ struct urb *urb;
+ unsigned pkt_len;
+ int len_used;
+ u8 *rx_data;
+ int ret;
+
+ netdev = tp-netdev;
+
+ if (!netif_running(netdev))
return;
- dev_kfree_skb_irq(tp-tx_skb);
- if (!netif_device_present(tp-netdev))
+
+ stats = rtl8152_get_stats(netdev);
+
+ spin_lock_irqsave(tp-rx_lock, lockflags);
+ list_for_each_safe(cursor, next, tp-rx_done) {
+ list_del_init(cursor);
+ spin_unlock_irqrestore(tp-rx_lock, lockflags);
+
+ agg = list_entry(cursor, struct rx_agg, list);
+ urb = agg-urb;
+ if (urb-actual_length ETH_ZLEN) {
+ ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
+ spin_lock_irqsave(tp-rx_lock, lockflags);
+ if (ret ret != -ENODEV) {
+ list_add_tail(agg-list, next);
+ tasklet_schedule(tp-tl);
+ }
+ continue;
+ }
+
+ 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?
+ pkt_len = le32_to_cpu(rx_desc-opts1)
RX_LEN_MASK;
+ len_used = (int)(rx_data - (u8 *)agg-head);
+ len_used += sizeof(struct rx_desc) + pkt_len;
+ }
+
+ ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
+ spin_lock_irqsave(tp-rx_lock, lockflags);
+ if (ret ret != -ENODEV) {
+ list_add_tail(agg-list, next);
+ tasklet_schedule(tp-tl);
+ }
+ }
+ spin_unlock_irqrestore(tp-rx_lock, lockflags);
+}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/3] net/usb/r8152: support aggregation
Oliver Neukum [mailto:oneu...@suse.de]
Sent: Tuesday, August 13, 2013 4:49 PM
To: Hayeswang
Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-...@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?
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
On Tue, 2013-08-13 at 20:32 +0800, hayeswang wrote:
Oliver Neukum [mailto:oneu...@suse.de]
Sent: Tuesday, August 13, 2013 4:49 PM
To: Hayeswang
Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-...@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.
Regards
Oliver
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
From: Oliver Neukum oneu...@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:oneu...@suse.de]
Sent: Tuesday, August 13, 2013 4:49 PM
To: Hayeswang
Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/3] net/usb/r8152: support aggregation
David Miller [mailto:da...@davemloft.net] Sent: Wednesday, August 14, 2013 7:41 AM To: oneu...@suse.de Cc: Hayeswang; net...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation [...] 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. I just want to make sure the rx_desc and rx_data are set correctly before they are used. However, I study some examples and information from internet, and I think that the memory barries is not necessary here. Therefore, I would remove them later. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
