Wed, May 13, 2020 at 01:59:25PM CEST, jtluka(a)redhat.com wrote:
Wed, May 13, 2020 at 01:42:23PM CEST, jtluka(a)redhat.com wrote:
>Wed, May 13, 2020 at 11:51:45AM CEST, olichtne(a)redhat.com wrote:
>>On Thu, May 07, 2020 at 11:05:07AM +0200, Jan Tluka wrote:
>>> - def hw_deconfig(self, config):
>>> - for dev in self.no_pause_frames_dev_list:
>>> - dev.host.run("ethtool -A {} rx on tx
on".format(dev.name))
>>> -
>>> - super().hw_deconfig(config)
>>
>>I'm not sure about removing this method. I assume you removed it because
>>the previous patch implements pause frame configuration in the Device
>>class such that it will be deconfigured with the Device deconfiguration?
>>
>>But, this is a SubConfig mixin class which means that theoretically you
>>could have multiple sub config loop iterations where each could define
>>it's own configuration for pause frames. Even though the value will
>>always be overwritten I think it may make more sense to have it in the
>>"comple" form of running both configuration and deconfiguration for
this
>>setting.
>>
>>What do you think?
>>
>
>I think it should not be removed. I can't remember why I'd want to
>remove this. I'll think about this more, but likely I will send a v3
>patchset.
>
So, I think I removed it based on the similar approach of
CoalescingHWConfigMixin class. This (and few others) do not have it
defined either.
Likely the other classes should be fixed, too.
-Jan
More thoughts.
Why does CoalescingHWConfigMixin save the coalescing status in the
Device class rather than doing this through hw_config and hw_deconfig?
With the current implementation the deconfig part behaves more like
a Testwide deconfiguration rather than Sub deconfiguration.
So, this should be fixed as well.
Saving a device feature status is ok, but each hw_deconfig should
call:
dev.restore_feature()
...
super().hw_deconfig()
-Jan