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

DISCUSSION Dan Carpenter 5 participants 11 messages Oct 31, 2014 – Nov 20, 2014 1 message not in the archive Media (V4L/DVB)

lore

Share & tools

lore

Dan Carpenter

author

2014-10-31 13:06 UTC #0 lore raw

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

Aya Mahfouz

2014-10-31 14:26 UTC #1 lore raw

Quoted 2 lines · Dan Carpenter

Hello Aya Mahfouz,

Hello Dan,

Quoted 19 lines

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");
                                ^^^^^^^^^

Quoted 3 lines

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().

Quoted 24 lines

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)

Quoted 5 lines

  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.

Quoted 6 lines

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

Quoted 9 lines

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

Quoted 2 lines

regards,
dan carpenter

Kind Regards,
Aya Saif El-yazal Mahfouz

Dan Carpenter

author

2014-10-31 14:35 UTC #2 lore raw

Quoted 13 lines · Aya Mahfouz

Quoted 12 lines · Dan Carpenter

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");
                                ^^^^^^^^^

Quoted 5 lines

Quoted 3 lines
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.

Quoted 4 lines

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

Aya Mahfouz

2014-11-01 20:05 UTC #3 lore raw

Quoted 14 lines · Dan Carpenter

Quoted 13 lines · Aya Mahfouz

Quoted 12 lines · Dan Carpenter

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");
                                ^^^^^^^^^

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 3 lines

regards,
dan carpenter

Kind Regards,
Aya Saif El-yazal Mahfouz

Sean Young

2014-11-06 12:46 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 14 lines · Dan Carpenter

Quoted 13 lines · Aya Mahfouz

Quoted 12 lines · Dan Carpenter

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");
                                ^^^^^^^^^

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
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

Mauro Carvalho Chehab

2014-11-06 13:05 UTC #5 lore raw

Hi Sean,

Em Thu, 06 Nov 2014 12:46:29 +0000
Sean Young <[email protected]> escreveu:

Quoted 21 lines · Dan Carpenter

Quoted 20 lines · Aya Mahfouz
Quoted 18 lines · Dan Carpenter
Quoted 12 lines
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");
                                ^^^^^^^^^
Quoted 3 lines
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().

Quoted 7 lines

Quoted 2 lines
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?

Quoted 1 line

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

Sean Young

2014-11-06 13:21 UTC #6 lore raw

Quoted 29 lines · Mauro Carvalho Chehab

Hi Sean,
Em Thu, 06 Nov 2014 12:46:29 +0000
Sean Young <[email protected]> escreveu:
Quoted 24 lines · Dan Carpenter
Quoted 22 lines · Aya Mahfouz
Quoted 13 lines · Dan Carpenter
Quoted 12 lines
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");
                                ^^^^^^^^^
Quoted 5 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 1 line
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

Andy Walls

2014-11-06 13:56 UTC #8 lore raw

In reply to a message not in the archive

Quoted 102 lines · Andy Walls

Quoted 37 lines · Sean Young
Quoted 11 lines · Mauro Carvalho Chehab
Hi Sean,
Em Thu, 06 Nov 2014 12:46:29 +0000
Sean Young <[email protected]> escreveu:
Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Aya Mahfouz
Quoted 4 lines · Dan Carpenter
Quoted 3 lines
drivers/staging/media/lirc/lirc_zilog.c
1333 /* Close the IR device */
1334 static int close(struct inode *node, struct file
*filep)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
  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");
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
                                ^^^^^^^^^
I suggest you just delete the error message. Can "ir"
actually be NULL
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
here anyway?
Since I'm a newbie and this is not my code, I prefer to use
pr_err().
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
This driver doesn't belong to anyone. Go ahead and take
ownership. The
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
message is fairly worthless and no one will miss it.
Speaking of ownership, what this driver really needs is to be
ported to
Quoted 9 lines
Quoted 1 line
Quoted 1 line
rc-core. In order to do this it'll need to be able to send raw IR
rather
Quoted 1 line
Quoted 1 line
key codes; I've been peering at the firmware but it neither looks
like
Quoted 3 lines
Quoted 1 line
zilog z8 opcodes nor space/pulse information.
Actually, I think that all features provided by this driver were
already
Quoted 1 line
migrated into the ir-kbd-i2c (drivers/media/i2c/ir-kbd-i2c.c)
driver.
Quoted 5 lines
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).
Quoted 13 lines
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.
Quoted 1 line
Andy and Jarod worked on this conversion, but we decided, on that
time,
Quoted 5 lines
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?
Quoted 15 lines
Transmitter side.
Quoted 1 line
Quoted 1 line
Does anyone have any contacts at Hauppauge who could help with
this?
Quoted 6 lines
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.
Quoted 8 lines
Thanks
Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
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.

Quoted 6 lines

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.

Sean Young

2014-11-09 21:35 UTC #9 lore raw

Quoted 12 lines · Andy Walls

Quoted 11 lines · Andy Walls
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.

Quoted 6 lines

Quoted 6 lines
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

Mauro Carvalho Chehab

2014-11-17 14:59 UTC #10 lore raw

Em Sun, 9 Nov 2014 21:35:17 +0000
Sean Young <[email protected]> escreveu:

Quoted 22 lines · Andy Walls

Quoted 12 lines · Andy Walls
Quoted 11 lines
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.
Quoted 6 lines
Quoted 6 lines
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.

Quoted 8 lines

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.

Quoted 9 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sean Young

2014-11-20 12:38 UTC #11 lore raw

Quoted 32 lines · Mauro Carvalho Chehab

Em Sun, 9 Nov 2014 21:35:17 +0000
Sean Young <[email protected]> escreveu:
Quoted 29 lines · Andy Walls
Quoted 12 lines · Andy Walls
Quoted 11 lines
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.
Quoted 6 lines
Quoted 6 lines
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.

Quoted 7 lines

Quoted 1 line
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