Re: [PATCH net-next v11 7/7] selftests: netconsole: validate target resume

[Matthieu Baerts <matttbe@kernel.org>]

Hi Andre,

Thank you for your reply!

On 08/07/2026 22:24, Andre Carvalho wrote:
> Hello Matthieu,
> 
> On Wed, Jul 08, 2026 at 05:50:53PM +0200, Matthieu Baerts wrote:
>>
>>> +function trigger_reactivation() {
>>> +	# Add back low level module
>>> +	modprobe netdevsim
>>> +	# Recreate namespace and two interfaces
>>> +	set_network
>>> +	# Restore MACs
>>> +	ip netns exec "${NAMESPACE}" ip link set "${DSTIF}" \
>>> +		address "${SAVED_DSTMAC}"
>>> +	if [ "${BINDMODE}" == "mac" ]; then
>>> +		ip link set dev "${SRCIF}" down
>>> +		ip link set dev "${SRCIF}" address "${SAVED_SRCMAC}"
>>> +		# Rename device in order to trigger target resume, as initial
>>> +		# when device was recreated it didn't have correct mac address.
>>> +		ip link set dev "${SRCIF}" name "${TARGET}"
>>
>> When I execute the test, the "ifname" bind mode works without issues,
>> but the "mac" one not. From what I see, the socat process doesn't get
>> any UDP packet when expected. I wonder if the problem might not come
>> from here: the interface is disabled before changing the MAC address and
>> renaming the interface, but not re-enabled at the end. Is it normal?
> 
> Yes, the expectation is that the target would be automatically re-enabled by
> netconsole. That is why we don't 'up' the interface here explicitly.

OK, that's what I thought when I looked at the previous commit of this
series, but it was still feeling "strange" :)

> The problem is that the test has an implict dependency on interface mac address
> changing when we recreate them, which is why we have to do this whole down/update/rename
> flow to trigger the reactivation in this case.

OK!

>> If I add 'up' at the end of this last line here, or if I remove the
>> whole if-statement block, the test passes.
> 
> For cases where the mac is persistent (e.g with systemd MACAddressPolicy=persistent),
> the actual re-enablement should "just work", which I suspect is why it works when
> you remove the whole if-statement block.

I see, I just noticed the discussion on lore about that on the previous
patch and on the v10. I now understand why MACAddressPolicy should be
set to 'none', and I can do that on my side.

> I think to make the test more robust to such scenarios, we should skip the
> if-statement block when we detect that the interface came back with the same mac
> as previously. Something like this:
> 
> if [ "${BINDMODE}" == "mac" ]; then
> 	CURR_SRCMAC=$(mac_get "${SRCIF}")
> 	if [ "${CURR_SRCMAC}" == "${SAVED_SRCMAC}" ]; then
> 		# Interface came back with same mac, no need to restore
> 		return
> 	fi
> 	ip link set dev "${SRCIF}" down
> 	ip link set dev "${SRCIF}" address "${SAVED_SRCMAC}"
> 	# Rename device in order to trigger target resume, as initial
> 	# when device was recreated it didn't have correct mac address.
> 	ip link set dev "${SRCIF}" name "${TARGET}"
> fi
> 
> Could you confirm if this fixes the selftest in your environment?

Yes, it fixes the selftest (without MACAddressPolicy=none), thank you!

And maybe this could be used to avoid the 'return':

  if [ "${BINDMODE}" == "mac" ] &&
     [ "$(mac_get "${SRCIF}")" != "${SAVED_SRCMAC}" ]; then

A detail, up to you :)

I think it would be interesting to have such check. If the MAC policy
can cause issues here, it might be good to also add a warning, or to
continue to fail, but with an explicit error message to look at this.

>>> +	echo "Running with bind mode: ${BINDMODE}" >&2
>>> +	# Set current loglevel to KERN_INFO(6), and default to KERN_NOTICE(5)
>>> +	echo "6 5" > /proc/sys/kernel/printk
>>
>> Can we remove this? Without that, it is hard to understand what went
>> wrong in case of issues.
> 
> Can you elaborate? This is typical for netconsole tests and I think required to
> ensure the write will show up on netconsole. I'm not opposed to changing it if
> there is a reason, but we probably want to make it consistent with other tests too.

I understand, and I asked because I was not sure if this was needed for
some tests. If it doesn't break some tests, I think it would be good not
to change the loglevel, because on my side, all I had was:

  # Running with bind mode: mac
  not ok 1 selftests: drivers/net: netcons_resume.sh # exit=1

Even with the suggested modification to have "FAIL: File was not
generated.", that's not helpful enough to understand where the problem
came from. Having the extra info in the console was really helpful, but
it was not obvious I could do that.

There could be a message saying "comment this to have more debug", but
that means a CI will not get such useful debug info in case of error. So
I think this behaviour should be changed. Or at least printing debug
info only in case of errors.

>>> +	# Send the message
>>> +	echo "${MSG}: ${TARGET}" > /dev/kmsg
>>> +	# Wait until socat saves the file to disk
>>> +	busywait "${BUSYWAIT_TIMEOUT}" test -s "${OUTPUT_FILE}"
>>
>> In my case, the script was stopping here, without any message. I can
>> send a patch adding "|| true" to go to the next instruction, and display
>> "FAIL: File was not generated.".
>>
> 
> This seems reasonable to me.

Good! I will send this patch.

Just to avoid a deadlock, do you plan to send one to handle the MAC
address that has not been modified (and eventually one to get more debug
messages in case of errors)?

Cheers,
Matt