linux-kernel - Re: [PATCH 1/2] drivers/memory: Add deep sleep support for IFC

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] [day] [month] [year] [list]

Date:	Thu, 29 Oct 2015 11:21:44 -0500
From:	Scott Wood <scottwood@...escale.com>
To:	Raghav Dogra <raghav@...escale.com>
CC:	<linux-kernel@...r.kernel.org>, <prabhakar@...escale.com>
Subject: Re: [PATCH 1/2] drivers/memory: Add deep sleep support for IFC

On Tue, 2015-10-27 at 16:34 +0530, Raghav Dogra wrote:
> Add support of suspend, resume function to support deep sleep.
> Also make sure of SRAM initialization  during resume.
> 
> Signed-off-by: Raghav Dogra <raghav@...escale.com>
> ---
>  drivers/memory/fsl_ifc.c | 56 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fsl_ifc.h  |  6 ++++++
>  2 files changed, 62 insertions(+)

Who are you asking to apply this?  If me, please send to 
 linuxppc-dev@...ts.ozlabs.orgso it gets on patchwork.

Also keep Prabhakar CCed on IFC patches.

> diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> index e87459f..163ccf2 100644
> --- a/drivers/memory/fsl_ifc.c
> +++ b/drivers/memory/fsl_ifc.c
> @@ -23,6 +23,7 @@
>  #include <linux/kernel.h>
>  #include <linux/compiler.h>
>  #include <linux/spinlock.h>
> +#include <linux/delay.h>
>  #include <linux/types.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
> @@ -35,6 +36,9 @@
>  struct fsl_ifc_ctrl *fsl_ifc_ctrl_dev;
>  EXPORT_SYMBOL(fsl_ifc_ctrl_dev);
>  
> +#define FSL_IFC_V1_3_0       0x01030000
> +#define IFC_TIMEOUT_MSECS    100000 /* 100ms */
> +
>  /*
>   * convert_ifc_address - convert the base address
>   * @addr_base:       base address of the memory bank
> @@ -308,6 +312,53 @@ err:
>       return ret;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +/* save ifc registers */
> +static int fsl_ifc_suspend(struct device *dev)
> +{
> +     struct fsl_ifc_ctrl *ctrl = dev_get_drvdata(dev);
> +     struct fsl_ifc_regs __iomem *ifc = ctrl->regs;
> +
> +     ctrl->saved_regs = kzalloc(sizeof(struct fsl_ifc_regs), GFP_KERNEL);
> +     if (!ctrl->saved_regs)
> +             return -ENOMEM;
> +
> +     _memcpy_fromio(ctrl->saved_regs, ifc, sizeof(struct fsl_ifc_regs));
> +
> +     return 0;
> +}

Why _memcpy_fromio() rather than memcpy_fromio()?

> +/* restore ifc registers */
> +static int fsl_ifc_resume(struct device *dev)
> +{
> +     struct fsl_ifc_ctrl *ctrl = dev_get_drvdata(dev);
> +     struct fsl_ifc_regs __iomem *ifc = ctrl->regs;
> +     uint32_t ver = 0, ncfgr, status;
> +
> +     if (ctrl->saved_regs) {
> +             _memcpy_toio(ifc, ctrl->saved_regs,
> +                             sizeof(struct fsl_ifc_regs));

This is too simplistic, and doesn't account for things like read-to-clear 
bits, or the possibility of reserved fields that expose internal state that 
shouldn't be messed with.  Registers that need state saving or 
reinitialization should be handled individually.

You should also make sure that interrupts are disabled at the device.

> +             kfree(ctrl->saved_regs);
> +             ctrl->saved_regs = NULL;
> +     }
> +
> +     ver = in_be32(&ctrl->regs->ifc_rev);
> +     ncfgr = in_be32(&ifc->ifc_nand.ncfgr);
> +     if (ver >= FSL_IFC_V1_3_0) {
> +             out_be32(&ifc->ifc_nand.ncfgr, ncfgr | IFC_NAND_SRAM_INIT_EN);
> +
> +             /* wait for  SRAM_INIT bit to be clear or timeout */
> +             status = spin_event_timeout(!(in_be32(&ifc->ifc_nand.ncfgr)
> +                                & IFC_NAND_SRAM_INIT_EN),
> +                                IFC_TIMEOUT_MSECS, 0);
> +             if (!status)
> +                     dev_err(ctrl->dev, "Timeout waiting for IFC SRAM INIT");
> +     }
> +
> +     return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */

Normally we do NAND SRAM init in the NAND driver.  Why are you doing it here? 
 How is SRAM going to be reset on older IFCs?

BTW, I see that Linux is missing SRAM init for IFCs newer than v1.1 -- we're 
relying on U-Boot to do so, which we shouldn't be.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/