netdev - Re: [PATCH] [v9] net: emac: emac gigabit ethernet controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 31 Aug 2016 12:15:10 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Timur Tabi <timur@...eaurora.org>,
        Rami Rosen <roszenrami@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>, devicetree@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, sdharia@...eaurora.org,
        shankerd@...eaurora.org, vikrams@...eaurora.org,
        cov@...eaurora.org, gavidov@...eaurora.org, robh+dt@...nel.org,
        andrew@...n.ch, bjorn.andersson@...aro.org, mlangsdo@...hat.com,
        jcm@...hat.com, agross@...eaurora.org,
        David Miller <davem@...emloft.net>, LinoSanfilippo@....de
Subject: Re: [PATCH] [v9] net: emac: emac gigabit ethernet controller driver

On 08/31/2016 11:57 AM, Timur Tabi wrote:
> Timur Tabi wrote:
>>
>>> Seems that there are several unused members in the emac_stats struct:
>>>
>>>> +struct emac_stats {
>>> ...
>>> ...
>>> Both rx_bcast_byte_cnt and rx_mcast_byte_cnt are not used anywhere/
>>>> +       u64 rx_bcast_byte_cnt;  /* broadcast packets byte count
>>>> (without FCS) */
>>>> +       u64 rx_mcast_byte_cnt;  /* multicast packets byte count
>>>> (without FCS) */
>>> ...
>>> rx_err_addr is not used
>>>> +       u64 rx_err_addr;        /* packets dropped due to address
>>>> filtering */
>>
>> I'll go through the structure and remove the unused fields.
> 
> It turns out I cannot actually strip out those "unused" fields.  They
> are all indirectly used in emac_get_stats64:
> 
>     u64 *stats_itr = &adpt->stats.rx_ok;
> 
>     while (addr <= REG_MAC_RX_STATUS_END) {
>         val = readl_relaxed(adpt->base + addr);
>         *stats_itr += val;
>         stats_itr++;
>         addr += sizeof(u32);
>     }

if these are truly 64-bits stats, how come you are using a single
readl_* to access them? Or is the u64 rx_err_addr just used as temporary
storage and aligned to the largest size you need to deal with?
-- 
Florian