Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200107212747.4182515-1-arnd@arndb.de> <20200107220019.GC7869@pendragon.ideasonboard.com>
 <CAK8P3a1Gt10_OLF1dXiNBxcO-seJfutcPu3w_dsHKNsDN4r7-A@mail.gmail.com>
 <20200107221222.GE7869@pendragon.ideasonboard.com> <CAK8P3a0ny0UhOpvVNE3x6gE=3SG7h_sBvtR7L7Hj2XsjrkavAA@mail.gmail.com>
In-Reply-To: <CAK8P3a0ny0UhOpvVNE3x6gE=3SG7h_sBvtR7L7Hj2XsjrkavAA@mail.gmail.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Wed, 8 Jan 2020 11:31:05 +0100
Message-ID: <CAK8P3a3jAnFZA3GFRtdYdg1-i-oih3pOQzkkrK-X3BGsFrMiZQ@mail.gmail.com>
Subject: Re: [PATCH] drm: panel: fix excessive stack usage in td028ttec1_prepare
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Oleksandr Natalenko <oleksandr@redhat.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Boris Brezillon <boris.brezillon@collabora.com>,
        Tomi Valkeinen <tomi.valkeinen@ti.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jan 7, 2020 at 11:26 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Jan 7, 2020 at 11:12 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Tue, Jan 07, 2020 at 11:09:13PM +0100, Arnd Bergmann wrote:
> > > On Tue, Jan 7, 2020 at 11:00 PM Laurent Pinchart wrote:
>
> > > > Isn't this something that should be fixed at the compiler level ?
> > >
> > > I suspect but have not verified that structleak gcc plugin is partly at
> > > fault here as well, it has caused similar problems elsewhere.
>
> I checked that now, and it's indeed the structleak plugin.
>
> Interestingly the problem goes away without the -fconserve-stack
> option, which is meant to reduce the stack usage bug has the
> opposite effect here (!).
>
> I'll do some more tests tomorrow.

Here's a reduced test case:

struct list_head {
  struct list_head *next, *prev;
} typedef initcall_t;
struct sg_table {
  int sgl;
  int nents;
  int orig_nents;
};
struct spi_transfer {
  void *tx_buf;
  void *rx_buf;
  unsigned len;
  int tx_dma;
  int rx_dma;
  struct sg_table tx_sg;
  struct sg_table rx_sg;
  short delay_usecs;
  int delay;
  int cs_change_delay;
  int word_delay;
  int speed_hz;
  int effective_speed_hz;
  int ptp_sts_word_pre;
  int ptp_sts_word_post;
  int ptp_sts;
  _Bool timestamped_pre;
  struct list_head transfer_list;
};
void spi_sync_transfer(struct spi_transfer *, int);
void spi_write(void) {
  struct spi_transfer t;
  spi_sync_transfer(&t, 0);
}
int jbt_ret_write_0_err;
void jbt_ret_write_0(void) {
  if (jbt_ret_write_0_err)
    spi_write();
}
void jbt_reg_write_1(int *err) {
  if (*err) {
    struct spi_transfer t;
    spi_sync_transfer(&t, 1);
  }
}
void jbt_reg_write_2(int *err) {
  short tx_buf[3];
  if (err) {
    struct spi_transfer t = {tx_buf};
    spi_sync_transfer(&t, 0);
  }
}
int td028ttec1_prepare_i;
void td028ttec1_prepare() {
  int ret;
  for (; td028ttec1_prepare_i; ++td028ttec1_prepare_i) {
    jbt_ret_write_0();
  }
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_2(&ret);
  jbt_ret_write_0();
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
  jbt_reg_write_1(&ret);
}

$ arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc panel-tpo-td028ttec1.c
-fplugin=scripts/gcc-plugins/structleak_plugin.so
-fplugin-arg-structleak_plugin-byref-all  -S -O3
-Wframe-larger-than=128
panel-tpo-td028ttec1.i: In function 'td028ttec1_prepare':
panel-tpo-td028ttec1.i:80:1: warning: the frame size of 192 bytes is
larger than 128 bytes [-Wframe-larger-than=]

$ arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc panel-tpo-td028ttec1.c
-fplugin=scripts/gcc-plugins/structleak_plugin.so
-fplugin-arg-structleak_plugin-byref-all  -S -O3
-Wframe-larger-than=128 -fconserve-stack
panel-tpo-td028ttec1.i: In function 'td028ttec1_prepare':
panel-tpo-td028ttec1.i:80:1: warning: the frame size of 2032 bytes is
larger than 128 bytes [-Wframe-larger-than=]

I'm still not entirely sure what to make of this. The -fconserve-stack
is supposed
to prevent inlining when the frames get too large, but it appears that inlining
less has the opposite effect here, as it leaves larger structures on the stack
of the caller. structleak_plugin-byref-all causes each copy of the
'struct spi_transfer'
to be initialized (intentionally) and left on the stack (as a
side-effect of a somewhat
suboptimal implementation).

         Arnd