Re: [PATCH net-next 2/4] netconsole: Add option to auto-populate CPU number in userdata

[Jakub Kicinski <kuba@kernel.org>]

Sorry for the late review, I think this will miss v6.13 :(

On Wed, 13 Nov 2024 07:10:53 -0800 Breno Leitao wrote:
>  /**
>   * struct netconsole_target - Represents a configured netconsole target.
>   * @list:	Links this target into the target_list.
> @@ -97,6 +105,7 @@ static struct console netconsole_ext;
>   * @userdata_group:	Links to the userdata configfs hierarchy
>   * @userdata_complete:	Cached, formatted string of append
>   * @userdata_length:	String length of userdata_complete
> + * @userdata_auto:	Kernel auto-populated bitwise fields in userdata.
>   * @enabled:	On / off knob to enable / disable target.
>   *		Visible from userspace (read-write).
>   *		We maintain a strict 1:1 correspondence between this and
> @@ -123,6 +132,7 @@ struct netconsole_target {
>  	struct config_group	userdata_group;
>  	char userdata_complete[MAX_USERDATA_ENTRY_LENGTH * MAX_USERDATA_ITEMS];
>  	size_t			userdata_length;
> +	enum userdata_auto	userdata_auto;

If you want to set multiple bits here type should probably be unsigned
long. Otherwise the enum will contain combination of its values, which
are in themselves not valid enum values ... if that makes sense.

>  #endif
>  	bool			enabled;
>  	bool			extended;

> +	/* Check if CPU NR should be populated, and append it to the user
> +	 * dictionary.
> +	 */
> +	if (child_count < MAX_USERDATA_ITEMS && nt->userdata_auto & AUTO_CPU_NR)
> +		scnprintf(&nt->userdata_complete[complete_idx],
> +			  MAX_USERDATA_ENTRY_LENGTH, " cpu=%u\n",
> +			  raw_smp_processor_id());

I guess it may be tricky for backward compat, but shouldn't we return
an error rather than silently skip?

>  	nt->userdata_length = strnlen(nt->userdata_complete,
>  				      sizeof(nt->userdata_complete));
>  }
> @@ -757,7 +788,36 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
>  	return ret;
>  }
>  
> +static ssize_t populate_cpu_nr_store(struct config_item *item, const char *buf,
> +				     size_t count)
> +{
> +	struct netconsole_target *nt = to_target(item->ci_parent);
> +	bool cpu_nr_enabled;
> +	ssize_t ret;
> +
> +	if (!nt)
> +		return -EINVAL;

Can this happen? Only if function gets called with a NULL @item
which would be pretty nutty.

> +	mutex_lock(&dynamic_netconsole_mutex);
> +	ret = kstrtobool(buf, &cpu_nr_enabled);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (cpu_nr_enabled)
> +		nt->userdata_auto |= AUTO_CPU_NR;
> +	else
> +		nt->userdata_auto &= ~AUTO_CPU_NR;
> +
> +	update_userdata(nt);
> +
> +	ret = strnlen(buf, count);
> +out_unlock:
> +	mutex_unlock(&dynamic_netconsole_mutex);
> +	return ret;
> +}