From: Arnd Bergmann <arnd@arndb.de>
To: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: "David Miller" <davem@davemloft.net>,
"Helmut Schaa" <helmut.schaa@googlemail.com>,
"Kalle Valo" <kvalo@codeaurora.org>,
"Daniel Golle" <daniel@makrotopia.org>,
"Mathias Kresin" <dev@kresin.me>,
"Johannes Berg" <johannes.berg@intel.com>,
"Tomislav Požega" <pozega.tomislav@gmail.com>,
"Serge Vasilugin" <vasilugin@yandex.ru>,
"Roman Yeryomin" <roman@advem.lv>,
linux-wireless <linux-wireless@vger.kernel.org>,
Networking <netdev@vger.kernel.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] rt2x00: improve calling conventions for register accessors
Date: Tue, 16 May 2017 17:11:58 +0200 [thread overview]
Message-ID: <CAK8P3a0AqAqWA5FHThKVJtJhymOYZH8F9puR+xOg=xMgtfE4cQ@mail.gmail.com> (raw)
In-Reply-To: <20170516142342.GA6086@redhat.com>
On Tue, May 16, 2017 at 4:23 PM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> On Tue, May 16, 2017 at 01:55:17PM +0200, Stanislaw Gruszka wrote:
>> On Mon, May 15, 2017 at 10:39:51AM -0400, David Miller wrote:
>> > From: Stanislaw Gruszka <sgruszka@redhat.com>
>> > Date: Mon, 15 May 2017 16:28:01 +0200
>> >
>> > > On Mon, May 15, 2017 at 03:46:55PM +0200, Arnd Bergmann wrote:
>> > >> With CONFIG_KASAN enabled and gcc-7, we get a warning about rather high
>> > >> stack usage (with a private patch set I have to turn on this warning,
>> > >> which I intend to get into the next kernel release):
>> > >>
>> > >> wireless/ralink/rt2x00/rt2800lib.c: In function 'rt2800_bw_filter_calibration':
>> > >> wireless/ralink/rt2x00/rt2800lib.c:7990:1: error: the frame size of 2144 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]
>> > >>
>> > >> The problem is that KASAN inserts a redzone around each local variable that
>> > >> gets passed by reference, and the newly added function has a lot of them.
>> > >> We can easily avoid that here by changing the calling convention to have
>> > >> the output as the return value of the function. This should also results in
>> > >> smaller object code, saving around 4KB in .text with KASAN, or 2KB without
>> > >> KASAN.
>> > >>
>> > >> Fixes: 41977e86c984 ("rt2x00: add support for MT7620")
>> > >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> > >> ---
>> > >> drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 319 +++++++++++++------------
>> > >> 1 file changed, 164 insertions(+), 155 deletions(-)
>> > >
>> > > We have read(, &val) calling convention since forever in rt2x00 and that
>> > > was never a problem. I dislike to change that now to make some tools
>> > > happy, I think problem should be fixed in the tools instead.
>> >
>> > Passing return values by reference is and always has been a really
>> > poor way to achieve what these functions are doing.
>> >
>> > And frankly, whilst the tool could see what's going on here better, we
>> > should be making code easier rather than more difficult to audit.
>> >
>> > I am therefore very much in favor of Arnd's change.
>> >
>> > This isn't even a situation where there are multiple return values,
>> > such as needing to signal an error and return an unsigned value at the
>> > same time.
>> >
>> > These functions return _one_ value, and therefore they should be
>> > returned as a true return value.
>>
>> In rt2x00 driver we use poor convention in other kind of registers
>> accessors like bbp, mac, eeprom. I dislike to changing only rfcsr
>> accessors and leaving others in the old way. And changing all accessors
>> would be massive and error prone change, which I'm not prefer either.
>>
>> Arnd, could this be fixed by refactoring rt2800_bw_filter_calibration()
>> function (which is enormous and definitely should be split into smaller
>> subroutines) ? If not, I would accept this patch.
>
> Does below patch make things better with KASAN compilation ?
Yes, that fixes the warning I got:
Before:
$ make -s EXTRA_CFLAGS=-Wframe-larger-than=500
/git/arm-soc/drivers/net/wireless/ralink/rt2x00/rt2800lib.c: In
function 'rt2800_bw_filter_calibration':
/git/arm-soc/drivers/net/wireless/ralink/rt2x00/rt2800lib.c:7990:1:
error: the frame size of 2144 bytes is larger than 500 bytes
[-Werror=frame-larger-than=]
$ size drivers/net/wireless/ralink/rt2x00/built-in.o text data
bss dec hex filename
255979 39442 1536 296957 487fd
drivers/net/wireless/ralink/rt2x00/built-in.o
With your patch:
$ make -s EXTRA_CFLAGS=-Wframe-larger-than=500
/git/arm-soc/drivers/net/wireless/ralink/rt2x00/rt2800lib.c: In
function 'rt2800_bw_filter_calibration':
/git/arm-soc/drivers/net/wireless/ralink/rt2x00/rt2800lib.c:7956:1:
warning: the frame size of 576 bytes is larger than 300 bytes
[-Wframe-larger-than=]
$ size drivers/net/wireless/ralink/rt2x00/built-in.o
text data bss dec hex filename
254367 39538 1536 295441 48211
drivers/net/wireless/ralink/rt2x00/built-in.o
With my 300kb patch:
$ make -s EXTRA_CFLAGS=-Wframe-larger-than=300
$ size drivers/net/wireless/ralink/rt2x00/built-in.o
237312 39442 1536 278290 43f12
drivers/net/wireless/ralink/rt2x00/built-in.o
I passed -Wframe-larger-than=500 here to see the actual stack consumption.
The 2144 bytes are definitely worrying, 576 bytes are generally harmless. My
larger patch improves stack consumption and code size further: it brings all
six functions that had >300 byte stacks below that, but it is not really needed
with your change.
Arnd
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
To: Stanislaw Gruszka <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: "David Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>,
"Helmut Schaa"
<helmut.schaa-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>,
"Kalle Valo" <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>,
"Daniel Golle" <daniel-g5gK2j5usbvCyp4qypjU+w@public.gmane.org>,
"Mathias Kresin" <dev-zg6vgJgm1sizQB+pC5nmwQ@public.gmane.org>,
"Johannes Berg"
<johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
"Tomislav Požega"
<pozega.tomislav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
"Serge Vasilugin"
<vasilugin-o+MxOtu4lMCHXe+LvDLADg@public.gmane.org>,
"Roman Yeryomin" <roman-9zmcapQ0v8Q@public.gmane.org>,
linux-wireless
<linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
Networking <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"Linux Kernel Mailing List"
<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH] rt2x00: improve calling conventions for register accessors
Date: Tue, 16 May 2017 17:11:58 +0200 [thread overview]
Message-ID: <CAK8P3a0AqAqWA5FHThKVJtJhymOYZH8F9puR+xOg=xMgtfE4cQ@mail.gmail.com> (raw)
In-Reply-To: <20170516142342.GA6086-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, May 16, 2017 at 4:23 PM, Stanislaw Gruszka <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, May 16, 2017 at 01:55:17PM +0200, Stanislaw Gruszka wrote:
>> On Mon, May 15, 2017 at 10:39:51AM -0400, David Miller wrote:
>> > From: Stanislaw Gruszka <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > Date: Mon, 15 May 2017 16:28:01 +0200
>> >
>> > > On Mon, May 15, 2017 at 03:46:55PM +0200, Arnd Bergmann wrote:
>> > >> With CONFIG_KASAN enabled and gcc-7, we get a warning about rather high
>> > >> stack usage (with a private patch set I have to turn on this warning,
>> > >> which I intend to get into the next kernel release):
>> > >>
>> > >> wireless/ralink/rt2x00/rt2800lib.c: In function 'rt2800_bw_filter_calibration':
>> > >> wireless/ralink/rt2x00/rt2800lib.c:7990:1: error: the frame size of 2144 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]
>> > >>
>> > >> The problem is that KASAN inserts a redzone around each local variable that
>> > >> gets passed by reference, and the newly added function has a lot of them.
>> > >> We can easily avoid that here by changing the calling convention to have
>> > >> the output as the return value of the function. This should also results in
>> > >> smaller object code, saving around 4KB in .text with KASAN, or 2KB without
>> > >> KASAN.
>> > >>
>> > >> Fixes: 41977e86c984 ("rt2x00: add support for MT7620")
>> > >> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>> > >> ---
>> > >> drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 319 +++++++++++++------------
>> > >> 1 file changed, 164 insertions(+), 155 deletions(-)
>> > >
>> > > We have read(, &val) calling convention since forever in rt2x00 and that
>> > > was never a problem. I dislike to change that now to make some tools
>> > > happy, I think problem should be fixed in the tools instead.
>> >
>> > Passing return values by reference is and always has been a really
>> > poor way to achieve what these functions are doing.
>> >
>> > And frankly, whilst the tool could see what's going on here better, we
>> > should be making code easier rather than more difficult to audit.
>> >
>> > I am therefore very much in favor of Arnd's change.
>> >
>> > This isn't even a situation where there are multiple return values,
>> > such as needing to signal an error and return an unsigned value at the
>> > same time.
>> >
>> > These functions return _one_ value, and therefore they should be
>> > returned as a true return value.
>>
>> In rt2x00 driver we use poor convention in other kind of registers
>> accessors like bbp, mac, eeprom. I dislike to changing only rfcsr
>> accessors and leaving others in the old way. And changing all accessors
>> would be massive and error prone change, which I'm not prefer either.
>>
>> Arnd, could this be fixed by refactoring rt2800_bw_filter_calibration()
>> function (which is enormous and definitely should be split into smaller
>> subroutines) ? If not, I would accept this patch.
>
> Does below patch make things better with KASAN compilation ?
Yes, that fixes the warning I got:
Before:
$ make -s EXTRA_CFLAGS=-Wframe-larger-than=500
/git/arm-soc/drivers/net/wireless/ralink/rt2x00/rt2800lib.c: In
function 'rt2800_bw_filter_calibration':
/git/arm-soc/drivers/net/wireless/ralink/rt2x00/rt2800lib.c:7990:1:
error: the frame size of 2144 bytes is larger than 500 bytes
[-Werror=frame-larger-than=]
$ size drivers/net/wireless/ralink/rt2x00/built-in.o text data
bss dec hex filename
255979 39442 1536 296957 487fd
drivers/net/wireless/ralink/rt2x00/built-in.o
With your patch:
$ make -s EXTRA_CFLAGS=-Wframe-larger-than=500
/git/arm-soc/drivers/net/wireless/ralink/rt2x00/rt2800lib.c: In
function 'rt2800_bw_filter_calibration':
/git/arm-soc/drivers/net/wireless/ralink/rt2x00/rt2800lib.c:7956:1:
warning: the frame size of 576 bytes is larger than 300 bytes
[-Wframe-larger-than=]
$ size drivers/net/wireless/ralink/rt2x00/built-in.o
text data bss dec hex filename
254367 39538 1536 295441 48211
drivers/net/wireless/ralink/rt2x00/built-in.o
With my 300kb patch:
$ make -s EXTRA_CFLAGS=-Wframe-larger-than=300
$ size drivers/net/wireless/ralink/rt2x00/built-in.o
237312 39442 1536 278290 43f12
drivers/net/wireless/ralink/rt2x00/built-in.o
I passed -Wframe-larger-than=500 here to see the actual stack consumption.
The 2144 bytes are definitely worrying, 576 bytes are generally harmless. My
larger patch improves stack consumption and code size further: it brings all
six functions that had >300 byte stacks below that, but it is not really needed
with your change.
Arnd
next prev parent reply other threads:[~2017-05-16 15:12 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-15 13:46 [PATCH] rt2x00: improve calling conventions for register accessors Arnd Bergmann
2017-05-15 14:28 ` Stanislaw Gruszka
2017-05-15 14:36 ` Arnd Bergmann
2017-05-15 14:40 ` David Miller
2017-05-15 19:02 ` Daniel Golle
2017-05-15 19:02 ` Daniel Golle
2017-05-15 14:39 ` David Miller
2017-05-16 11:55 ` Stanislaw Gruszka
2017-05-16 11:58 ` Johannes Berg
2017-05-16 12:43 ` Stanislaw Gruszka
2017-05-16 15:37 ` David Miller
2017-05-16 13:44 ` Jes Sorensen
2017-05-16 14:19 ` Arnd Bergmann
2017-05-16 14:19 ` Arnd Bergmann
2017-05-16 14:31 ` Jes Sorensen
2017-05-16 14:31 ` Jes Sorensen
[not found] ` <CAKR_QVKYpcficnx5G5NdLKh7xa9S18udYQNXDQ=LmfG=Qeapbg@mail.gmail.com>
2017-05-17 14:48 ` Arnd Bergmann
2017-05-16 14:23 ` Stanislaw Gruszka
2017-05-16 15:11 ` Arnd Bergmann [this message]
2017-05-16 15:11 ` Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAK8P3a0AqAqWA5FHThKVJtJhymOYZH8F9puR+xOg=xMgtfE4cQ@mail.gmail.com' \
--to=arnd@arndb.de \
--cc=daniel@makrotopia.org \
--cc=davem@davemloft.net \
--cc=dev@kresin.me \
--cc=helmut.schaa@googlemail.com \
--cc=johannes.berg@intel.com \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pozega.tomislav@gmail.com \
--cc=roman@advem.lv \
--cc=sgruszka@redhat.com \
--cc=vasilugin@yandex.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.