Re: [PATCH v12 5/6] power: reset: Introduce PSCR Recording Framework for Non-Volatile Storage

Hi Oleksij,

sorry for the late reply. I was wondering about this series some time
ago and it was a nice surprise to find a new revision in my inbox.

> Introduce the Power State Change Reasons Recording (PSCRR) framework: a
> generic mechanism to record why the last power state change (shutdown or
> reboot) happened - under-voltage, thermal, watchdog, software-triggered,
> etc. - so a postmortem reason survives a reboot or an abrupt power loss.
> 
> PSCRR is built around providers. A provider is either a hardware reason
> source (a PMIC, SoC reset controller or watchdog exposing a reset cause)
> or a recorder that persists the reason across a power cycle (an NVMEM or
> RTC scratch cell). Each provider gets a directory under
> /sys/kernel/pscrr/providerN/ exposing its name, backing device, the set
> of observed reasons (as tokens), its capabilities, the reasons it
> supports and - for recorders - a record policy. The reason set is
> deliberately not collapsed to a single winning cause, since resets are
> often multi-causal.
> 
> Reasons are the numeric enum psc_reason values from reboot.h, shared with
> the POWER_ON_REASON_* vocabulary, so they store compactly in small
> battery-backed cells. The current reason (get/set_psc_reason(), set by
> the thermal/regulator/hw_protection paths) is written to every recorder
> from the reboot notifier.
> 
> Signed-off-by: Oleksij Rempel <[email protected]>
>

[...]

> +/*----------------------------------------------------------------------*/
> +/* Provider registration */
> +/*----------------------------------------------------------------------*/
> +
> +/**
> + * pscrr_provider_register - register a power state change reason provider
> + * @p: caller-owned provider description
> + *
> + * Creates /sys/kernel/pscrr/providerN/ with "name" and "reason" attributes
> + * and, when @p->dev is set, a "device" symlink. @p->reason is writable when
> + * @p provides write_reason(). The provider must outlive the matching
> + * pscrr_provider_unregister() call.
> + *
> + * Return: 0 on success or a negative errno.
> + */
> +int pscrr_provider_register(struct pscrr_provider *p)
> +{
> +	struct pscrr_provider_dir *dir;
> +	int ret;
> +
> +	if (!p || !p->name || !p->ops || !p->ops->read_reasons)
> +		return -EINVAL;
> +
> +	dir = kzalloc_obj(*dir);
> +	if (!dir)
> +		return -ENOMEM;
> +
> +	dir->provider = p;
> +	dir->policy = PSCRR_RECORD_FIRST;
> +
> +	scoped_guard(mutex, &pscrr_lock) {
> +		if (!pscrr_root) {
> +			kfree(dir);
> +			return -ENODEV;
> +		}

Since the existence of pscrr_root is not protected by the lock, consider
to move this check before the allocation. 

> +
> +		dir->id = ida_alloc(&pscrr_ida, GFP_KERNEL);
> +		if (dir->id < 0) {
> +			ret = dir->id;
> +			kfree(dir);
> +			return ret;
> +		}
> +

Given that ida_alloc() should be safe to call without locking, couldn't
the scoped_guard be limited to the list operation below? Or, if the
goal is to protect against sysfs access during a provider registration,
right before the call to sysfs_create_link()?

> +		ret = kobject_init_and_add(&dir->kobj, &pscrr_dir_ktype,
> +					   pscrr_root, "provider%d", dir->id);
> +		if (ret) {
> +			/*
> +			 * kobject_init_and_add() failed: per its contract only
> +			 * kobject_put() may follow, no kobject_del(). The
> +			 * kobject is not in sysfs, so this is safe under the
> +			 * lock.
> +			 */
> +			ida_free(&pscrr_ida, dir->id);
> +			kobject_put(&dir->kobj);
> +			return ret;
> +		}
> +
> +		if (p->dev) {
> +			ret = sysfs_create_link(&dir->kobj, &p->dev->kobj,
> +						"device");
> +			if (ret)
> +				break;
> +		}
> +
> +		list_add_tail(&dir->node, &pscrr_dirs);
> +		return 0;
> +	}
> +
> +	/*
> +	 * sysfs_create_link() failed after the directory was created: tear it
> +	 * down outside pscrr_lock (kobject_del() drains sysfs stores that take
> +	 * the lock) and release the id only once the directory is gone.
> +	 */
> +	kobject_del(&dir->kobj);
> +	ida_free(&pscrr_ida, dir->id);
> +	kobject_put(&dir->kobj);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pscrr_provider_register);
> +

Regards,

Francesco

-- 
Francesco Valla <[email protected]>