Re: [PATCH net-next v13 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] build: undefined macro kzalloc_obj causes compilation failure
- [Low] locking: sit9531x_chan_state_fetch called without required multiop_lock
--

commit 5f5cba1ccd97d6786f10dd67578b1b35e4e746e8
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>
Subject: dpll: sit9531x: register DPLL devices and pins

This commit registers SiT9531x PLLs as DPLL devices and exposes their inputs
and outputs as DPLL pins. It implements device and pin structures, initial
hardware state fetching, and periodic polling for state 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
[ ... ]
> @@ -934,22 +934,17 @@
>  	for (i = 0; i < sitdev->info->num_outputs; i++) {
>  		/*
>  		 * Nothing runs concurrently before registration, but the
>  		 * output fetch walks paged registers under the same contract
>  		 * as at runtime, so it holds the lock the same way.
>  		 */
>  		mutex_lock(&sitdev->multiop_lock);
>  		rc = sit9531x_out_state_fetch(sitdev, i);
>  		mutex_unlock(&sitdev->multiop_lock);
>  		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]
sit9531x_chan_state_fetch() explicitly invokes sit9531x_prio_table_read(),
which is documented as requiring the caller to hold sitdev->multiop_lock
to protect paged I2C register accesses.

Is it intentional to omit the lock acquisition here? The adjacent loops in
this initialization sequence acquire the lock before calling the state
fetchers.

>  		if (rc) {
>  			dev_err(sitdev->dev,
>  				"Failed to fetch PLL%c state: %d\n",
>  				'A' + i, rc);
>  			return rc;
>  		}
>  	}
>  
>  	return 0;
>  }
[ ... ]
> @@ -1338,15 +1338,15 @@
>  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]
Is kzalloc_obj defined somewhere in the tree?

It does not appear to exist in the standard kernel API, which will result
in a build failure during compilation. Should this be replaced with
kzalloc(sizeof(*pin), GFP_KERNEL)?

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

[Severity: Low]
Similarly here, the undefined kzalloc_obj macro is used. Can this be updated
to kzalloc(sizeof(*sitdpll), GFP_KERNEL) to avoid compilation failures?

>  	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/20261013224131.27149-1-arouhi@sitime.com?part=5