Re: [PATCH] checkstack fixes for drivers/media/dvb

2010-05-12 Thread Mauro Carvalho Chehab
Prarit Bhargava wrote:
 When compiling 2.6.34-rc7 I see the following warnings
 
 drivers/media/dvb/frontends/dib3000mc.c: In function 
 'dib3000mc_i2c_enumeration':
 drivers/media/dvb/frontends/dib3000mc.c:853: warning: the frame size of 2224 
 bytes is larger than 2048 bytes
 drivers/media/dvb/frontends/dib7000p.c: In function 
 'dib7000p_i2c_enumeration':
 drivers/media/dvb/frontends/dib7000p.c:1346: warning: the frame size of 2304 
 bytes is larger than 2048 bytes
 
 because the dib*_state structs are large and they are alloc'd on the stack.
 
 This patch moves the structures off the stack.

Hi Prarit,

Thanks for the patch, but I've received two patches to fix the same thing some 
time ago.
Unfortunately, it took a long time to be merged, since I was waiting for the 
driver
maintainer's ack. It is at those changesets:

http://git.linuxtv.org/v4l-dvb.git?a=commit;h=65483f7e5f3e169ea038de26068395231dd3b13b
http://git.linuxtv.org/v4l-dvb.git?a=commit;h=370c0cb185d4fccfb2c66fbe94b48579d4c5fa1c

 
 I also noticed that the cxusb driver doesn't check the return value from
 dib7000p_i2c_enumeration().

