Re: [PATCH net-next v2 11/20] net: packetengines: slight optimizationof addr

From: Ding Tianhong
Date: Sun Dec 29 2013 - 21:40:29 EST


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" <davem@xxxxxxxxxxxxx>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@xxxxxxxxxx>
>>>> ---
>>>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/