Re: [PATCH net-next v13 04/12] dpll: sit9531x: read DPLL types and pin properties from system firmware

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dpll: state leak on error path for XO pin
--

commit 1b38c14d1669809a7ef952760c8da64d9afcc03c
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: read DPLL types and pin properties from system firmware
    
The DPLL core wants a type per device and a property set per pin: a
label, the direction, the capabilities, and the frequencies the pin
supports. None of that can be read from the chip, so they come
from the firmware node.

> diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c
> new file mode 100644
> index 0000000000000..3635ea497e415
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/prop.c

[ ... ]

> +struct sit9531x_pin_props *
> +sit9531x_pin_props_get(struct sit9531x_dev *sitdev,
> +		       enum dpll_pin_direction dir, u8 index)
> +{

[ ... ]

> +	} else if (dir == DPLL_PIN_DIRECTION_INPUT &&
> +		   index == SIT9531X_MAX_INPUTS) {
> +		/* The XO reference is fixed: no state or priority control. */
> +		props->dpll_props.type = DPLL_PIN_TYPE_INT_OSCILLATOR;
> +		props->dpll_props.capabilities = 0;
> +		sitdev->ref[index].freq = sitdev->xtal_freq;

[Severity: Low]
Does this code leak state if an allocation fails later in the function?

The comment later in sit9531x_pin_props_get() explicitly states that the
allocation for ranges is the last thing that can fail, and a call that reports
failure must not leave the device's cached rate changed behind it.

However, if the allocation for ranges fails, the function returns -ENOMEM
and leaves sitdev->ref[index].freq permanently modified for the XO pin.

Should the update to sitdev->ref[index].freq be deferred to the end of the
function here as well, rather than doing it early in this block?

> +		curr_freq = sitdev->xtal_freq;
> +	} else if (dir == DPLL_PIN_DIRECTION_INPUT) {

[ ... ]

> +	ranges = kcalloc(num_freqs + 2, sizeof(*ranges), GFP_KERNEL);
> +	if (!ranges) {
> +		rc = -ENOMEM;
> +		goto err_alloc_ranges;
> +	}
> +
> +	/*
> +	 * Publish the seeded rate only once the pin is certain to be built.
> +	 * The allocation above is the last thing that can fail, and a call
> +	 * that reports failure must not leave the device's cached rate
> +	 * changed behind it.
> +	 */
> +	if (curr_freq) {
> +		if (dir == DPLL_PIN_DIRECTION_INPUT)
> +			sitdev->ref[index].freq = curr_freq;
> +		else
> +			sitdev->out[index].freq = curr_freq;
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013224131.27149-1-arouhi@sitime.com?part=4