Re: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

[Jakub Kicinski <kuba@kernel.org>]

On Sun, 6 Sep 2020 13:47:13 +0300 Shay Agroskin wrote:
> Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:
> > I don't want to stir up the pot, but do you really need the 
> > offsetof() of
> > each member in the stats struct? Couldn't you piggyback on 
> > assumption that
> > these stats need to be u64 and just walk the struct with 
> > pointer?
> >
> > 	struct ena_ring *ring;
> > 	int offset;
> > 	int i, j;
> > 	u8 *ptr;
> >
> > 	for (i = 0; i < adapter->num_io_queues; i++) {
> > 		/* Tx stats */
> > 		ring = &adapter->tx_ring[i];
> > 		ptr = (u8 *)&ring->tx_stats;
> >
> > 		for (j = 0; j < ENA_STATS_ARRAY_TX; j++) {
> > 			ena_safe_update_stat((u64 *)ptr, 
> > (*data)++, &ring->syncp);
> > 			ptr += sizeof(u64);
> > 		}
> > 	}
> >
> > I find this as a simpler and lighter solution. There might be 
> > issues with
> > code typed in email client, but you get the idea.
> >  
> >> 
> >> of course we need to convert the stat_offset field to be in 8 
> >> bytes resolution instead.
> >> 
> >> This approach has a potential bug hidden in it. If in the 
> >> future
> >> someone decides to expand the "ena_stats_tx" struct and add a 
> >> field preceding cnt,
> >> cnt will no longer be the beginning of the struct, which will 
> >> cause a bug."
> >> 
> >> Therefore, if you have another way to do this, please share 
> >> it. Otherwise I'd
> >> rather leave this code as it is for the sake of robustness.
> >>   
> >> > 
> >> >      Andrew  
> 
> Hi all,
> 
> We tried to implement your suggestion, and found that removing the 
> stat_offset
> field causes problems that are challenging to solve.
> Removing stat_offset introduces a requirement that the statistics 
> in a stat
> strings array (check [1] for example) and stat variables struct 
> (check [2] for
> example) must be in the same order.
> This requirement is prone to future bugs that might be challenging 
> to locate.
> We also tried to unify the array and struct creation by
> using X macros. At the moment this change requires more time and 
> effort by us
> and our customers need this code merged asap.
> 
> [1] https://elixir.bootlin.com/linux/v5.9-
> rc3/source/drivers/net/ethernet/amazon/ena/ena_ethtool.c#L71
> [2] https://elixir.bootlin.com/linux/v5.9-
> rc3/source/drivers/net/ethernet/amazon/ena/ena_netdev.h#L232
> 
> (This message was sent before but didn't seem to get into the 
> mailing list. Apologies if you got it twice)

Divide the offset by 8, cast &ring->tx_stats to u64 * (without
referencing cnt). That should be fine.