All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode"
@ 2022-03-21 14:47 Marcin Szycik
  2022-03-21 14:57 ` Paul Menzel
  0 siblings, 1 reply; 7+ messages in thread
From: Marcin Szycik @ 2022-03-21 14:47 UTC (permalink / raw)
  To: intel-wired-lan

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

Signed-off-by: Marcin Szycik <marcin.szycik@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;
 }
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode"
  2022-03-21 14:47 [Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode" Marcin Szycik
@ 2022-03-21 14:57 ` Paul Menzel
  2022-03-21 16:14   ` Marcin Szycik
  2022-03-22 18:26   ` Keller, Jacob E
  0 siblings, 2 replies; 7+ messages in thread
From: Paul Menzel @ 2022-03-21 14:57 UTC (permalink / raw)
  To: intel-wired-lan

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?


Kind regards,

Paul


> Signed-off-by: Marcin Szycik <marcin.szycik@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;
>   }
>   

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode"
  2022-03-21 14:57 ` Paul Menzel
@ 2022-03-21 16:14   ` Marcin Szycik
  2022-03-22 18:39     ` Keller, Jacob E
  2022-03-22 18:26   ` Keller, Jacob E
  1 sibling, 1 reply; 7+ messages in thread
From: Marcin Szycik @ 2022-03-21 16:14 UTC (permalink / raw)
  To: intel-wired-lan



On 21-Mar-22 15:57, 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?

The only downside of leaving this workaround is that PCI number won't be shown in ethtool for port representor netdevs. I'm not aware of any applications (other than lshw) that depend on this information, but there might be some.

> 
> 
> Kind regards,
> 
> Paul
> 
> 
>> Signed-off-by: Marcin Szycik <marcin.szycik@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;
>> ? }
>> ? 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode"
  2022-03-21 14:57 ` Paul Menzel
  2022-03-21 16:14   ` Marcin Szycik
@ 2022-03-22 18:26   ` Keller, Jacob E
  2022-03-22 18:54     ` Keller, Jacob E
  1 sibling, 1 reply; 7+ messages in thread
From: Keller, Jacob E @ 2022-03-22 18:26 UTC (permalink / raw)
  To: intel-wired-lan

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@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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode"
  2022-03-21 16:14   ` Marcin Szycik
@ 2022-03-22 18:39     ` Keller, Jacob E
  0 siblings, 0 replies; 7+ messages in thread
From: Keller, Jacob E @ 2022-03-22 18:39 UTC (permalink / raw)
  To: intel-wired-lan

On 3/21/2022 9:14 AM, Marcin Szycik wrote:
> 
> 
> On 21-Mar-22 15:57, 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?
> 
> The only downside of leaving this workaround is that PCI number won't be shown in ethtool for port representor netdevs. I'm not aware of any applications (other than lshw) that depend on this information, but there might be some.
> 

That's not quite correct. If you don't include this information, then at
least systemd based distributions won't generate the expected altname.

If we keep the workaround, these netdevs remain with their default
generic ethX device names. (eth0, eth1, etc).

If we remove the workaround, the netdevs get altnamed using a scheme
which matches the existing PF netdev schemes like eno0, enp24s0f0 etc.

For example, the netdev will get an altname such as enp24s0f0npf0vf0

This indicates that the netdev is a representor for VF 0 on the
enp24s0f0 main netdev.

