From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail.kapsi.fi ([217.30.184.167]:45921 "EHLO mail.kapsi.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750732Ab3KERLm (ORCPT ); Tue, 5 Nov 2013 12:11:42 -0500 Message-ID: <527926CB.8070006@iki.fi> Date: Tue, 05 Nov 2013 19:11:39 +0200 From: Antti Palosaari MIME-Version: 1.0 To: Mauro Carvalho Chehab , unlisted-recipients:; CC: Linux Media Mailing List , Mauro Carvalho Chehab Subject: Re: [PATCH v3 18/29] [media] tuners: Don't use dynamic static allocation References: <1383645702-30636-1-git-send-email-m.chehab@samsung.com> <1383645702-30636-19-git-send-email-m.chehab@samsung.com> In-Reply-To: <1383645702-30636-19-git-send-email-m.chehab@samsung.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Acked-by: Antti Palosaari Reviewed-by: Antti Palosaari Antti On 05.11.2013 12:01, Mauro Carvalho Chehab wrote: > Dynamic static allocation is evil, as Kernel stack is too low, and > compilation complains about it on some archs: > drivers/media/tuners/e4000.c:50:1: warning: 'e4000_wr_regs' uses dynamic stack allocation [enabled by default] > drivers/media/tuners/e4000.c:83:1: warning: 'e4000_rd_regs' uses dynamic stack allocation [enabled by default] > drivers/media/tuners/fc2580.c:66:1: warning: 'fc2580_wr_regs.constprop.1' uses dynamic stack allocation [enabled by default] > drivers/media/tuners/fc2580.c:98:1: warning: 'fc2580_rd_regs.constprop.0' uses dynamic stack allocation [enabled by default] > drivers/media/tuners/tda18212.c:57:1: warning: 'tda18212_wr_regs' uses dynamic stack allocation [enabled by default] > drivers/media/tuners/tda18212.c:90:1: warning: 'tda18212_rd_regs.constprop.0' uses dynamic stack allocation [enabled by default] > drivers/media/tuners/tda18218.c:60:1: warning: 'tda18218_wr_regs' uses dynamic stack allocation [enabled by default] > drivers/media/tuners/tda18218.c:92:1: warning: 'tda18218_rd_regs.constprop.0' uses dynamic stack allocation [enabled by default] > > Instead, let's enforce a limit for the buffer. Considering that I2C > transfers are generally limited, and that devices used on USB has a > max data length of 64 bytes for the control URBs. > > So, it seem safe to use 64 bytes as the hard limit for all those devices. > > On most cases, the limit is a way lower than that, but this limit > is small enough to not affect the Kernel stack, and it is a no brain > limit, as using smaller ones would require to either carefully each > driver or to take a look on each datasheet. > > Signed-off-by: Mauro Carvalho Chehab > --- > drivers/media/tuners/e4000.c | 21 +++++++++++++++++++-- > drivers/media/tuners/fc2580.c | 21 +++++++++++++++++++-- > drivers/media/tuners/tda18212.c | 21 +++++++++++++++++++-- > drivers/media/tuners/tda18218.c | 21 +++++++++++++++++++-- > 4 files changed, 76 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c > index ad9309da4a91..30192463c9e1 100644 > --- a/drivers/media/tuners/e4000.c > +++ b/drivers/media/tuners/e4000.c > @@ -20,11 +20,14 @@ > > #include "e4000_priv.h" > > +/* Max transfer size done by I2C transfer functions */ > +#define MAX_XFER_SIZE 64 > + > /* write multiple registers */ > static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > { > int ret; > - u8 buf[1 + len]; > + u8 buf[MAX_XFER_SIZE]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_addr, > @@ -34,6 +37,13 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > } > }; > > + if (1 + len > sizeof(buf)) { > + dev_warn(&priv->i2c->dev, > + "%s: i2c wr reg=%04x: len=%d is too big!\n", > + KBUILD_MODNAME, reg, len); > + return -EINVAL; > + } > + > buf[0] = reg; > memcpy(&buf[1], val, len); > > @@ -53,7 +63,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > { > int ret; > - u8 buf[len]; > + u8 buf[MAX_XFER_SIZE]; > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_addr, > @@ -68,6 +78,13 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > } > }; > > + if (len > sizeof(buf)) { > + dev_warn(&priv->i2c->dev, > + "%s: i2c rd reg=%04x: len=%d is too big!\n", > + KBUILD_MODNAME, reg, len); > + return -EINVAL; > + } > + > ret = i2c_transfer(priv->i2c, msg, 2); > if (ret == 2) { > memcpy(val, buf, len); > diff --git a/drivers/media/tuners/fc2580.c b/drivers/media/tuners/fc2580.c > index 81f38aae9c66..430fa5163ec7 100644 > --- a/drivers/media/tuners/fc2580.c > +++ b/drivers/media/tuners/fc2580.c > @@ -20,6 +20,9 @@ > > #include "fc2580_priv.h" > > +/* Max transfer size done by I2C transfer functions */ > +#define MAX_XFER_SIZE 64 > + > /* > * TODO: > * I2C write and read works only for one single register. Multiple registers > @@ -41,7 +44,7 @@ > static int fc2580_wr_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len) > { > int ret; > - u8 buf[1 + len]; > + u8 buf[MAX_XFER_SIZE]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_addr, > @@ -51,6 +54,13 @@ static int fc2580_wr_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len) > } > }; > > + if (1 + len > sizeof(buf)) { > + dev_warn(&priv->i2c->dev, > + "%s: i2c wr reg=%04x: len=%d is too big!\n", > + KBUILD_MODNAME, reg, len); > + return -EINVAL; > + } > + > buf[0] = reg; > memcpy(&buf[1], val, len); > > @@ -69,7 +79,7 @@ static int fc2580_wr_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len) > static int fc2580_rd_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len) > { > int ret; > - u8 buf[len]; > + u8 buf[MAX_XFER_SIZE]; > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_addr, > @@ -84,6 +94,13 @@ static int fc2580_rd_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len) > } > }; > > + if (len > sizeof(buf)) { > + dev_warn(&priv->i2c->dev, > + "%s: i2c rd reg=%04x: len=%d is too big!\n", > + KBUILD_MODNAME, reg, len); > + return -EINVAL; > + } > + > ret = i2c_transfer(priv->i2c, msg, 2); > if (ret == 2) { > memcpy(val, buf, len); > diff --git a/drivers/media/tuners/tda18212.c b/drivers/media/tuners/tda18212.c > index e4a84ee231cf..b3a4adf9ff8f 100644 > --- a/drivers/media/tuners/tda18212.c > +++ b/drivers/media/tuners/tda18212.c > @@ -20,6 +20,9 @@ > > #include "tda18212.h" > > +/* Max transfer size done by I2C transfer functions */ > +#define MAX_XFER_SIZE 64 > + > struct tda18212_priv { > struct tda18212_config *cfg; > struct i2c_adapter *i2c; > @@ -32,7 +35,7 @@ static int tda18212_wr_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > int len) > { > int ret; > - u8 buf[len+1]; > + u8 buf[MAX_XFER_SIZE]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_address, > @@ -42,6 +45,13 @@ static int tda18212_wr_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > } > }; > > + if (1 + len > sizeof(buf)) { > + dev_warn(&priv->i2c->dev, > + "%s: i2c wr reg=%04x: len=%d is too big!\n", > + KBUILD_MODNAME, reg, len); > + return -EINVAL; > + } > + > buf[0] = reg; > memcpy(&buf[1], val, len); > > @@ -61,7 +71,7 @@ static int tda18212_rd_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > int len) > { > int ret; > - u8 buf[len]; > + u8 buf[MAX_XFER_SIZE]; > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_address, > @@ -76,6 +86,13 @@ static int tda18212_rd_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > } > }; > > + if (len > sizeof(buf)) { > + dev_warn(&priv->i2c->dev, > + "%s: i2c rd reg=%04x: len=%d is too big!\n", > + KBUILD_MODNAME, reg, len); > + return -EINVAL; > + } > + > ret = i2c_transfer(priv->i2c, msg, 2); > if (ret == 2) { > memcpy(val, buf, len); > diff --git a/drivers/media/tuners/tda18218.c b/drivers/media/tuners/tda18218.c > index 2d31aeb6b088..7e2b32ee5349 100644 > --- a/drivers/media/tuners/tda18218.c > +++ b/drivers/media/tuners/tda18218.c > @@ -20,11 +20,14 @@ > > #include "tda18218_priv.h" > > +/* Max transfer size done by I2C transfer functions */ > +#define MAX_XFER_SIZE 64 > + > /* write multiple registers */ > static int tda18218_wr_regs(struct tda18218_priv *priv, u8 reg, u8 *val, u8 len) > { > int ret = 0, len2, remaining; > - u8 buf[1 + len]; > + u8 buf[MAX_XFER_SIZE]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_address, > @@ -33,6 +36,13 @@ static int tda18218_wr_regs(struct tda18218_priv *priv, u8 reg, u8 *val, u8 len) > } > }; > > + if (1 + len > sizeof(buf)) { > + dev_warn(&priv->i2c->dev, > + "%s: i2c wr reg=%04x: len=%d is too big!\n", > + KBUILD_MODNAME, reg, len); > + return -EINVAL; > + } > + > for (remaining = len; remaining > 0; > remaining -= (priv->cfg->i2c_wr_max - 1)) { > len2 = remaining; > @@ -63,7 +73,7 @@ static int tda18218_wr_regs(struct tda18218_priv *priv, u8 reg, u8 *val, u8 len) > static int tda18218_rd_regs(struct tda18218_priv *priv, u8 reg, u8 *val, u8 len) > { > int ret; > - u8 buf[reg+len]; /* we must start read always from reg 0x00 */ > + u8 buf[MAX_XFER_SIZE]; /* we must start read always from reg 0x00 */ > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_address, > @@ -78,6 +88,13 @@ static int tda18218_rd_regs(struct tda18218_priv *priv, u8 reg, u8 *val, u8 len) > } > }; > > + if (reg + len > sizeof(buf)) { > + dev_warn(&priv->i2c->dev, > + "%s: i2c wr reg=%04x: len=%d is too big!\n", > + KBUILD_MODNAME, reg, len); > + return -EINVAL; > + } > + > ret = i2c_transfer(priv->i2c, msg, 2); > if (ret == 2) { > memcpy(val, &buf[reg], len); > -- http://palosaari.fi/