On Wed, May 13, 2020 at 02:49:48PM +0200, Jan Tluka wrote:
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.
Yeah, good point, I didn't realize that the others used this approach, I
think all of these subconfig classes should have both the config and the
deconfig methods implemented and they should be running with every
subconfig loop iteration...
>
>-Jan
>
More thoughts.
Why does CoalescingHWConfigMixin save the coalescing status in the
Device class rather than doing this through hw_config and hw_deconfig?
Not sure what you mean by that, CoalescingHWConfigMixin class uses the
_configure_dev_attribute() method which also stores the original value
in the config object for the Recipe.
It's also stored in the Device class so that the Slave can do cleanup
after a recipe is finished in case the recipe didn't do proper cleanup
(either due to incorrect implementation or due to a crash).
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()
yes, I think that's good.
-Ondrej