linux-kernel - Re: [PATCH v3 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aFF7qqlexxh540FW@pollux>
Date: Tue, 17 Jun 2025 16:28:58 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Drew Fustini <drew@...7.com>, Guo Ren <guoren@...nel.org>,
	Fu Wei <wefu@...hat.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Benno Lossin <lossin@...nel.org>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org,
	linux-pwm@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-clk@...r.kernel.org
Subject: Re: [PATCH v3 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC

On Tue, Jun 17, 2025 at 04:07:27PM +0200, Michal Wilczynski wrote:
> +    fn write_waveform(
> +        chip: &mut pwm::Chip,
> +        pwm: &mut pwm::Device,

I think you can't hand out mutable references here. This would allow things like
mem::swap(), which I think are not valid on those structures.

> +        wfhw: &Self::WfHw,
> +        parent_dev: &Device<Bound>,
> +    ) -> Result {
> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
> +        let hwpwm = pwm.hwpwm();
> +        let iomem_guard = data.iomem.access(parent_dev)?;

Technically, this isn't a guard, hence would't call it that way.

> +        let iomap = iomem_guard.deref();
> +        let was_enabled = pwm.state().enabled();
> +
> +        if !wfhw.enabled {
> +            if was_enabled {
> +                iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
> +                iomap.try_write32(0, th1520_pwm_fp(hwpwm))?;
> +                iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
> +            }
> +            return Ok(());
> +        }
> +
> +        iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
> +        iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?;
> +        iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?;
> +        iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;

None of the offsets are known at compile time? :(

> +
> +        // The `PWM_START` bit must be written in a separate, final transaction, and
> +        // only when enabling the channel from a disabled state.
> +        if !was_enabled {
> +            iomap.try_write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm))?;
> +        }
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "PWM-{}: Wrote (per: {}, duty: {})",
> +            hwpwm,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +        );
> +
> +        Ok(())
> +    }
> +}

<snip>

> +impl platform::Driver for Th1520PwmPlatformDriver {
> +    type IdInfo = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    fn probe(
> +        pdev: &platform::Device<Core>,
> +        _id_info: Option<&Self::IdInfo>,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        let dev = pdev.as_ref();
> +        let resource = pdev.resource(0).ok_or(ENODEV)?;
> +        let iomem = pdev.ioremap_resource_sized::<TH1520_PWM_REG_SIZE>(resource)?;
> +        let clk = Clk::get(pdev.as_ref(), None)?;
> +
> +        clk.prepare_enable()?;
> +
> +        // TODO: Get exclusive ownership of the clock to prevent rate changes.
> +        // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available.
> +        // This should be updated once it is implemented.
> +        let rate_hz = clk.rate().as_hz();
> +        if rate_hz == 0 {
> +            dev_err!(dev, "Clock rate is zero\n");
> +            return Err(EINVAL);
> +        }
> +
> +        if rate_hz > time::NSEC_PER_SEC as usize {
> +            dev_err!(
> +                dev,
> +                "Clock rate {} Hz is too high, not supported.\n",
> +                rate_hz
> +            );
> +            return Err(ERANGE);
> +        }
> +
> +        let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?;
> +
> +        let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
> +        chip.set_drvdata(drvdata);
> +
> +        let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?;
> +
> +        Ok(KBox::new(
> +            Th1520PwmPlatformDriver {
> +                _registration: registration,
> +            },
> +            GFP_KERNEL,
> +        )?
> +        .into())

Here you are setting up the registration for the correct lifetime, however
drivers could extend the lifetime of the registration arbitrarily, which would
break the guarantee of the &Device<Bound> we rely on in the callbacks above
(e.g. write_waveform()).

Hence, pwm::Registration's lifetime has to be controlled by Devres. I'll also
add a corresponding comment in your registration patch.

> +    }
> +}