Thanks,
Jake

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode"
  2022-03-22 18:26   ` Keller, Jacob E
@ 2022-03-22 18:54     ` Keller, Jacob E
  2022-03-24 14:51       ` Marcin Szycik
  0 siblings, 1 reply; 7+ messages in thread
From: Keller, Jacob E @ 2022-03-22 18:54 UTC (permalink / raw)
  To: intel-wired-lan

On 3/22/2022 11:26 AM, Keller, Jacob E wrote:
> 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.
> 

I apologize. I was somewhat confused as I had thought this patch
included a change to also call SET_NETDEV_DEV as well. That is required
in order for the netdev renaming to work properly. This was being
discussed internally and I mixed up the patches.

However, doing that breaks lshw in the same way that reverting this
workaround would. If we don't include SET_NETDEV_DEV, we end up without
the nice representor names.

So the way I see it:

We discovered the bug in lshw, and had a workaround in ice (which
arguably we shouldn't have done at all, but alas its been done). This
workaround removes the bus information from ethtool and this hides the
fact that the representor device is linked to that PCI device.

We also haven't yet called SET_NETDEV_DEV, which also links the
netdevice to the PCI device.

The same bug in lshw causes it to mis-assign the name of the devices
regardless of whether we include that info from ethtool or from
SET_NETDEV_DEV.

However, if we *don't* include SET_NETDEV_DEV in the driver, we end up
with the situation I described above: we get poor device names instead
of the more useful altnames scheme which aligns with the PF name scheme
and similar.

I believe lack of SET_NETDEV_REG is a more significant regression and
lack of functionality than a minor display bug in lshw that is already
fixed.

Thus, I think we should remove this workaround *and* include the fix to
call SET_NETEDEV_DEV.

We may need to get some opinion on other vendors and implementations.
From my understanding other device vendors call SET_NETDEV_DEV for their
representors already.

I hope this clarifies my statements above and removes the confusion.

Perhaps we should submit both changes together in a series with this
context in the commit message?

>>
>> Kind regards,
>>
>> Paul
>>
>>
>>> Signed-off-by: Marcin Szycik <marcin.szycik@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
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode"
  2022-03-22 18:54     ` Keller, Jacob E
@ 2022-03-24 14:51       ` Marcin Szycik
  0 siblings, 0 replies; 7+ messages in thread
From: Marcin Szycik @ 2022-03-24 14:51 UTC (permalink / raw)
  To: intel-wired-lan



On 22-Mar-22 19:54, Keller, Jacob E wrote:
> On 3/22/2022 11:26 AM, Keller, Jacob E wrote:
>> 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.
>>
> 
> I apologize. I was somewhat confused as I had thought this patch
> included a change to also call SET_NETDEV_DEV as well. That is required
> in order for the netdev renaming to work properly. This was being
> discussed internally and I mixed up the patches.
> 
> However, doing that breaks lshw in the same way that reverting this
> workaround would. If we don't include SET_NETDEV_DEV, we end up without
> the nice representor names.
> 
> So the way I see it:
> 
> We discovered the bug in lshw, and had a workaround in ice (which
> arguably we shouldn't have done at all, but alas its been done). This
> workaround removes the bus information from ethtool and this hides the
> fact that the representor device is linked to that PCI device.
> 
> We also haven't yet called SET_NETDEV_DEV, which also links the
> netdevice to the PCI device.
> 
> The same bug in lshw causes it to mis-assign the name of the devices
> regardless of whether we include that info from ethtool or from
> SET_NETDEV_DEV.
> 
> However, if we *don't* include SET_NETDEV_DEV in the driver, we end up
> with the situation I described above: we get poor device names instead
> of the more useful altnames scheme which aligns with the PF name scheme
> and similar.
> 
> I believe lack of SET_NETDEV_REG is a more significant regression and
> lack of functionality than a minor display bug in lshw that is already
> fixed.
> 
> Thus, I think we should remove this workaround *and* include the fix to
> call SET_NETEDEV_DEV.
> 
> We may need to get some opinion on other vendors and implementations.
> From my understanding other device vendors call SET_NETDEV_DEV for their
> representors already.
> 
> I hope this clarifies my statements above and removes the confusion.
> 
> Perhaps we should submit both changes together in a series with this
> context in the commit message?

Ok, I will take this patch and SET_NETDEV_DEV patch and submit both of them in a mini-patchset, with detailed explanation and examples.

> 
>>>
>>> Kind regards,
>>>
>>> Paul
>>>
>>>
>>>> Signed-off-by: Marcin Szycik <marcin.szycik@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
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan at osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-03-24 14:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-21 14:47 [Intel-wired-lan] [PATCH net-next] Revert "ice: Hide bus-info in ethtool for PRs in switchdev mode" Marcin Szycik
2022-03-21 14:57 ` Paul Menzel
2022-03-21 16:14   ` Marcin Szycik
2022-03-22 18:39     ` Keller, Jacob E
2022-03-22 18:26   ` Keller, Jacob E
2022-03-22 18:54     ` Keller, Jacob E
2022-03-24 14:51       ` Marcin Szycik

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.