Randy's patch also added a test for it, but without the warning printk. It may 
be a good
idea to have that warning. So, please be free to send it as a separate patch if 
you also
think so.
 
 Signed-off-by: Prarit Bhargava pra...@redhat.com
 
 diff --git a/drivers/media/dvb/dvb-usb/cxusb.c 
 b/drivers/media/dvb/dvb-usb/cxusb.c
 index 960376d..8141b10 100644
 --- a/drivers/media/dvb/dvb-usb/cxusb.c
 +++ b/drivers/media/dvb/dvb-usb/cxusb.c
 @@ -1025,8 +1025,11 @@ static int cxusb_dualdig4_rev2_frontend_attach(struct 
 dvb_usb_adapter *adap)
  
   cxusb_bluebird_gpio_pulse(adap-dev, 0x02, 1);
  
 - dib7000p_i2c_enumeration(adap-dev-i2c_adap, 1, 18,
 -  cxusb_dualdig4_rev2_config);
 + if (dib7000p_i2c_enumeration(adap-dev-i2c_adap, 1, 18,
 +  cxusb_dualdig4_rev2_config)) {
 + printk(KERN_WARNING Unable to enumerate dib7000p\n);
 + return -ENODEV;
 + }
  
   adap-fe = dvb_attach(dib7000p_attach, adap-dev-i2c_adap, 0x80,
 cxusb_dualdig4_rev2_config);
 diff --git a/drivers/media/dvb/frontends/dib3000mc.c 
 b/drivers/media/dvb/frontends/dib3000mc.c
 index 40a0998..6a178f1 100644
 --- a/drivers/media/dvb/frontends/dib3000mc.c
 +++ b/drivers/media/dvb/frontends/dib3000mc.c
 @@ -814,42 +814,52 @@ EXPORT_SYMBOL(dib3000mc_set_config);
  
  int dib3000mc_i2c_enumeration(struct i2c_adapter *i2c, int no_of_demods, u8 
 default_addr, struct dib3000mc_config cfg[])
  {
 - struct dib3000mc_state st = { .i2c_adap = i2c };
 + struct dib3000mc_state *st;
   int k;
   u8 new_addr;
 -
   static u8 DIB3000MC_I2C_ADDRESS[] = {20,22,24,26};
  
 + st = kzalloc(sizeof(*st), GFP_KERNEL);
 + if (!st)
 + return -ENOMEM;
 + st-i2c_adap = i2c;
 +
   for (k = no_of_demods-1; k = 0; k--) {
 - st.cfg = cfg[k];
 + st-cfg = cfg[k];
  
   /* designated i2c address */
 - new_addr  = DIB3000MC_I2C_ADDRESS[k];
 - st.i2c_addr = new_addr;
 - if (dib3000mc_identify(st) != 0) {
 - st.i2c_addr = default_addr;
 - if (dib3000mc_identify(st) != 0) {
 - dprintk(-E-  DiB3000P/MC #%d: not 
 identified\n, k);
 + new_addr = DIB3000MC_I2C_ADDRESS[k];
 + st-i2c_addr = new_addr;
 + if (dib3000mc_identify(st) != 0) {
 + st-i2c_addr = default_addr;
 + if (dib3000mc_identify(st) != 0) {
 + dprintk(-E-  DiB3000P/MC #%d: not
 +  identified\n, k);
 + kfree(st);
   return -ENODEV;
   }
   }
  
 - dib3000mc_set_output_mode(st, OUTMODE_MPEG2_PAR_CONT_CLK);
 + dib3000mc_set_output_mode(st, OUTMODE_MPEG2_PAR_CONT_CLK);
  
 - // set new i2c address and force divstr (Bit 1) to value 0 (Bit 
 0)
 - dib3000mc_write_word(st, 1024, (new_addr  3) | 0x1);
 - st.i2c_addr = new_addr;
 + /* set new i2c address and force divstr (Bit 1) to
 +  * value 0 (Bit 0)
 +  */
 + dib3000mc_write_word(st, 1024, (new_addr  3) | 0x1);
 + st-i2c_addr = new_addr;
   }
  
   for (k = 0; k  no_of_demods; k++) {
 - st.cfg = cfg[k];
 - st.i2c_addr = DIB3000MC_I2C_ADDRESS[k];
 + st-cfg = cfg[k];
 + st-i2c_addr = DIB3000MC_I2C_ADDRESS[k];
  
 - dib3000mc_write_word(st, 1024, st.i2c_addr  3);
 + dib3000mc_write_word(st, 1024, st-i2c_addr  3);
  
   /* turn off data output */
 - dib3000mc_set_output_mode(st, OUTMODE_HIGH_Z);
 + 

Re: [PATCH] checkstack fixes for drivers/media/dvb

2010-05-12 Thread Prarit Bhargava



On 05/12/2010 04:39 PM, Mauro Carvalho Chehab wrote:

Prarit Bhargava wrote:
   

When compiling 2.6.34-rc7 I see the following warnings

drivers/media/dvb/frontends/dib3000mc.c: In function 
'dib3000mc_i2c_enumeration':
drivers/media/dvb/frontends/dib3000mc.c:853: warning: the frame size of 2224 
bytes is larger than 2048 bytes
drivers/media/dvb/frontends/dib7000p.c: In function 'dib7000p_i2c_enumeration':
drivers/media/dvb/frontends/dib7000p.c:1346: warning: the frame size of 2304 
bytes is larger than 2048 bytes

because the dib*_state structs are large and they are alloc'd on the stack.

This patch moves the structures off the stack.
 

Hi Prarit,

Thanks for the patch, but I've received two patches to fix the same thing some 
time ago.
Unfortunately, it took a long time to be merged, since I was waiting for the 
driver
maintainer's ack. It is at those changesets:

http://git.linuxtv.org/v4l-dvb.git?a=commit;h=65483f7e5f3e169ea038de26068395231dd3b13b
http://git.linuxtv.org/v4l-dvb.git?a=commit;h=370c0cb185d4fccfb2c66fbe94b48579d4c5fa1c

   


Oops!  Sorry about that Mauro :( -- I didn't realize there was another 
git tree to check.  Just curious -- is the one listed in MAINTAINERS 
still active?



I also noticed that the cxusb driver doesn't check the return value from
dib7000p_i2c_enumeration().
 

Randy's patch also added a test for it, but without the warning printk. It may 
be a good
idea to have that warning. So, please be free to send it as a separate patch if 
you also
think so.
   



Sure I'll do that shortly (after checking out the above tree ;) ).

P.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html