Re: [PATCH net-next v12 05/12] dpll: sit9531x: register DPLL devices and pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] locking: Missing multiop_lock in sit9531x_dev_state_fetch() violating function contracts
- [Low] build: Use of undefined kzalloc_obj macro causes compilation failure
--

commit 07998476897b60ae6e981144b634fb6008fd348c
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: register DPLL devices and pins

This commit registers each of the four on-chip PLLs as a DPLL device and
exposes their respective input and output clocks as DPLL pins. It adds
initialization for the device hardware state, pin ops mapping, and a
periodic kthread worker to poll hardware status and notify the core of
changes.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
>  static int sit9531x_dev_state_fetch(struct sit9531x_dev *sitdev)
>  {
[ ... ]
>  	/*
>  	 * The priority-table read walks the Page-1 registers, so it runs
>  	 * with multiop_lock held like every other multi-register sequence.
>  	 * Nothing can race with it here -- the DPLLs are not registered and
>  	 * the monitor is not running yet -- but the page handling stays
>  	 * serialized the same way as at runtime.
>  	 */
>  	mutex_lock(&sitdev->multiop_lock);
>  	rc = sit9531x_ref_pll_mask_fetch(sitdev);
>  	mutex_unlock(&sitdev->multiop_lock);
>  	if (rc) {
>  		dev_err(sitdev->dev,
>  			"Failed to fetch input priority tables: %d\n", rc);
>  		return rc;
>  	}
>  
>  	for (i = 0; i < sitdev->info->num_outputs; i++) {
>  		rc = sit9531x_out_state_fetch(sitdev, i);

[Severity: Low]
Does this code violate the internal locking contract?

The function sit9531x_out_state_fetch() calls sit9531x_output_mode_fetch(),
which documents that the caller must hold sitdev->multiop_lock because it
accesses multiple paged registers. While this is called during probe when
concurrent threads might not be active yet, the lock is explicitly dropped
just before this loop.

>  		if (rc) {
>  			dev_err(sitdev->dev,
>  				"Failed to fetch output %u state: %d\n", i, rc);
>  			return rc;
>  		}
>  	}
>  
>  	for (i = 0; i < SIT9531X_NUM_PLLS; i++) {
>  		rc = sit9531x_chan_state_fetch(sitdev, i);

[Severity: Low]
Can this lead to paged register desynchronization?

Similarly, sit9531x_chan_state_fetch() calls sit9531x_prio_table_read(),
which also requires multiop_lock to serialize page accesses. Dropping the
lock before these state fetches breaks the driver's documented concurrency
model for register pages.

>  		if (rc) {
>  			dev_err(sitdev->dev,
>  				"Failed to fetch PLL%c state: %d\n",
>  				'A' + i, rc);
>  			return rc;
>  		}
>  	}
>  
>  	return 0;
>  }
[ ... ]
>  static struct sit9531x_dpll_pin *
>  sit9531x_dpll_pin_alloc(struct sit9531x_dpll *sitdpll,
>  			enum dpll_pin_direction dir, u8 id)
>  {
>  	struct sit9531x_dpll_pin *pin;
>  
>  	pin = kzalloc_obj(*pin, GFP_KERNEL);

[Severity: Low]
Will this compile successfully?

The kzalloc_obj macro used here receives two arguments. However,
kzalloc_obj is not a standard kernel macro (the standard is kzalloc), and
where it is used internally in the DPLL core, it only takes a single
argument. This looks like a syntax error that will fail to build. Should
this use kzalloc() instead?

>  	if (!pin)
>  		return ERR_PTR(-ENOMEM);
>  
>  	pin->dpll = sitdpll;
>  	pin->dir = dir;
>  	pin->id = id;
>  
>  	return pin;
>  }
[ ... ]
>  struct sit9531x_dpll *sit9531x_dpll_alloc(struct sit9531x_dev *sitdev, u8 ch)
>  {
>  	struct sit9531x_dpll *sitdpll;
>  
>  	sitdpll = kzalloc_obj(*sitdpll, GFP_KERNEL);

[Severity: Low]
The same compilation issue with kzalloc_obj() appears here.

>  	if (!sitdpll)
>  		return ERR_PTR(-ENOMEM);
>  
>  	sitdpll->dev = sitdev;
>  	sitdpll->id = ch;
>  	sitdpll->lock_status = DPLL_LOCK_STATUS_UNLOCKED;
>  	sitdpll->lock_status_error = DPLL_LOCK_STATUS_ERROR_NONE;
>  	INIT_LIST_HEAD(&sitdpll->pins);
>  
>  	return sitdpll;
>  }

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