linux-kernel - Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

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: <a929309891f9f28ae71f7ee09e990dc8bc362fdf.camel@pengutronix.de>
Date:   Tue, 14 Jun 2022 16:49:31 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Brad Larson <brad@...sando.io>,
        linux-arm-kernel@...ts.infradead.org
Cc:     linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org,
        adrian.hunter@...el.com, alcooperx@...il.com,
        andy.shevchenko@...il.com, arnd@...db.de, blarson@....com,
        brijeshkumar.singh@....com, catalin.marinas@....com,
        gsomlo@...il.com, gerg@...ux-m68k.org, krzk@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, lee.jones@...aro.org,
        broonie@...nel.org, yamada.masahiro@...ionext.com,
        piotrs@...ence.com, p.yadav@...com, rdunlap@...radead.org,
        robh+dt@...nel.org, samuel@...lland.org, fancer.lancer@...il.com,
        suravee.suthikulpanit@....com, thomas.lendacky@....com,
        ulf.hansson@...aro.org, will@...nel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset
 Controller

Hi Brad,

On Mo, 2022-06-13 at 12:56 -0700, Brad Larson wrote:
> From: Brad Larson <blarson@....com>
> 
> This patch adds the reset controller functionality for the
> AMD Pensando Elba System Resource Chip.
> 
> Signed-off-by: Brad Larson <blarson@....com>
[...]
> diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
> new file mode 100644
> index 000000000000..6e429cb11466
> --- /dev/null
> +++ b/drivers/reset/reset-elbasr.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2022 AMD Pensando
> + */
> +
> +#include <linux/mfd/pensando-elbasr.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +
> +#include <dt-bindings/reset/amd,pensando-elba-reset.h>
> +
> +struct elbasr_reset {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +};
> +
> +static inline struct elbasr_reset *to_elbasr_rst(struct reset_controller_dev *rc)
> +{
> +	return container_of(rc, struct elbasr_reset, rcdev);
> +}
> +
> +static inline int elbasr_reset_shift(unsigned long id)
> +{
> +	switch (id) {
> +	case EMMC_HW_RESET:

Are there more reset controls than EMMC_HW_RESET?
If so, please list them all.
If not, why is this a function with a switch statement for a single
reset bit?

> +		return 6;
> +	default:
> +		return -EINVAL;

The error return value is never checked.
This can't be reached, since ELBASR_NR_RESETS == 1. So id will only
ever be 0.

> +static int elbasr_reset_probe(struct platform_device *pdev)
> +{
> +	struct elbasr_data *elbasr = dev_get_drvdata(pdev->dev.parent);

Peeking into the MFD driver's private data structure seems unnecessary.
Consider using dev_get_regmap() instead.


regards
Philipp