All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Gregory CLEMENT <gregory.clement@free-electrons.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@free-electrons.com>,
	rtc-linux@googlegroups.com, Jason Cooper <jason@lakedaemon.net>,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
	Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
	linux-arm-kernel@lists.infradead.org,
	Shaker Daibes <shaker@marvell.com>
Subject: [rtc-linux] Re: [PATCH 1/3] rtc: armada38x: improve RTC errata implementation
Date: Fri, 9 Dec 2016 17:33:02 +0100	[thread overview]
Message-ID: <20161209163302.GF9923@lunn.ch> (raw)
In-Reply-To: <8760mtf538.fsf@free-electrons.com>

On Fri, Dec 09, 2016 at 05:19:07PM +0100, Gregory CLEMENT wrote:
> Hi Andrew,
> =20
>  On jeu., d=C3=A9c. 08 2016, Andrew Lunn <andrew@lunn.ch> wrote:
>=20
> >> +struct str_value_to_freq {
> >> +	unsigned long value;
> >> +	u8 freq;
> >> +} __packed;
> >> +
> >> +static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, =
u8 rtc_reg)
> >> +{
> >> +	unsigned long value_array[SAMPLE_NR], i, j, value;
> >> +	unsigned long max =3D 0, index_max =3D SAMPLE_NR - 1;
> >> +	struct str_value_to_freq value_to_freq[SAMPLE_NR];
> >
> > Hi Gregory
> >
> > This appears to be putting over 900 bytes on the stack. Is there any
>=20
> Actually the structure being packed it is 500 bytes.

Did you verify this? I never remember where the __packed needs to go.
You clearly have a packed structure, but is the array of structures
packed?

And the long value_array[SAMPLE_NR] is another 400 bytes, totalling
900. And as Russell pointed out, this is on 32 bit systems. Until your
third patch, 64 bit systems probably have double that. I would also
suggest squashing patch #3 into #1.

> > danger of overflowing the stack? Would it be safer to make these
> > arrays part of armada38x_rtc?
>=20
> We could do this if you fear a stack overflow.

It is generally consider not a good idea to put > $BIG structures on
the stack, but the value of $BIG is not clearly defined. Stack
overflow seems to be an issue with lots of layering going on, swap on
NFS etc. But it seems unlikely to me reading the RTC will happen with
an already deep stack. So this is probably O.K.

   Andrew

--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

WARNING: multiple messages have this Message-ID (diff)
From: andrew@lunn.ch (Andrew Lunn)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/3] rtc: armada38x: improve RTC errata implementation
Date: Fri, 9 Dec 2016 17:33:02 +0100	[thread overview]
Message-ID: <20161209163302.GF9923@lunn.ch> (raw)
In-Reply-To: <8760mtf538.fsf@free-electrons.com>

On Fri, Dec 09, 2016 at 05:19:07PM +0100, Gregory CLEMENT wrote:
> Hi Andrew,
>  
>  On jeu., d?c. 08 2016, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >> +struct str_value_to_freq {
> >> +	unsigned long value;
> >> +	u8 freq;
> >> +} __packed;
> >> +
> >> +static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
> >> +{
> >> +	unsigned long value_array[SAMPLE_NR], i, j, value;
> >> +	unsigned long max = 0, index_max = SAMPLE_NR - 1;
> >> +	struct str_value_to_freq value_to_freq[SAMPLE_NR];
> >
> > Hi Gregory
> >
> > This appears to be putting over 900 bytes on the stack. Is there any
> 
> Actually the structure being packed it is 500 bytes.

Did you verify this? I never remember where the __packed needs to go.
You clearly have a packed structure, but is the array of structures
packed?

And the long value_array[SAMPLE_NR] is another 400 bytes, totalling
900. And as Russell pointed out, this is on 32 bit systems. Until your
third patch, 64 bit systems probably have double that. I would also
suggest squashing patch #3 into #1.

> > danger of overflowing the stack? Would it be safer to make these
> > arrays part of armada38x_rtc?
> 
> We could do this if you fear a stack overflow.

It is generally consider not a good idea to put > $BIG structures on
the stack, but the value of $BIG is not clearly defined. Stack
overflow seems to be an issue with lots of layering going on, swap on
NFS etc. But it seems unlikely to me reading the RTC will happen with
an already deep stack. So this is probably O.K.

   Andrew

  reply	other threads:[~2016-12-09 16:33 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-08 17:10 [rtc-linux] [PATCH 0/3] rtc: armada38x: Few improvement and cleanup Gregory CLEMENT
2016-12-08 17:10 ` Gregory CLEMENT
2016-12-08 17:10 ` [rtc-linux] [PATCH 1/3] rtc: armada38x: improve RTC errata implementation Gregory CLEMENT
2016-12-08 17:10   ` Gregory CLEMENT
2016-12-08 17:29   ` [rtc-linux] " Andrew Lunn
2016-12-08 17:29     ` Andrew Lunn
2016-12-09 16:19     ` [rtc-linux] " Gregory CLEMENT
2016-12-09 16:19       ` Gregory CLEMENT
2016-12-09 16:33       ` Andrew Lunn [this message]
2016-12-09 16:33         ` Andrew Lunn
2016-12-08 17:37   ` [rtc-linux] " Russell King - ARM Linux
2016-12-08 17:37     ` Russell King - ARM Linux
2016-12-09 16:37     ` [rtc-linux] " Gregory CLEMENT
2016-12-09 16:37       ` Gregory CLEMENT
2016-12-08 17:10 ` [rtc-linux] [PATCH 2/3] rtc: armada38x: Convert to time64_t Gregory CLEMENT
2016-12-08 17:10   ` Gregory CLEMENT
2016-12-08 18:08   ` [rtc-linux] " Russell King - ARM Linux
2016-12-08 18:08     ` Russell King - ARM Linux
2016-12-08 17:10 ` [rtc-linux] [PATCH 3/3] rtc: armada38x: Prepare for being use on 64 bits Gregory CLEMENT
2016-12-08 17:10   ` Gregory CLEMENT
2016-12-08 18:09   ` [rtc-linux] " Russell King - ARM Linux
2016-12-08 18:09     ` Russell King - ARM Linux

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=20161209163302.GF9923@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=a.zummo@towertech.it \
    --cc=alexandre.belloni@free-electrons.com \
    --cc=gregory.clement@free-electrons.com \
    --cc=jason@lakedaemon.net \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=rtc-linux@googlegroups.com \
    --cc=sebastian.hesselbarth@gmail.com \
    --cc=shaker@marvell.com \
    --cc=thomas.petazzoni@free-electrons.com \
    /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.