[bug report] NFC: fix broken device allocation

[bug report] NFC: fix broken device allocation

[Dan Carpenter]

Hello Johan Hovold,

The patch 20777bc57c34: "NFC: fix broken device allocation" from Mar
30, 2017, leads to the following static checker warning:

	drivers/nfc/pn533/pn533.c:2653 pn533_register_device()
	error: 'priv->nfc_dev' dereferencing possible ERR_PTR()

drivers/nfc/pn533/pn533.c
  2639          skb_queue_head_init(&priv->resp_q);
  2640          skb_queue_head_init(&priv->fragment_skb);
  2641  
  2642          INIT_LIST_HEAD(&priv->cmd_queue);
  2643  
  2644          priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
  2645                                             priv->ops->tx_header_len +
  2646                                             PN533_CMD_DATAEXCH_HEAD_LEN,
  2647                                             priv->ops->tx_tail_len);

We changed this to return error pointers as well as NULL.  When
functions return a NULL as well as error pointers, then NULL is supposed
to be a special case of success but here it's just a failure.  That's
messy and bug prone.

  2648          if (!priv->nfc_dev) {
  2649                  rc = -ENOMEM;
  2650                  goto destroy_wq;
  2651          }
  2652  
  2653          nfc_set_parent_dev(priv->nfc_dev, parent);
                                   ^^^^^^^^^^^^^
Oops.

  2654          nfc_set_drvdata(priv->nfc_dev, priv);
  2655  
  2656          rc = nfc_register_device(priv->nfc_dev);
  2657          if (rc)
  2658                  goto free_nfc_dev;

regards,
dan carpenter

Re: [bug report] NFC: fix broken device allocation

[Johan Hovold]

On Fri, Jul 07, 2017 at 12:33:34PM +0300, Dan Carpenter wrote:
> Hello Johan Hovold,
> 
> The patch 20777bc57c34: "NFC: fix broken device allocation" from Mar
> 30, 2017, leads to the following static checker warning:
> 
> 	drivers/nfc/pn533/pn533.c:2653 pn533_register_device()
> 	error: 'priv->nfc_dev' dereferencing possible ERR_PTR()

> drivers/nfc/pn533/pn533.c
>   2639          skb_queue_head_init(&priv->resp_q);
>   2640          skb_queue_head_init(&priv->fragment_skb);
>   2641  
>   2642          INIT_LIST_HEAD(&priv->cmd_queue);
>   2643  
>   2644          priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
>   2645                                             priv->ops->tx_header_len +
>   2646                                             PN533_CMD_DATAEXCH_HEAD_LEN,
>   2647                                             priv->ops->tx_tail_len);
> 
> We changed this to return error pointers as well as NULL.  When
> functions return a NULL as well as error pointers, then NULL is supposed
> to be a special case of success but here it's just a failure.  That's
> messy and bug prone.

Thanks for reporting this, Dan.

I'll take a closer look at this tomorrow, but I guess we could just
continue using NULL for all errors for now.

Thanks,
Johan