On 2013/12/29 1:23, Joe Perches wrote: > On Sat, 2013-12-28 at 23:18 +0800, Ding Tianhong wrote: >> δΊ 2013/12/28 21:58, Sergei Shtylyov ει: >>> Hello. >>> >>> On 28-12-2013 10:17, Ding Tianhong wrote: >>> >>>> Use possibly more efficient ether_addr_equal >>>> to instead of memcmp. >>> >>>> Cc: "David S. Miller" <da...@davemloft.net> >>>> Signed-off-by: Ding Tianhong <dingtianh...@huawei.com> >>>> --- >>>> drivers/net/ethernet/packetengines/yellowfin.c | 12 ++++++------ >>>> 1 files changed, 6 insertions(+), 6 deletions(-) >>> >>>> diff --git a/drivers/net/ethernet/packetengines/yellowfin.c >>>> b/drivers/net/ethernet/packetengines/yellowfin.c >>>> index d28593b..b83ac0e 100644 >>>> --- a/drivers/net/ethernet/packetengines/yellowfin.c >>>> +++ b/drivers/net/ethernet/packetengines/yellowfin.c >>>> @@ -1097,12 +1097,12 @@ static int yellowfin_rx(struct net_device *dev) >>>> if (status2 & 0x80) dev->stats.rx_dropped++; >>>> #ifdef YF_PROTOTYPE /* Support for prototype hardware errata. */ >>>> } else if ((yp->flags & HasMACAddrBug) && >>>> - memcmp(le32_to_cpu(yp->rx_ring_dma + >>>> - entry*sizeof(struct yellowfin_desc)), >>>> - dev->dev_addr, 6) != 0 && >>>> - memcmp(le32_to_cpu(yp->rx_ring_dma + >>>> - entry*sizeof(struct yellowfin_desc)), >>>> - "\377\377\377\377\377\377", 6) != 0) { >>>> + !ether_addr_equal(le32_to_cpu(yp->rx_ring_dma + >>>> + entry * sizeof(struct yellowfin_desc)), >>>> + dev->dev_addr) && >>> >>> Previous line was aligned correctly, the above line should start under >>> le32_to_cpu. >>> >>>> + !ether_addr_equal(le32_to_cpu(yp->rx_ring_dma + >>>> + entry * sizeof(struct yellowfin_desc)), >>> >>> Start the continuation lines under 'yp', please. >>> >>>> + "\377\377\377\377\377\377")) { >>> >>> This line should start under le32_to_cpu. >>> >>> WBR, Sergei >>> >> >> Hi sergei: >> you mean this way? >> !ether_addr_equal(le32_to_cpu(yp->rx_ring_dma + >> entry * sizeof(struct yellowfin_desc)), >> dev->dev_addr) && >> >> !ether_addr_equal(le32_to_cpu(yp->rx_ring_dma + >> entry * sizeof(struct yellowfin_desc)), >> "\377\377\377\377\377\377")) { > > Does this really matter? > Does anyone have a packetengine NIC anymore? > > \377 octal is 0xff, so this is matching a broadcast address. > is_broadcast_ether_addr(addr) would be appropriate. > > So would using a temporary address. > > u8 *addr = (u8 *)(unsigned long)le32_to_cpu(etc) > > but the whole thing looks very suspect as an le32 > value could not be added to correctly on a > big-endian arch anyway. > > My guess is this was tested only on an x86 and > it should be: > > u8 *addr = (u8 *)(unsigned long)(le32_to_cpu(yp->rx_ring_dma) + > entry * sizeof(struct yellowfin_desc)); > > It maybe better just to leave these two alone. >
Hi Joe: I don't understand packetengine NIC anymore, But I think the change is clearly, as your said, the broadcast check is enough here, did you mean that? !is_broadcast_ether_addr((u8 *)(le32_to_cpu(yp->rx_ring_dma) + entry * sizeof(struct yellowfin_desc))) Thanks Regards Ding > > . > -- 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/