[PATCH 1/1] et8ek8: Decrease stack usage

[PATCH 1/1] et8ek8: Decrease stack usage

[Sakari Ailus]

The et8ek8 driver combines I²C register writes to a single array that it
passes to i2c_transfer(). The maximum number of writes is 48 at once,
decrease it to 8 and make more transfers if needed, thus avoiding a
warning on stack usage.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Pavel: this is just compile tested. Could you test it on N900, please?

 drivers/media/i2c/et8ek8/et8ek8_driver.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
index f39f517..c14f0fd 100644
--- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
+++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
@@ -43,7 +43,7 @@
 
 #define ET8EK8_NAME		"et8ek8"
 #define ET8EK8_PRIV_MEM_SIZE	128
-#define ET8EK8_MAX_MSG		48
+#define ET8EK8_MAX_MSG		8
 
 struct et8ek8_sensor {
 	struct v4l2_subdev subdev;
@@ -220,7 +220,8 @@ static void et8ek8_i2c_create_msg(struct i2c_client *client, u16 len, u16 reg,
 
 /*
  * A buffered write method that puts the wanted register write
- * commands in a message list and passes the list to the i2c framework
+ * commands in smaller number of message lists and passes the lists to
+ * the i2c framework
  */
 static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
 					  const struct et8ek8_reg *wnext,
@@ -231,11 +232,7 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
 	int wcnt = 0;
 	u16 reg, data_length;
 	u32 val;
-
-	if (WARN_ONCE(cnt > ET8EK8_MAX_MSG,
-		      ET8EK8_NAME ": %s: too many messages.\n", __func__)) {
-		return -EINVAL;
-	}
+	int rval;
 
 	/* Create new write messages for all writes */
 	while (wcnt < cnt) {
@@ -249,10 +246,21 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
 
 		/* Update write count */
 		wcnt++;
+
+		if (wcnt < ET8EK8_MAX_MSG)
+			continue;
+
+		rval = i2c_transfer(client->adapter, msg, wcnt);
+		if (rval < 0)
+			return rval;
+
+		cnt -= wcnt;
+		wcnt = 0;
 	}
 
-	/* Now we send everything ... */
-	return i2c_transfer(client->adapter, msg, wcnt);
+	rval = i2c_transfer(client->adapter, msg, wcnt);
+
+	return rval < 0 ? rval : 0;
 }
 
 /*
-- 
2.7.4

Re: [PATCH 1/1] et8ek8: Decrease stack usage

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3325 bytes --]

On Wed 2017-08-16 10:33:45, Sakari Ailus wrote:
> The et8ek8 driver combines I²C register writes to a single array that it
> passes to i2c_transfer(). The maximum number of writes is 48 at once,
> decrease it to 8 and make more transfers if needed, thus avoiding a
> warning on stack usage.

Dunno. Slowing down code to save stack does not sound attractive.

What about this one? Way simpler, too... (Unless there's some rule
about i2c, DMA and static buffers. Is it?)

Signed-off-by: Pavel Machek <pavel@ucw.cz>

 (untested)
								Pavel

diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
index f39f517..64da731 100644
--- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
+++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
@@ -227,7 +227,7 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
 					  int cnt)
 {
 	struct i2c_msg msg[ET8EK8_MAX_MSG];
-	unsigned char data[ET8EK8_MAX_MSG][6];
+	static unsigned char data[ET8EK8_MAX_MSG][6];
 	int wcnt = 0;
 	u16 reg, data_length;
 	u32 val;



> ---
> Pavel: this is just compile tested. Could you test it on N900, please?
> 
>  drivers/media/i2c/et8ek8/et8ek8_driver.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> index f39f517..c14f0fd 100644
> --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
> +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> @@ -43,7 +43,7 @@
>  
>  #define ET8EK8_NAME		"et8ek8"
>  #define ET8EK8_PRIV_MEM_SIZE	128
> -#define ET8EK8_MAX_MSG		48
> +#define ET8EK8_MAX_MSG		8
>  
>  struct et8ek8_sensor {
>  	struct v4l2_subdev subdev;
> @@ -220,7 +220,8 @@ static void et8ek8_i2c_create_msg(struct i2c_client *client, u16 len, u16 reg,
>  
>  /*
>   * A buffered write method that puts the wanted register write
> - * commands in a message list and passes the list to the i2c framework
> + * commands in smaller number of message lists and passes the lists to
> + * the i2c framework
>   */
>  static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
>  					  const struct et8ek8_reg *wnext,
> @@ -231,11 +232,7 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
>  	int wcnt = 0;
>  	u16 reg, data_length;
>  	u32 val;
> -
> -	if (WARN_ONCE(cnt > ET8EK8_MAX_MSG,
> -		      ET8EK8_NAME ": %s: too many messages.\n", __func__)) {
> -		return -EINVAL;
> -	}
> +	int rval;
>  
>  	/* Create new write messages for all writes */
>  	while (wcnt < cnt) {
> @@ -249,10 +246,21 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
>  
>  		/* Update write count */
>  		wcnt++;
> +
> +		if (wcnt < ET8EK8_MAX_MSG)
> +			continue;
> +
> +		rval = i2c_transfer(client->adapter, msg, wcnt);
> +		if (rval < 0)
> +			return rval;
> +
> +		cnt -= wcnt;
> +		wcnt = 0;
>  	}
>  
> -	/* Now we send everything ... */
> -	return i2c_transfer(client->adapter, msg, wcnt);
> +	rval = i2c_transfer(client->adapter, msg, wcnt);
> +
> +	return rval < 0 ? rval : 0;
>  }
>  
>  /*

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/1] et8ek8: Decrease stack usage

[Sakari Ailus]

On Wed, Aug 16, 2017 at 10:13:05AM +0200, Pavel Machek wrote:
> On Wed 2017-08-16 10:33:45, Sakari Ailus wrote:
> > The et8ek8 driver combines I²C register writes to a single array that it
> > passes to i2c_transfer(). The maximum number of writes is 48 at once,
> > decrease it to 8 and make more transfers if needed, thus avoiding a
> > warning on stack usage.
> 
> Dunno. Slowing down code to save stack does not sound attractive.
> 
> What about this one? Way simpler, too... (Unless there's some rule
> about i2c, DMA and static buffers. Is it?)
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
>  (untested)
> 								Pavel
> 
> diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> index f39f517..64da731 100644
> --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
> +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> @@ -227,7 +227,7 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
>  					  int cnt)
>  {
>  	struct i2c_msg msg[ET8EK8_MAX_MSG];
> -	unsigned char data[ET8EK8_MAX_MSG][6];
> +	static unsigned char data[ET8EK8_MAX_MSG][6];

Works, but we'll need to serialise calls to the function then.

I'm not really sure if passing multiple messages to i2c_transfer() really
even helps here. I think it could be removed altogether as well.

>  	int wcnt = 0;
>  	u16 reg, data_length;
>  	u32 val;
> 
> 
> 
> > ---
> > Pavel: this is just compile tested. Could you test it on N900, please?
> > 
> >  drivers/media/i2c/et8ek8/et8ek8_driver.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> > index f39f517..c14f0fd 100644
> > --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
> > +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> > @@ -43,7 +43,7 @@
> >  
> >  #define ET8EK8_NAME		"et8ek8"
> >  #define ET8EK8_PRIV_MEM_SIZE	128
> > -#define ET8EK8_MAX_MSG		48
> > +#define ET8EK8_MAX_MSG		8
> >  
> >  struct et8ek8_sensor {
> >  	struct v4l2_subdev subdev;
> > @@ -220,7 +220,8 @@ static void et8ek8_i2c_create_msg(struct i2c_client *client, u16 len, u16 reg,
> >  
> >  /*
> >   * A buffered write method that puts the wanted register write
> > - * commands in a message list and passes the list to the i2c framework
> > + * commands in smaller number of message lists and passes the lists to
> > + * the i2c framework
> >   */
> >  static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
> >  					  const struct et8ek8_reg *wnext,
> > @@ -231,11 +232,7 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
> >  	int wcnt = 0;
> >  	u16 reg, data_length;
> >  	u32 val;
> > -
> > -	if (WARN_ONCE(cnt > ET8EK8_MAX_MSG,
> > -		      ET8EK8_NAME ": %s: too many messages.\n", __func__)) {
> > -		return -EINVAL;
> > -	}
> > +	int rval;
> >  
> >  	/* Create new write messages for all writes */
> >  	while (wcnt < cnt) {
> > @@ -249,10 +246,21 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client,
> >  
> >  		/* Update write count */
> >  		wcnt++;
> > +
> > +		if (wcnt < ET8EK8_MAX_MSG)
> > +			continue;
> > +
> > +		rval = i2c_transfer(client->adapter, msg, wcnt);
> > +		if (rval < 0)
> > +			return rval;
> > +
> > +		cnt -= wcnt;
> > +		wcnt = 0;
> >  	}
> >  
> > -	/* Now we send everything ... */
> > -	return i2c_transfer(client->adapter, msg, wcnt);
> > +	rval = i2c_transfer(client->adapter, msg, wcnt);
> > +
> > +	return rval < 0 ? rval : 0;
> >  }
> >  
> >  /*
> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html



-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/1] et8ek8: Decrease stack usage

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

On Wed 2017-08-16 10:33:45, Sakari Ailus wrote:
> The et8ek8 driver combines I²C register writes to a single array that it
> passes to i2c_transfer(). The maximum number of writes is 48 at once,
> decrease it to 8 and make more transfers if needed, thus avoiding a
> warning on stack usage.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Pavel: this is just compile tested. Could you test it on N900, please?

(More than 1 et8ek8 device makes static bad idea).

Acked-by: Pavel Machek <pavel@ucw.cz>
Tested-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/1] et8ek8: Decrease stack usage

[Sakari Ailus]

On Thu, Aug 17, 2017 at 11:38:43PM +0200, Pavel Machek wrote:
> On Wed 2017-08-16 10:33:45, Sakari Ailus wrote:
> > The et8ek8 driver combines I²C register writes to a single array that it
> > passes to i2c_transfer(). The maximum number of writes is 48 at once,
> > decrease it to 8 and make more transfers if needed, thus avoiding a
> > warning on stack usage.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Pavel: this is just compile tested. Could you test it on N900, please?
> 
> (More than 1 et8ek8 device makes static bad idea).
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Tested-by: Pavel Machek <pavel@ucw.cz>

Thanks!

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi