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]
Date:   Fri, 15 Jan 2021 16:35:07 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Sameer Pujar <spujar@...dia.com>,
        Peter Geis <pgwipeout@...il.com>,
        Nicolas Chauvet <kwizart@...il.com>,
        Takashi Iwai <tiwai@...e.com>,
        Jaroslav Kysela <perex@...ex.cz>, alsa-devel@...a-project.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/5] ALSA: hda/tegra: Reset hardware

On Tue, Jan 12, 2021 at 03:58:31PM +0300, Dmitry Osipenko wrote:
> Reset hardware in order to bring it into a predictable state.
> 
> Tested-by: Peter Geis <pgwipeout@...il.com>
> Tested-by: Nicolas Chauvet <kwizart@...il.com>
> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> ---
>  sound/pci/hda/hda_tegra.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> index 4c799661c2f6..e406b7980f31 100644
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -17,6 +17,7 @@
>  #include <linux/moduleparam.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/time.h>
>  #include <linux/string.h>
> @@ -70,6 +71,7 @@
>  struct hda_tegra {
>  	struct azx chip;
>  	struct device *dev;
> +	struct reset_control *reset;
>  	struct clk_bulk_data clocks[3];
>  	unsigned int nclocks;
>  	void __iomem *regs;
> @@ -167,6 +169,12 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
>  	int rc;
>  
> +	if (!(chip && chip->running)) {

Isn't that check for !chip a bit redundant? If that pointer isn't valid,
we're just going to go crash when dereferencing hda later on, so I think
this can simply be:

	if (!chip->running)

I guess you took this from the inverse check below, but I think we can
also drop it from there, perhaps in a separate patch.

> +		rc = reset_control_assert(hda->reset);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
>  	if (rc != 0)
>  		return rc;
> @@ -176,6 +184,10 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  		/* disable controller wake up event*/
>  		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
>  			   ~STATESTS_INT_MASK);
> +	} else {
> +		rc = reset_control_reset(hda->reset);

The "if (chip)" part definitely doesn't make sense after this anymore
because now if chip == NULL, then we end up in here and dereference an
invalid "hda" pointer.

Also, why reset_control_reset() here? We'll reach this if we ran
reset_control_assert() above, so this should just be
reset_control_deassert() to undo that, right? I suppose it wouldn't hurt
to put throw that standard usleep_range() in there as well that we use
to wait between reset assert and deassert to make sure the clocks have
stabilized and the reset has indeed propagated through the whole IP.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists