Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-11-20 Thread Sean Young
On Mon, Nov 17, 2014 at 12:59:09PM -0200, Mauro Carvalho Chehab wrote:
> Em Sun, 9 Nov 2014 21:35:17 +
> Sean Young  escreveu:
> 
> > On Thu, Nov 06, 2014 at 08:56:47AM -0500, Andy Walls wrote:
> > > On November 6, 2014 8:54:28 AM EST, Andy Walls  
> > > wrote:
> > > >Sean,
> > > >
> > > >Ir-kbd-i2c was never intended for Tx.
> > > >
> > > >You can transmit *short* arbitrary pulse-space streams with the zilog
> > > >chip, by feeding it a parameter block that has the pulse timing
> > > >information and then subsequently has been obfuscated.  The firmware
> > > >file that LIRC uses in userspace is full of predefined versions of
> > > >these things for RC5 and NEC IIRC.  This LIRC firmware file also holds
> > > >the (de)obfuscation key.
> > > >
> > > >I've got a bunch of old notes on this stuff from essentially reverse
> > > >engineering the firmware in the Z8.  IANAL, but to me, its use in
> > > >developing in-kernel stuff could be dubious.
> > > >
> > > >Regards,
> > > >Andy
> > 
> > Very interesting.
> > 
> > I had considered reverse engineering the z8 firmware but I never found a
> > way to access it. I guess we have three options:
> > 
> > 1. I could use Andy's notes to implement Tx. I have not seen the original
> >firmware code so I'm not contaminated by reverse engineering it. IANAL 
> >but I thought this is an acceptable way of writing a driver.
> > 
> > 2. Hauppauge could prove us with documentation to write a driver with.
> 
> I tried to get some info about that, but they are unable to get anything
> related to this design so far.
> 
> So, I think that, if you have some time to dedicate to it, the best would
> be to go for  option #1.

Ok, thanks for asking.

Andy -- if you please send your notes please, I can work on implementing
the driver. I have hardware and time for to work on this.

Thanks

Sean
--
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


Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-11-17 Thread Mauro Carvalho Chehab
Em Sun, 9 Nov 2014 21:35:17 +
Sean Young  escreveu:

> On Thu, Nov 06, 2014 at 08:56:47AM -0500, Andy Walls wrote:
> > On November 6, 2014 8:54:28 AM EST, Andy Walls  
> > wrote:
> > >Sean,
> > >
> > >Ir-kbd-i2c was never intended for Tx.
> > >
> > >You can transmit *short* arbitrary pulse-space streams with the zilog
> > >chip, by feeding it a parameter block that has the pulse timing
> > >information and then subsequently has been obfuscated.  The firmware
> > >file that LIRC uses in userspace is full of predefined versions of
> > >these things for RC5 and NEC IIRC.  This LIRC firmware file also holds
> > >the (de)obfuscation key.
> > >
> > >I've got a bunch of old notes on this stuff from essentially reverse
> > >engineering the firmware in the Z8.  IANAL, but to me, its use in
> > >developing in-kernel stuff could be dubious.
> > >
> > >Regards,
> > >Andy
> 
> Very interesting.
> 
> I had considered reverse engineering the z8 firmware but I never found a
> way to access it. I guess we have three options:
> 
> 1. I could use Andy's notes to implement Tx. I have not seen the original
>firmware code so I'm not contaminated by reverse engineering it. IANAL 
>but I thought this is an acceptable way of writing a driver.
> 
> 2. Hauppauge could prove us with documentation to write a driver with.

I tried to get some info about that, but they are unable to get anything
related to this design so far.

So, I think that, if you have some time to dedicate to it, the best would
be to go for  option #1.

> 3. Leave it as-is, lirc_zilog will eventually be deleted from staging as it
>can't be ported to rc-core.
> 
> 
> Sean
> --
> 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
--
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


Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-11-09 Thread Sean Young
On Thu, Nov 06, 2014 at 08:56:47AM -0500, Andy Walls wrote:
> On November 6, 2014 8:54:28 AM EST, Andy Walls  wrote:
> >Sean,
> >
> >Ir-kbd-i2c was never intended for Tx.
> >
> >You can transmit *short* arbitrary pulse-space streams with the zilog
> >chip, by feeding it a parameter block that has the pulse timing
> >information and then subsequently has been obfuscated.  The firmware
> >file that LIRC uses in userspace is full of predefined versions of
> >these things for RC5 and NEC IIRC.  This LIRC firmware file also holds
> >the (de)obfuscation key.
> >
> >I've got a bunch of old notes on this stuff from essentially reverse
> >engineering the firmware in the Z8.  IANAL, but to me, its use in
> >developing in-kernel stuff could be dubious.
> >
> >Regards,
> >Andy

Very interesting.

I had considered reverse engineering the z8 firmware but I never found a
way to access it. I guess we have three options:

1. I could use Andy's notes to implement Tx. I have not seen the original
   firmware code so I'm not contaminated by reverse engineering it. IANAL 
   but I thought this is an acceptable way of writing a driver.

2. Hauppauge could prove us with documentation to write a driver with.

3. Leave it as-is, lirc_zilog will eventually be deleted from staging as it
   can't be ported to rc-core.


Sean
--
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


Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-11-06 Thread Andy Walls
On November 6, 2014 8:54:28 AM EST, Andy Walls  wrote:
>On November 6, 2014 8:21:13 AM EST, Sean Young  wrote:
>>On Thu, Nov 06, 2014 at 11:05:49AM -0200, Mauro Carvalho Chehab wrote:
>>> Hi Sean,
>>> 
>>> Em Thu, 06 Nov 2014 12:46:29 +
>>> Sean Young  escreveu:
>>> 
>>> > On Fri, Oct 31, 2014 at 05:35:41PM +0300, Dan Carpenter wrote:
>>> > > On Fri, Oct 31, 2014 at 04:26:45PM +0200, Aya Mahfouz wrote:
>>> > > > On Fri, Oct 31, 2014 at 04:06:00PM +0300, Dan Carpenter wrote:
>>> > > > > drivers/staging/media/lirc/lirc_zilog.c
>>> > > > >   1333  /* Close the IR device */
>>> > > > >   1334  static int close(struct inode *node, struct file
>>*filep)
>>> > > > >   1335  {
>>> > > > >   1336  /* find our IR struct */
>>> > > > >   1337  struct IR *ir = filep->private_data;
>>> > > > >   1338  
>>> > > > >   1339  if (ir == NULL) {
>>> > > > > ^^
>>> > > > >   1340  dev_err(ir->l.dev, "close: no
>>private_data attached to the file!\n");
>>> > > > > ^
>>> > > > > 
>>> > > > > I suggest you just delete the error message.  Can "ir"
>>actually be NULL
>>> > > > > here anyway?
>>> > > > >
>>> > > > 
>>> > > > Since I'm a newbie and this is not my code, I prefer to use
>>pr_err().
>>> > > 
>>> > > This driver doesn't belong to anyone.  Go ahead and take
>>ownership.  The
>>> > > message is fairly worthless and no one will miss it.
>>> > 
>>> > Speaking of ownership, what this driver really needs is to be
>>ported to 
>>> > rc-core. In order to do this it'll need to be able to send raw IR
>>rather
>>> > key codes; I've been peering at the firmware but it neither looks
>>like
>>> > zilog z8 opcodes nor space/pulse information.
>>> 
>>> Actually, I think that all features provided by this driver were
>>already
>>> migrated into the ir-kbd-i2c (drivers/media/i2c/ir-kbd-i2c.c)
>driver.
>>
>>All the features for the IR receiver are implemented (very nicely) in
>>ir-kbd-i2c (the ir_rx_z8f0811_haup and ir_rx_z8f0811_hdpvr i2c
>>drivers).
>>
>>However the IR emitter (i2c driver ir_tx_z8f0811_haup and 
>>ir_tx_z8f0811_hdpvr) are not implemented there. I wanted to port the 
>>IR emitter but the driver can only send specific rc5 (IIRC) keycodes,
>>not
>>raw IR. So I cannot port it.
>>
>>> Andy and Jarod worked on this conversion, but we decided, on that
>>time,
>>> to keep lirc_zilog for a while (can't remember why).
>>> 
>>> Andy/Jarod,
>>> 
>>> What's the status of the ir-kbd-i2c with regards to Zilog z8
>support?
>>
>>Transmitter side.
>>
>>> > Does anyone have any contacts at Hauppauge who could help with
>>this?
>>> 
>>> Probably, it won't be easy to get someone there that worked on it,
>>> as this device is too old.
>>> 
>>> Anyway, if are there anything still pending, I may be able to get
>>> some contacts at the vendor.
>>
>>That would be great, I have hardware and I'm happy to re-write the
>>driver.
>>
>>Thanks
>>
>>Sean
>>--
>>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
>
>Sean,
>
>Ir-kbd-i2c was never intended for Tx.
>
>You can transmit *short* arbitrary pulse-space streams with the zilog
>chip, by feeding it a parameter block that has the pulse timing
>information and then subsequently has been obfuscated.  The firmware
>file that LIRC uses in userspace is full of predefined versions of
>these things for RC5 and NEC IIRC.  This LIRC firmware file also holds
>the (de)obfuscation key.
>
>I've got a bunch of old notes on this stuff from essentially reverse
>engineering the firmware in the Z8.  IANAL, but to me, its use in
>developing in-kernel stuff could be dubious.
>
>Regards,
>Andy

Resending as plain text for mailing list.
--
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


Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-11-06 Thread Sean Young
On Thu, Nov 06, 2014 at 11:05:49AM -0200, Mauro Carvalho Chehab wrote:
> Hi Sean,
> 
> Em Thu, 06 Nov 2014 12:46:29 +
> Sean Young  escreveu:
> 
> > On Fri, Oct 31, 2014 at 05:35:41PM +0300, Dan Carpenter wrote:
> > > On Fri, Oct 31, 2014 at 04:26:45PM +0200, Aya Mahfouz wrote:
> > > > On Fri, Oct 31, 2014 at 04:06:00PM +0300, Dan Carpenter wrote:
> > > > > drivers/staging/media/lirc/lirc_zilog.c
> > > > >   1333  /* Close the IR device */
> > > > >   1334  static int close(struct inode *node, struct file *filep)
> > > > >   1335  {
> > > > >   1336  /* find our IR struct */
> > > > >   1337  struct IR *ir = filep->private_data;
> > > > >   1338  
> > > > >   1339  if (ir == NULL) {
> > > > > ^^
> > > > >   1340  dev_err(ir->l.dev, "close: no private_data 
> > > > > attached to the file!\n");
> > > > > ^
> > > > > 
> > > > > I suggest you just delete the error message.  Can "ir" actually be 
> > > > > NULL
> > > > > here anyway?
> > > > >
> > > > 
> > > > Since I'm a newbie and this is not my code, I prefer to use pr_err().
> > > 
> > > This driver doesn't belong to anyone.  Go ahead and take ownership.  The
> > > message is fairly worthless and no one will miss it.
> > 
> > Speaking of ownership, what this driver really needs is to be ported to 
> > rc-core. In order to do this it'll need to be able to send raw IR rather
> > key codes; I've been peering at the firmware but it neither looks like
> > zilog z8 opcodes nor space/pulse information.
> 
> Actually, I think that all features provided by this driver were already
> migrated into the ir-kbd-i2c (drivers/media/i2c/ir-kbd-i2c.c) driver.

All the features for the IR receiver are implemented (very nicely) in
ir-kbd-i2c (the ir_rx_z8f0811_haup and ir_rx_z8f0811_hdpvr i2c drivers).

However the IR emitter (i2c driver ir_tx_z8f0811_haup and 
ir_tx_z8f0811_hdpvr) are not implemented there. I wanted to port the 
IR emitter but the driver can only send specific rc5 (IIRC) keycodes, not
raw IR. So I cannot port it.

> Andy and Jarod worked on this conversion, but we decided, on that time,
> to keep lirc_zilog for a while (can't remember why).
> 
> Andy/Jarod,
> 
> What's the status of the ir-kbd-i2c with regards to Zilog z8 support?

Transmitter side.

> > Does anyone have any contacts at Hauppauge who could help with this?
> 
> Probably, it won't be easy to get someone there that worked on it,
> as this device is too old.
> 
> Anyway, if are there anything still pending, I may be able to get
> some contacts at the vendor.

That would be great, I have hardware and I'm happy to re-write the
driver.

Thanks

Sean
--
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


Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-11-06 Thread Mauro Carvalho Chehab
Hi Sean,

Em Thu, 06 Nov 2014 12:46:29 +
Sean Young  escreveu:

> On Fri, Oct 31, 2014 at 05:35:41PM +0300, Dan Carpenter wrote:
> > On Fri, Oct 31, 2014 at 04:26:45PM +0200, Aya Mahfouz wrote:
> > > On Fri, Oct 31, 2014 at 04:06:00PM +0300, Dan Carpenter wrote:
> > > > drivers/staging/media/lirc/lirc_zilog.c
> > > >   1333  /* Close the IR device */
> > > >   1334  static int close(struct inode *node, struct file *filep)
> > > >   1335  {
> > > >   1336  /* find our IR struct */
> > > >   1337  struct IR *ir = filep->private_data;
> > > >   1338  
> > > >   1339  if (ir == NULL) {
> > > > ^^
> > > >   1340  dev_err(ir->l.dev, "close: no private_data 
> > > > attached to the file!\n");
> > > > ^
> > > > 
> > > > I suggest you just delete the error message.  Can "ir" actually be NULL
> > > > here anyway?
> > > >
> > > 
> > > Since I'm a newbie and this is not my code, I prefer to use pr_err().
> > 
> > This driver doesn't belong to anyone.  Go ahead and take ownership.  The
> > message is fairly worthless and no one will miss it.
> 
> Speaking of ownership, what this driver really needs is to be ported to 
> rc-core. In order to do this it'll need to be able to send raw IR rather
> key codes; I've been peering at the firmware but it neither looks like
> zilog z8 opcodes nor space/pulse information.

Actually, I think that all features provided by this driver were already
migrated into the ir-kbd-i2c (drivers/media/i2c/ir-kbd-i2c.c) driver.

Andy and Jarod worked on this conversion, but we decided, on that time,
to keep lirc_zilog for a while (can't remember why).

Andy/Jarod,

What's the status of the ir-kbd-i2c with regards to Zilog z8 support?

> Does anyone have any contacts at Hauppauge who could help with this?

Probably, it won't be easy to get someone there that worked on it,
as this device is too old.

Anyway, if are there anything still pending, I may be able to get
some contacts at the vendor.

Regards,
Mauro
--
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


Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-11-06 Thread Sean Young
On Fri, Oct 31, 2014 at 05:35:41PM +0300, Dan Carpenter wrote:
> On Fri, Oct 31, 2014 at 04:26:45PM +0200, Aya Mahfouz wrote:
> > On Fri, Oct 31, 2014 at 04:06:00PM +0300, Dan Carpenter wrote:
> > > drivers/staging/media/lirc/lirc_zilog.c
> > >   1333  /* Close the IR device */
> > >   1334  static int close(struct inode *node, struct file *filep)
> > >   1335  {
> > >   1336  /* find our IR struct */
> > >   1337  struct IR *ir = filep->private_data;
> > >   1338  
> > >   1339  if (ir == NULL) {
> > > ^^
> > >   1340  dev_err(ir->l.dev, "close: no private_data 
> > > attached to the file!\n");
> > > ^
> > > 
> > > I suggest you just delete the error message.  Can "ir" actually be NULL
> > > here anyway?
> > >
> > 
> > Since I'm a newbie and this is not my code, I prefer to use pr_err().
> 
> This driver doesn't belong to anyone.  Go ahead and take ownership.  The
> message is fairly worthless and no one will miss it.

Speaking of ownership, what this driver really needs is to be ported to 
rc-core. In order to do this it'll need to be able to send raw IR rather
key codes; I've been peering at the firmware but it neither looks like
zilog z8 opcodes nor space/pulse information.

Does anyone have any contacts at Hauppauge who could help with this?

Thanks,

Sean
--
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


Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-11-01 Thread Aya Mahfouz
On Fri, Oct 31, 2014 at 05:35:41PM +0300, Dan Carpenter wrote:
> On Fri, Oct 31, 2014 at 04:26:45PM +0200, Aya Mahfouz wrote:
> > On Fri, Oct 31, 2014 at 04:06:00PM +0300, Dan Carpenter wrote:
> > > drivers/staging/media/lirc/lirc_zilog.c
> > >   1333  /* Close the IR device */
> > >   1334  static int close(struct inode *node, struct file *filep)
> > >   1335  {
> > >   1336  /* find our IR struct */
> > >   1337  struct IR *ir = filep->private_data;
> > >   1338  
> > >   1339  if (ir == NULL) {
> > > ^^
> > >   1340  dev_err(ir->l.dev, "close: no private_data 
> > > attached to the file!\n");
> > > ^
> > > 
> > > I suggest you just delete the error message.  Can "ir" actually be NULL
> > > here anyway?
> > >
> > 
> > Since I'm a newbie and this is not my code, I prefer to use pr_err().
> 
> This driver doesn't belong to anyone.  Go ahead and take ownership.  The
> message is fairly worthless and no one will miss it.
> 
ok.
> > 
> > In general, I can send a new patch to fix the aforementioned warnings.
> > Kindly let me know if you prefer that I send a second version of this
> > patch.
> 
> No.  The first patch was already applied so send a new patch.
> 
I will fix the static errors that my patch caused. The warning concerning
the double free will require rewriting some parts of the function and was
not caused by my patch. I have a couple of ideas in mind but I need
sometime to apply them. Greg too is not happy about the coding style of
this driver in general.

> regards,
> dan carpenter
> 

Kind Regards,
Aya Saif El-yazal Mahfouz
--
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


Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-10-31 Thread Dan Carpenter
On Fri, Oct 31, 2014 at 04:26:45PM +0200, Aya Mahfouz wrote:
> On Fri, Oct 31, 2014 at 04:06:00PM +0300, Dan Carpenter wrote:
> > drivers/staging/media/lirc/lirc_zilog.c
> >   1333  /* Close the IR device */
> >   1334  static int close(struct inode *node, struct file *filep)
> >   1335  {
> >   1336  /* find our IR struct */
> >   1337  struct IR *ir = filep->private_data;
> >   1338  
> >   1339  if (ir == NULL) {
> > ^^
> >   1340  dev_err(ir->l.dev, "close: no private_data attached 
> > to the file!\n");
> > ^
> > 
> > I suggest you just delete the error message.  Can "ir" actually be NULL
> > here anyway?
> >
> 
> Since I'm a newbie and this is not my code, I prefer to use pr_err().

This driver doesn't belong to anyone.  Go ahead and take ownership.  The
message is fairly worthless and no one will miss it.

> 
> In general, I can send a new patch to fix the aforementioned warnings.
> Kindly let me know if you prefer that I send a second version of this
> patch.

No.  The first patch was already applied so send a new patch.

regards,
dan carpenter

--
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


Re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-10-31 Thread Aya Mahfouz
On Fri, Oct 31, 2014 at 04:06:00PM +0300, Dan Carpenter wrote:
> Hello Aya Mahfouz,
> 

Hello Dan, 

> The patch be4aa8157c98: "staging: media: lirc: lirc_zilog.c: replace
> custom print macros with dev_* and pr_*" from Oct 26, 2014, leads to
> the following static checker warning:
> 
>   drivers/staging/media/lirc/lirc_zilog.c:1340 close()
>   error: we previously assumed 'ir' could be null (see line 1339)
> 
> drivers/staging/media/lirc/lirc_zilog.c
>   1333  /* Close the IR device */
>   1334  static int close(struct inode *node, struct file *filep)
>   1335  {
>   1336  /* find our IR struct */
>   1337  struct IR *ir = filep->private_data;
>   1338  
>   1339  if (ir == NULL) {
> ^^
>   1340  dev_err(ir->l.dev, "close: no private_data attached 
> to the file!\n");
> ^
> 
> I suggest you just delete the error message.  Can "ir" actually be NULL
> here anyway?
>

Since I'm a newbie and this is not my code, I prefer to use pr_err().
 
>   1341  return -ENODEV;
>   1342  }
>   1343  
> 
>   drivers/staging/media/lirc/lirc_zilog.c:1636 ir_probe()
>   error: potential null dereference 'ir'.  (kzalloc returns null)
> 
> drivers/staging/media/lirc/lirc_zilog.c
>   1614  dev_info(ir->l.dev, "IR unit on %s (i2c-%d) registered as 
> lirc%d and ready\n",
>   1615 adap->name, adap->nr, ir->l.minor);
>   1616  
>   1617  out_ok:
>   1618  if (rx != NULL)
>   1619  put_ir_rx(rx, true);
>   1620  if (tx != NULL)
>   1621  put_ir_tx(tx, true);
>   1622  put_ir_device(ir, true);
>   1623  dev_info(ir->l.dev, "probe of IR %s on %s (i2c-%d) done\n",
>   1624 tx_probe ? "Tx" : "Rx", adap->name, adap->nr);
>   1625  mutex_unlock(&ir_devices_lock);
>   1626  return 0;
>   1627  
>   1628  out_put_xx:
>   1629  if (rx != NULL)
>   1630  put_ir_rx(rx, true);
> ^^^
> Btw, I think this is a double free?  On some paths we call put_ir_rx()
> before the goto out_put_xx;.
> 

I'll read about this and see how to fix it.

>   1631  if (tx != NULL)
>   1632  put_ir_tx(tx, true);
>   1633  out_put_ir:
>   1634  put_ir_device(ir, true);
>   1635  out_no_ir:
>   1636  dev_err(ir->l.dev, "%s: probing IR %s on %s (i2c-%d) failed 
> with %d\n",
> ^
> Null dereference.
> 
>   1637  __func__, tx_probe ? "Tx" : "Rx", adap->name, 
> adap->nr,
>   1638 ret);
>   1639  mutex_unlock(&ir_devices_lock);
>   1640  return ret;
>   1641  }
> 

In general, I can send a new patch to fix the aforementioned warnings.
Kindly let me know if you prefer that I send a second version of this
patch.

> regards,
> dan carpenter

Kind Regards,
Aya Saif El-yazal Mahfouz
--
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


re: staging: media: lirc: lirc_zilog.c: replace custom print macros with dev_* and pr_*

2014-10-31 Thread Dan Carpenter
Hello Aya Mahfouz,

The patch be4aa8157c98: "staging: media: lirc: lirc_zilog.c: replace
custom print macros with dev_* and pr_*" from Oct 26, 2014, leads to
the following static checker warning:

drivers/staging/media/lirc/lirc_zilog.c:1340 close()
error: we previously assumed 'ir' could be null (see line 1339)

drivers/staging/media/lirc/lirc_zilog.c
  1333  /* Close the IR device */
  1334  static int close(struct inode *node, struct file *filep)
  1335  {
  1336  /* find our IR struct */
  1337  struct IR *ir = filep->private_data;
  1338  
  1339  if (ir == NULL) {
^^
  1340  dev_err(ir->l.dev, "close: no private_data attached to 
the file!\n");
^

I suggest you just delete the error message.  Can "ir" actually be NULL
here anyway?

  1341  return -ENODEV;
  1342  }
  1343  

drivers/staging/media/lirc/lirc_zilog.c:1636 ir_probe()
error: potential null dereference 'ir'.  (kzalloc returns null)

drivers/staging/media/lirc/lirc_zilog.c
  1614  dev_info(ir->l.dev, "IR unit on %s (i2c-%d) registered as 
lirc%d and ready\n",
  1615 adap->name, adap->nr, ir->l.minor);
  1616  
  1617  out_ok:
  1618  if (rx != NULL)
  1619  put_ir_rx(rx, true);
  1620  if (tx != NULL)
  1621  put_ir_tx(tx, true);
  1622  put_ir_device(ir, true);
  1623  dev_info(ir->l.dev, "probe of IR %s on %s (i2c-%d) done\n",
  1624 tx_probe ? "Tx" : "Rx", adap->name, adap->nr);
  1625  mutex_unlock(&ir_devices_lock);
  1626  return 0;
  1627  
  1628  out_put_xx:
  1629  if (rx != NULL)
  1630  put_ir_rx(rx, true);
^^^
Btw, I think this is a double free?  On some paths we call put_ir_rx()
before the goto out_put_xx;.

  1631  if (tx != NULL)
  1632  put_ir_tx(tx, true);
  1633  out_put_ir:
  1634  put_ir_device(ir, true);
  1635  out_no_ir:
  1636  dev_err(ir->l.dev, "%s: probing IR %s on %s (i2c-%d) failed 
with %d\n",
^
Null dereference.

  1637  __func__, tx_probe ? "Tx" : "Rx", adap->name, 
adap->nr,
  1638 ret);
  1639  mutex_unlock(&ir_devices_lock);
  1640  return ret;
  1641  }

regards,
dan carpenter
--
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