linux-kernel - Re: [PATCHv4 3/3] gpio: msm: Add device tree and irqdomain support for gpio-msm-v2

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 05 Jun 2013 22:38:13 +0100
From:	Grant Likely <grant.likely@...aro.org>
To:	Rohit Vaswani <rvaswani@...eaurora.org>
Cc:	Linus Walleij <linus.walleij@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>,
	Russell King <linux@....linux.org.uk>,
	David Brown <davidb@...eaurora.org>,
	Bryan Huntsman <bryanh@...eaurora.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCHv4 3/3] gpio: msm: Add device tree and irqdomain support for gpio-msm-v2

On Sun, 02 Jun 2013 19:31:33 -0700, Rohit Vaswani <rvaswani@...eaurora.org> wrote:
> On 6/1/2013 1:09 AM, Grant Likely wrote:
> > On Sat, Jun 1, 2013 at 1:22 AM, Rohit Vaswani <rvaswani@...eaurora.org> wrote:
> >> This cleans up the gpio-msm-v2 driver of all the global define usage.
> >> The number of gpios are now defined in the device tree. This enables
> >> adding irqdomain support as well.
> >>
> >> Signed-off-by: Rohit Vaswani <rvaswani@...eaurora.org>
> >> @@ -101,11 +96,27 @@ enum {
> >>    */
> >>   struct msm_gpio_dev {
> >>          struct gpio_chip gpio_chip;
> >> -       DECLARE_BITMAP(enabled_irqs, NR_GPIO_IRQS);
> >> -       DECLARE_BITMAP(wake_irqs, NR_GPIO_IRQS);
> >> -       DECLARE_BITMAP(dual_edge_irqs, NR_GPIO_IRQS);
> >> +       unsigned long *enabled_irqs;
> >> +       unsigned long *wake_irqs;
> >> +       unsigned long *dual_edge_irqs;
> > Was there a reason you ignored the comment to leave these bitmaps as
> > statically allocated?
> >
> > [...]
> >
> >> +       msm_gpio.enabled_irqs = devm_kzalloc(&pdev->dev,
> >> +                                               sizeof(unsigned long) *
> >> +                                               BITS_TO_LONGS(ngpio) * 3,
> >> +                                               GFP_KERNEL);
> >> +       msm_gpio.wake_irqs = &msm_gpio.enabled_irqs[BITS_TO_LONGS(ngpio)];
> >> +       msm_gpio.dual_edge_irqs =
> >> +                       &msm_gpio.enabled_irqs[BITS_TO_LONGS(ngpio * 2)];
> > I should have just deleted my comment about doing it this way. I was
> > making the point that one allocation is better that three; but then I
> > also said that it was better to not allocate at all. Go back to the
> > statically allocated bitmap array. It is far better than this.
> >
> > g.
> 
> I agree that DECLARE_BITMAP is the most efficient way, but 
> DECLARE_BITMAP takes a statically defined number of gpios as an 
> argument. Since we get the ngpio from device tree, these had to go as 
> well. Under this scheme, 1 allocation was better than 3 and went ahead 
> with your suggestion.

Think about it for a moment; You *know* all the devices that are going
to use the driver. The largest size for ngpios that I see is 173 which
is a mere 18 long words for 3 bitmaps. Even if you were to quadruple
that amount the total for all the bitmasks is 72 long words. You're
optimizing at the wrong place. Changing it to dynamic allocation makes
the code slower, more complicated, and probably doesn't do anything to
save on real memory consumption.

The solution is simple; choose a maximum value of ngpios that the driver
will likely need to work support. If a device appears with more GPIOs,
then the driver should throw a warning and work with the maximum it can
support. Part of enablement for the new device will be increasing the
driver to handle more gpios.

g.
--
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/