linux-kernel - RE: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

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: <CH2PR13MB33680443C101511E66ECADF08C4D0@CH2PR13MB3368.namprd13.prod.outlook.com>
Date:   Mon, 18 Nov 2019 10:03:52 +0000
From:   Yash Shah <yash.shah@...ive.com>
To:     Bartosz Golaszewski <bgolaszewski@...libre.com>
CC:     "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "palmer@...belt.com" <palmer@...belt.com>,
        "Paul Walmsley ( Sifive)" <paul.walmsley@...ive.com>,
        "aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "jason@...edaemon.net" <jason@...edaemon.net>,
        "maz@...nel.org" <maz@...nel.org>,
        "bmeng.cn@...il.com" <bmeng.cn@...il.com>,
        "atish.patra@....com" <atish.patra@....com>,
        Sagar Kadam <sagar.kadam@...ive.com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Sachin Ghadi <sachin.ghadi@...ive.com>
Subject: RE: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

> -----Original Message-----
> From: Bartosz Golaszewski <bgolaszewski@...libre.com>
> Sent: 13 November 2019 18:41
> To: Yash Shah <yash.shah@...ive.com>
> Cc: linus.walleij@...aro.org; robh+dt@...nel.org; mark.rutland@....com;
> palmer@...belt.com; Paul Walmsley ( Sifive) <paul.walmsley@...ive.com>;
> aou@...s.berkeley.edu; tglx@...utronix.de; jason@...edaemon.net;
> maz@...nel.org; bmeng.cn@...il.com; atish.patra@....com; Sagar Kadam
> <sagar.kadam@...ive.com>; linux-gpio@...r.kernel.org;
> devicetree@...r.kernel.org; linux-riscv@...ts.infradead.org; linux-
> kernel@...r.kernel.org; Sachin Ghadi <sachin.ghadi@...ive.com>
> Subject: Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
> 
> wt., 12 lis 2019 o 13:12 Yash Shah <yash.shah@...ive.com> napisał(a):
> >
> > Adds the GPIO driver for SiFive RISC-V SoCs.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@...ive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@....com>
> > Signed-off-by: Yash Shah <yash.shah@...ive.com>

[...]

> > +
> > +static int sifive_gpio_probe(struct platform_device *pdev) {
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *node = pdev->dev.of_node;
> > +       struct device_node *irq_parent;
> > +       struct irq_domain *parent;
> > +       struct gpio_irq_chip *girq;
> > +       struct sifive_gpio *chip;
> > +       struct resource *res;
> > +       int ret, ngpio;
> > +
> > +       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> > +       if (!chip)
> > +               return -ENOMEM;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       chip->base = devm_ioremap_resource(dev, res);
> 
> Use devm_platform_ioremap_resource() and drop the res variable.
> 

Sure, will do that.

> > +       if (IS_ERR(chip->base)) {
> > +               dev_err(dev, "failed to allocate device memory\n");
> > +               return PTR_ERR(chip->base);
> > +       }
> > +
> > +       chip->regs = devm_regmap_init_mmio(dev, chip->base,
> > +
> > + &sifive_gpio_regmap_config);
> 
> Why do you need this regmap here? You initialize a new regmap, then use
> your own locking despite not having disabled the internal locking in regmap,
> and then you initialize the mmio generic GPIO code which will use yet
> another lock to operate on the same registers and in the end you write to
> those registers without taking any lock anyway.
> Doesn't make much sense to me.
> 

As suggested in the comments received on the RFC version of this patch[0], I am trying to use regmap MMIO by looking at gpio-mvebu.c. I got your point regarding the usage of own locks is not making any sense.
Here is what I will do in v2:
1. drop the usage of own locks
2. consistently use regmap_* apis for register access (replace all iowrites).
Does this make sense now?

> > +       if (IS_ERR(chip->regs))
> > +               return PTR_ERR(chip->regs);
> > +

[...]

> > +
> > +       ret = gpiochip_add_data(&chip->gc, chip);
> > +       if (ret)
> > +               return ret;
> > +
> > +       platform_set_drvdata(pdev, chip);
> > +       dev_info(dev, "SiFive GPIO chip registered %d GPIOs\n",
> > + ngpio);
> 
> Core gpio library emits a very similar debug message from
> gpiochip_setup_dev(), I think you can drop it and directly return
> gpiochip_add_data().
> 
> Bartosz

Ok. Will directly return gpiochip_add_data().
Thanks for your comments!

- Yash

[0] https://lore.kernel.org/linux-riscv/20181010123519.RVexDppaPFpIWl7QU_hpP8tc5qqWPJgeuLYn0FaGbeQ@z/