2012-01-05 19:12:31

by Arend van Spriel

[permalink] [raw]
Subject: Re: [PATCH 2/2] brcmfmac: use existing net_device_stats

On 01/05/2012 07:39 PM, Stephen Hemminger wrote:
> Minor space savings. Compile tested only.
>

This is actually not going to work. The struct brcmf_if represents
individual interfaces which each have their own device statistics.

> Signed-off-by: Stephen Hemminger <[email protected]>
>
>
> --- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_linux.c 2012-01-05 10:02:57.488495190 -0800
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_linux.c 2012-01-05 10:09:58.536972268 -0800
> @@ -56,7 +56,6 @@ struct brcmf_if {
> struct brcmf_info *info; /* back pointer to brcmf_info */
> /* OS/stack specifics */
> struct net_device *ndev;
> - struct net_device_stats stats;
> int idx; /* iface idx in dongle */
> int state; /* interface state */
> u8 mac_addr[ETH_ALEN]; /* assigned MAC address */
> @@ -526,7 +525,6 @@ static struct net_device_stats *brcmf_ne
> {
> struct brcmf_info *drvr_priv = *(struct brcmf_info **)
> netdev_priv(ndev);
> - struct brcmf_if *ifp;
> int ifidx;
>

Gr. AvS



2012-01-05 19:39:43

by Franky Lin

[permalink] [raw]
Subject: Re: [PATCH 2/2] brcmfmac: use existing net_device_stats

On 01/05/2012 11:15 AM, Stephen Hemminger wrote:
> On Thu, 5 Jan 2012 20:12:12 +0100
> "Arend van Spriel"<[email protected]> wrote:
>
>> On 01/05/2012 07:39 PM, Stephen Hemminger wrote:
>>> Minor space savings. Compile tested only.
>>>
>>
>> This is actually not going to work. The struct brcmf_if represents
>> individual interfaces which each have their own device statistics.
>>
>
> Why not, if you look it is only used during the aggregation and return
> of netdevice stats.
>

We only support one primary interface at the moment. But we have plan to
add P2P support using virtual interfaces. It would be better to have
individual stats for different interfaces.

Thanks,
Franky


2012-01-05 19:15:57

by Stephen Hemminger

[permalink] [raw]
Subject: Re: [PATCH 2/2] brcmfmac: use existing net_device_stats

On Thu, 5 Jan 2012 20:12:12 +0100
"Arend van Spriel" <[email protected]> wrote:

> On 01/05/2012 07:39 PM, Stephen Hemminger wrote:
> > Minor space savings. Compile tested only.
> >
>
> This is actually not going to work. The struct brcmf_if represents
> individual interfaces which each have their own device statistics.
>
> > Signed-off-by: Stephen Hemminger <[email protected]>
> >
> >
> > --- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_linux.c 2012-01-05 10:02:57.488495190 -0800
> > +++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_linux.c 2012-01-05 10:09:58.536972268 -0800
> > @@ -56,7 +56,6 @@ struct brcmf_if {
> > struct brcmf_info *info; /* back pointer to brcmf_info */
> > /* OS/stack specifics */
> > struct net_device *ndev;
> > - struct net_device_stats stats;
> > int idx; /* iface idx in dongle */
> > int state; /* interface state */
> > u8 mac_addr[ETH_ALEN]; /* assigned MAC address */
> > @@ -526,7 +525,6 @@ static struct net_device_stats *brcmf_ne
> > {
> > struct brcmf_info *drvr_priv = *(struct brcmf_info **)
> > netdev_priv(ndev);
> > - struct brcmf_if *ifp;
> > int ifidx;
> >
>
> Gr. AvS
>

Why not, if you look it is only used during the aggregation and return
of netdevice stats.

2012-01-06 05:00:06

by Stephen Hemminger

[permalink] [raw]
Subject: Re: [PATCH 2/2] brcmfmac: use existing net_device_stats

On Thu, 5 Jan 2012 11:39:34 -0800
"Franky Lin" <[email protected]> wrote:

> On 01/05/2012 11:15 AM, Stephen Hemminger wrote:
> > On Thu, 5 Jan 2012 20:12:12 +0100
> > "Arend van Spriel"<[email protected]> wrote:
> >
> >> On 01/05/2012 07:39 PM, Stephen Hemminger wrote:
> >>> Minor space savings. Compile tested only.
> >>>
> >>
> >> This is actually not going to work. The struct brcmf_if represents
> >> individual interfaces which each have their own device statistics.
> >>
> >
> > Why not, if you look it is only used during the aggregation and return
> > of netdevice stats.
> >
>
> We only support one primary interface at the moment. But we have plan to
> add P2P support using virtual interfaces. It would be better to have
> individual stats for different interfaces.
>
> Thanks,
> Franky
>

You are confused. The only place the data in question is used is:
static struct net_device_stats *brcmf_netdev_get_stats

This is a per-network device standard API and it returns per-network device
statistics. Several releases ago, structure element was added to the network_device
struct for common usage by drivers. All this patch does is use that instead of the
private device scratch pad.

If you really want to be more clever, the device should be convert to the
new getstats64 API, but that requires more work (like wrapping updates with
the u64_stats_sync() macros.

2012-01-06 20:08:00

by Franky Lin

[permalink] [raw]
Subject: Re: [PATCH 2/2] brcmfmac: use existing net_device_stats

On 01/05/2012 09:00 PM, Stephen Hemminger wrote:
> You are confused. The only place the data in question is used is:
> static struct net_device_stats *brcmf_netdev_get_stats
>
> This is a per-network device standard API and it returns per-network device
> statistics. Several releases ago, structure element was added to the network_device
> struct for common usage by drivers. All this patch does is use that instead of the
> private device scratch pad.
>
> If you really want to be more clever, the device should be convert to the
> new getstats64 API, but that requires more work (like wrapping updates with
> the u64_stats_sync() macros.
>

I misunderstood since the first mail I got of this thread is Arend's
reply. But I did receive the "brcmf: make net_device_ops const" patch.
Something wrong with the mail server?

Franky