Re: [PATCH v16 1/7] spi: pxa2xx: introduce clock enable and disable helper functions

Hi Andy,

  Thanks for the detailed review comments.

On Tue, Jul 21, 2026 at 3:22 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Jul 21, 2026 at 12:21:10AM +0800, Shih-Yuan Lee wrote:
> > The driver disables the clock during PM runtime suspend, PM system
> > suspend, and device unbinding (remove). It also disables the clock
> > on various error unwinding paths in pxa2xx_spi_probe().
> >
> > However, if the clock is already disabled (for example, if the device is
> > already runtime-suspended during driver unbinding), calling the common
> > clock framework's clk_disable_unprepare() again leads to clock prepare/enable
> > count underflows, generating kernel warnings.
>
> Any real life example here?
Yes, a concrete real-world scenario occurs during sysfs driver
unbinding while the device is in PM runtime suspend:

  1. When the system is idle, PM runtime suspends the controller,
disabling the clock (clk_disable_unprepare()).
  2. If the user unbinds the driver via sysfs:
   echo 0000:00:15.4 > /sys/bus/pci/drivers/pxa2xx_spi_pci/unbind
  pxa2xx_spi_remove() is invoked.
  3. Without clk_enabled tracking, pxa2xx_spi_remove() unconditionally
calls clk_disable_unprepare() a second time on the already disabled
clock.
  4. This triggers a Common Clock Framework (CCF) underflow warning:
  WARN_ON(clk->enable_count == 0)

A similar underflow occurs if pxa2xx_spi_probe() fails and executes
error unwinding after disabling the clock.
>
> > Introduce pxa2xx_spi_clk_enable() and pxa2xx_spi_clk_disable() helper
> > functions that track the clock enable state using a new 'clk_enabled'
> > boolean flag in struct driver_data.
> >
> > This ensures clk_disable_unprepare() is called only when the clock is
> > active, preventing clock underflows during suspend transitions and unbind.
> > It also allows the probe function to safely unwind resource allocations
> > without triggering clock underflows.
>
> ...
>
> Same issue, I have no cover letter in my mailbox. Please, slow down and check
> your email setup. Something is wrong.
Duly noted. I will check my git send-email configuration and ensure the
cover letter is properly sent as the thread root.
>
> ...
>
> > +static int pxa2xx_spi_clk_enable(struct driver_data *drv_data)
> > +{
> > +     int ret;
> > +
> > +     if (drv_data->clk_enabled)
> > +             return 0;
> > +
> > +     ret = clk_prepare_enable(drv_data->ssp->clk);
>
> > +     if (ret == 0)
> > +             drv_data->clk_enabled = true;
> > +
> > +     return ret;
>
> Use usual pattern
>
>         if (ret)
>                 return ret;
>
>         ...
>         return 0;
>
> > +}
Will update in the next version to follow the standard guard pattern:

    static int pxa2xx_spi_clk_enable(struct driver_data *drv_data)
    {
        int ret;

        if (drv_data->clk_enabled)
            return 0;

        ret = clk_prepare_enable(drv_data->ssp->clk);
        if (ret)
            return ret;

        drv_data->clk_enabled = true;
        return 0;
    }
>
> > +static void pxa2xx_spi_clk_disable(struct driver_data *drv_data)
> > +{
> > +     if (drv_data->clk_enabled) {
>
>         if (!drv_data->clk_enabled)
>                 return;
>
> > +             clk_disable_unprepare(drv_data->ssp->clk);
> > +             drv_data->clk_enabled = false;
> > +     }
> > +}
Will update in the next version to use early return:

    static void pxa2xx_spi_clk_disable(struct driver_data *drv_data)
    {
        if (!drv_data->clk_enabled)
            return;

        drv_data->clk_enabled = false;
        clk_disable_unprepare(drv_data->ssp->clk);
    }
>
> ...
>
> > struct driver_data {
>
> >       void __iomem *lpss_base;
> >
> > +     bool clk_enabled;
>
> How is this protected against simultaneously called enable/disable on different
> CPUs?
pxa2xx_spi_clk_enable() and pxa2xx_spi_clk_disable() are only invoked
within serialized contexts:

  1. Driver probe() / remove() paths (serialized by bus locks).
  2. PM Runtime callbacks (serialized by dev->power.lock in PM core).
  3. System suspend / resume callbacks (serialized by PM subsystem).

They are never called concurrently from asynchronous contexts or worker threads.
>
> >       /* Optional slave FIFO ready signal */
> >       struct gpio_desc *gpiod_ready;
> >  };
>
> Whenever you add the field, check with `pahole` that the layout is optimal.
Thanks for pointing out pahole. Checked with pahole: bool clk_enabled;
is placed immediately after bool suspended; in struct driver_data.
Since both are 1-byte booleans, clk_enabled fits into the existing
struct padding byte, resulting in zero additional hole padding and keeping
the total struct size unchanged.

Thanks,
Shih-Yuan