| 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
| ||
|
Message-ID: <20210423172041.jwmrrbnv76irmzxz@pengutronix.de>
Date: Fri, 23 Apr 2021 19:20:41 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Thierry Reding <thierry.reding@...il.com>
Cc: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>,
Rob Herring <robh+dt@...nel.org>,
Lee Jones <lee.jones@...aro.org>, devicetree@...r.kernel.org,
linux-pwm@...r.kernel.org, punit1.agrawal@...hiba.co.jp,
yuji2.ishikawa@...hiba.co.jp, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM
support
On Fri, Apr 23, 2021 at 07:05:35PM +0200, Thierry Reding wrote:
> On Mon, Apr 19, 2021 at 09:00:05AM +0900, Nobuhiro Iwamatsu wrote:
> > Hi,
> >
> > This series is the PWM driver for Toshiba's ARM SoC, Visconti[0].
> > This provides DT binding documentation and device driver.
> >
> > [0]: https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-recognition-processors-visconti.html
> >
> > Updates:
> >
> > dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller
> > v5 -> v6:
> > - No update.
> > v4 -> v5:
> > - No update.
> > v3 -> v4:
> > - No update.
> > v2 -> v3:
> > - Change compatible to toshiba,visconti-pwm
> > - Change filename to toshiba,visconti-pwm.yaml.
> > - Add Reviewed-by tag from Rob.
> > v1 -> v2:
> > - Change SPDX-License-Identifier to GPL-2.0-only OR BSD-2-Clause.
> > - Set compatible toshiba,pwm-visconti only.
> > - Drop unnecessary comments.
> >
> > pwm: visconti: Add Toshiba Visconti SoC PWM support
> > v5 -> v6:
> > - Update year in copyright.
> > - Update limitations.
> > - Fix coding style, used braces for both branches.
> > v4 -> v5:
> > - Droped checking PIPGM_PCSR from visconti_pwm_get_state.
> > - Changed from to_visconti_chip to visconti_pwm_from_chip.
> > - Removed pwmchip_remove return value management.
> > - Add limitations of this device.
> > - Add 'state->enabled = true' to visconti_pwm_get_state().
> > v3 -> v4:
> > - Sorted alphabetically include files.
> > - Changed container_of to using static inline functions.
> > - Dropped unnecessary dev_dbg().
> > - Drop Initialization of chip.base.
> > - Drop commnet "period too small".
> > - Rebased for-next.
> > v2 -> v3:
> > - Change compatible to toshiba,visconti-pwm.
> > - Fix MODULE_ALIAS to platform:pwm-visconti, again.
> > - Align continuation line to the opening parenthesis.
> > - Rewrite the contents of visconti_pwm_apply() based on the contents suggested by Uwe.
> > v1 -> v2:
> > - Change SPDX-License-Identifier to GPL-2.0-only.
> > - Add prefix for the register defines.
> > - Drop struct device from struct visconti_pwm_chip.
> > - Use '>>' instead of '/'.
> > - Drop error message by devm_platform_ioremap_resource().
> > - Use dev_err_probe instead of dev_err.
> > - Change dev_info to dev_dbg.
> > - Remove some empty lines.
> > - Fix MODULE_ALIAS to platform:pwm-visconti.
> > - Add .get_state() function.
> > - Use the author name and email address to MODULE_AUTHOR.
> > - Add more comment to function of the hardware.
> > - Support .get_status() function.
> > - Use NSEC_PER_USEC instead of 1000.
> > - Alphabetically sorted for Makefile and Kconfig.
> > - Added check for set value in visconti_pwm_apply().
> >
> > Nobuhiro Iwamatsu (2):
> > dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller
> > pwm: visconti: Add Toshiba Visconti SoC PWM support
> >
> > .../bindings/pwm/toshiba,pwm-visconti.yaml | 43 ++++
> > drivers/pwm/Kconfig | 9 +
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-visconti.c | 189 ++++++++++++++++++
> > 4 files changed, 242 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml
> > create mode 100644 drivers/pwm/pwm-visconti.c
>
> Both patches applied, thanks.
>
> checkpatch did complain when I applied:
>
> > WARNING: please write a paragraph that describes the config symbol fully
> > #9: FILE: drivers/pwm/Kconfig:604:
> > +config PWM_VISCONTI
>
> That seems a bit excessive. The paragraph is perhaps not a poster child
> for Kconfig, but there are others that aren't better, so I think that's
> fine.
>
> > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> > #32:
> > new file mode 100644
>
> Fine, too.
>
> > WARNING: 'loosing' may be misspelled - perhaps 'losing'?
> > #112: FILE: drivers/pwm/pwm-visconti.c:76:
> > + * NSEC_PER_SEC / CLKFREQ = 1000 without loosing precision.
> > ^^^^^^^
>
> I've fixed that up while applying.
>
> > WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
> > #127: FILE: drivers/pwm/pwm-visconti.c:91:
> > + BUG_ON(pwmc0 > 3);
>
> I think that one is legit. I've turned that into:
>
> if (WARN_ON(pwmc0 > 3))
> return -EINVAL;
>
> so that requests for too big period will be rejected rather than crash
> the system.
If this BUG_ON (or your if) triggers we have a compiler or memory
problem. The relevant parts of the code are:
if (state->period > (0xffff << 3) * 1000)
period = (0xffff << 3) * 1000;
else
period = state->period;
period /= 1000;
if (period > 0xffff) {
pwmc0 = ilog2(period >> 16);
BUG_ON(pwmc0 > 3);
Given that period is never bigger than 0xffff << 3 when it is used to
calculate the argument to ilog2, pwmc0 <= ilog2(7) = 2.
Hmm, I wonder if the formula is wrong given that pwmc0 never becomes 3?!
Should this better be
pwmc0 = fls(period >> 16);
?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists