linux-kernel - Re: [PATCH] misc: aspeed-lpc-ctrl: make memory-region optional

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]

Message-ID: <6C0EA492-D9D4-481F-BF8B-5EBD8B38390B@fb.com>
Date:   Wed, 16 Jan 2019 19:16:53 +0000
From:   Vijay Khemka <vijaykhemka@...com>
To:     Joel Stanley <joel@....id.au>, Andrew Jeffery <andrew@...id.au>
CC:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "openbmc @ lists . ozlabs . org" <openbmc@...ts.ozlabs.org>
Subject: Re: [PATCH] misc: aspeed-lpc-ctrl: make memory-region optional

Sure Joel,
I will do v2 and submit for review today.

Regards
-Vijay

On 1/15/19, 4:11 PM, "Joel Stanley" <joel@....id.au> wrote:

    Hi Vijay,
    
    On Tue, 15 Jan 2019 at 12:51, Vijay Khemka <vijaykhemka@...com> wrote:
    >
    > Makiing memory-region as optional parameter in device tree if
    > user needs to use memory-region then define in devicetree.
    
    Thank you for finding the time to do this work. You're not the first
    to be blocked by this limitation, but you are the first who found the
    time to propose a patch.
    
    I think we should complete the rework to make the flash device
    optional too. Can you do a v2 with a proposal for that?
    
    >
    > Signed-off-by: Vijay Khemka <vijaykhemka@...com>
    > ---
    >  drivers/misc/aspeed-lpc-ctrl.c | 30 ++++++++++++++++++------------
    >  1 file changed, 18 insertions(+), 12 deletions(-)
    >
    > diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
    > index a024f8042259..20507f0764fb 100644
    > --- a/drivers/misc/aspeed-lpc-ctrl.c
    > +++ b/drivers/misc/aspeed-lpc-ctrl.c
    > @@ -90,6 +90,10 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
    >                 if (map.window_id != 0)
    >                         return -EINVAL;
    >
    > +               /* If memory-region is not described in device tree */
    > +               if (!lpc_ctrl->mem_size)
    > +                       return -EINVAL;
    > +
    >                 map.size = lpc_ctrl->mem_size;
    >
    >                 return copy_to_user(p, &map, sizeof(map)) ? -EFAULT : 0;
    > @@ -129,6 +133,10 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
    >                         addr = lpc_ctrl->pnor_base;
    >                         size = lpc_ctrl->pnor_size;
    >                 } else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
    > +                       /* If memory-region is not described in device tree */
    > +                       if (!lpc_ctrl->mem_size)
    
    Should it print a message here? I think a dev_dbg makes sense.
    
    > +                               return -EINVAL;
    > +
    >                         addr = lpc_ctrl->mem_base;
    >                         size = lpc_ctrl->mem_size;
    >                 } else {
    > @@ -214,22 +222,20 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
    >
    >         dev_set_drvdata(&pdev->dev, lpc_ctrl);
    >
    > +       /* If memory-region is described in device tree then store */
    >         node = of_parse_phandle(dev->of_node, "memory-region", 0);
    > -       if (!node) {
    > -               dev_err(dev, "Didn't find reserved memory\n");
    > -               return -EINVAL;
    > -       }
    > +       if (node) {
    
    Print a debug message when we didn't find the node.
    
    > +               rc = of_address_to_resource(node, 0, &resm);
    > +               of_node_put(node);
    > +               if (rc) {
    > +                       dev_err(dev, "Couldn't address to resource for reserved memory\n");
    > +                       return -ENOMEM;
    > +               }
    >
    > -       rc = of_address_to_resource(node, 0, &resm);
    > -       of_node_put(node);
    > -       if (rc) {
    > -               dev_err(dev, "Couldn't address to resource for reserved memory\n");
    > -               return -ENOMEM;
    > +               lpc_ctrl->mem_size = resource_size(&resm);
    > +               lpc_ctrl->mem_base = resm.start;
    >         }
    >
    > -       lpc_ctrl->mem_size = resource_size(&resm);
    > -       lpc_ctrl->mem_base = resm.start;
    > -
    >         lpc_ctrl->regmap = syscon_node_to_regmap(
    >                         pdev->dev.parent->of_node);
    >         if (IS_ERR(lpc_ctrl->regmap)) {
    
    Down below here is this line:
    
    >        dev_info(dev, "Loaded at %pr\n", &resm);
    
    As resm is first used for the flash resource and then the memory
    region, it will display the location of the flash when the memory
    region is not present. This is confusing. You should either print out
    the regions separately, or consider removing this dev_info completely.

Yes it did confuse me too and I also thought of removing it. I was waiting for your opinion __
    
    > --
    > 2.17.1
    >