[Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode"

[Keller, Jacob E]

On 3/21/2022 7:57 AM, Paul Menzel wrote:
> Dear Marcin,
> 
> 
> Am 21.03.22 um 15:47 schrieb Marcin Szycik:
>> This reverts commit bfaaba99e680bf82bf2cbf69866c3f37434ff766.
>>
>> Commit bfaaba99e680 ("ice: Hide bus-info in ethtool for PRs in switchdev
>> mode") was a workaround for lshw tool displaying incorrect
>> descriptions for port representors and PF in switchdev mode. Now the issue
>> has been fixed in the lshw tool itself [1].
>>
>> [1] https://ezix.org/src/pkg/lshw/commit/9bf4e4c9c1
> 
> As you cannot know what lshw version users have installed, I am afraid 
> the workaround (part of Linux 5.16. and 5.17) has to stay in the Linux 
> kernel to not violate Linux’ no-regression policy.
> 
> What are the downsides of keeping the workaround around?
> 


I understand wanting to avoid breaking userspace. However, I think its
important to understand the context here.

lshw was making an incorrect assumption about how netdevs tie to PCI
devices. This assumption caused a bug where representor netdevs would
get mis-named by the utility, and then the real netdev would get a
generic name.

This is akin to a userspace program reading some data reported by kernel
and misinterpreting it. It's simply a bug in the application.

The ice driver was changed to stop reporting the PCI bus info with a
representor. This itself is an actual regression in functionality:
Without this information it isn't possible for userspace to know which
PCI device this representor belongs to.

In particular, if the appropriate information is available and provided,
user space will generally rename these devices:

without linking (i.e. with keeping the workaround:
eth0

with linking (i.e. reverting this work around):
enp24s0f0npf0vf0

The fact that we aren't providing this bus information is to me the
actual regression, and removing this workaround fixes it.

If the workaround did not have such side effects, I would agree that it
would be preferable to keep it. However, I believe that the correct view
of the situation is that the original fix was a regression in behavior,
and that we are fixing that regression by reverting this.

This should be made more explicit in the commit message so that its
clear what we're actually fixing.

> 
> Kind regards,
> 
> Paul
> 
> 
>> Signed-off-by: Marcin Szycik <marcin.szycik at linux.intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> index 24cda7e1f916..476bd1c83c87 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> @@ -190,19 +190,17 @@ __ice_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo,
>>   	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
>>   		 "%x.%02x 0x%x %d.%d.%d", nvm->major, nvm->minor,
>>   		 nvm->eetrack, orom->major, orom->build, orom->patch);
>> +
>> +	strscpy(drvinfo->bus_info, pci_name(pf->pdev),
>> +		sizeof(drvinfo->bus_info));
>>   }
>>   
>>   static void
>>   ice_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo)
>>   {
>>   	struct ice_netdev_priv *np = netdev_priv(netdev);
>> -	struct ice_pf *pf = np->vsi->back;
>>   
>>   	__ice_get_drvinfo(netdev, drvinfo, np->vsi);
>> -
>> -	strscpy(drvinfo->bus_info, pci_name(pf->pdev),
>> -		sizeof(drvinfo->bus_info));
>> -
>>   	drvinfo->n_priv_flags = ICE_PRIV_FLAG_ARRAY_SIZE;
>>   }
>>   
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan