Re: [PATCH next V2] drivers: Use NULL not zero pointer assignments

[Ryan Mallon <rmallon@gmail.com>]

On 27/01/12 16:33, Joe Perches wrote:

> Using NULL pointer assignments is more kernel-style standard.
> 
> Uncompiled, untested.
> 
> Done via cocinelle script:
> 
> @@
> type T;
> T *pointer;
> @@
> -pointer = 0
> +pointer = NULL
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---

Hi Joe,

I think you can drop a lot of the initialisations completely rather than
convert them. I've pointed some out below. I'm just scanning through for
likely candidates and I didn't bother looking at the staging drivers, so
this list is not comprehensive.

~Ryan

<snip>

> diff --git a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
> index f8f41e0..8df54cf 100644
> --- a/drivers/atm/ambassador.c
> +++ b/drivers/atm/ambassador.c
> @@ -1922,7 +1922,7 @@ static int __devinit ucode_init (loader_block * lb, amb_dev * dev) {
>    const struct firmware *fw;
>    unsigned long start_address;
>    const struct ihex_binrec *rec;
> -  const char *errmsg = 0;
> +  const char *errmsg = NULL;


This one looks like the assignment can just be dropped. errmsg is only
used on the fail: exit path and errmsg gets assigned on all paths which
goto there. Assuming gcc is smart enough to get this right, the
initialisation here is not required.

Unrelated: What happened to the indentation in that file. How did it
ever get passed review?


> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 791c0ef..1db1845 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -113,7 +113,7 @@ static int psbfb_pan(struct fb_var_screeninfo *var, struct fb_info *info)
>  
>  void psbfb_suspend(struct drm_device *dev)
>  {
> -	struct drm_framebuffer *fb = 0;
> +	struct drm_framebuffer *fb = NULL;

>  	struct psb_framebuffer *psbfb = to_psb_fb(fb);

This code looks broken? to_psb_fb is defined as:

  container_of(x, struct psb_framebuffer, base)

So passing a NULL fb is not useful. I think it can be fixed by not
initialising either fb or psbfb, and then doing:

  list_for_each_entry(fb, &dev->mode_config.fb_list, head) {
  	psbfb = to_psb_fb(fb);
  	...

>  
>  	console_lock();
> @@ -129,7 +129,7 @@ void psbfb_suspend(struct drm_device *dev)
>  
>  void psbfb_resume(struct drm_device *dev)
>  {
> -	struct drm_framebuffer *fb = 0;
> +	struct drm_framebuffer *fb = NULL;
>  	struct psb_framebuffer *psbfb = to_psb_fb(fb);


Code looks broken in the same way as the psbfb_suspend function.

> diff --git a/drivers/gpu/drm/nouveau/nv50_instmem.c b/drivers/gpu/drm/nouveau/nv50_instmem.c
> index a7c12c9..b2f89ef 100644
> --- a/drivers/gpu/drm/nouveau/nv50_instmem.c
> +++ b/drivers/gpu/drm/nouveau/nv50_instmem.c
> @@ -253,7 +253,7 @@ nv50_instmem_takedown(struct drm_device *dev)
>  	nouveau_gpuobj_ref(NULL, &priv->bar1_dmaobj);
>  
>  	nouveau_vm_ref(NULL, &dev_priv->bar1_vm, chan->vm_pd);
> -	dev_priv->channels.ptr[127] = 0;
> +	dev_priv->channels.ptr[127] = NULL;
>  	nv50_channel_del(&dev_priv->channels.ptr[0]);
>  
>  	nouveau_gpuobj_ref(NULL, &dev_priv->bar3_vm->pgt[0].obj[0]);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 0af6ebd..15531c5 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -378,7 +378,7 @@ int vmw_framebuffer_create_handle(struct drm_framebuffer *fb,
>  				  unsigned int *handle)
>  {
>  	if (handle)
> -		handle = 0;
> +		handle = NULL;


This doesn't do anything useful. I think maybe it is meant to be:

  if (handle)
  	*handle = 0;

Otherwise the whole function should just get removed.

>  
>  	return 0;
>  }



> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index bdea288..c727cf6 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1210,7 +1210,7 @@ static int __init init_exclusion_range(struct ivmd_header *m)
>  /* called for unity map ACPI definition */
>  static int __init init_unity_map_range(struct ivmd_header *m)
>  {
> -	struct unity_map_entry *e = 0;
> +	struct unity_map_entry *e = NULL;


This initialisation can be removed. e is immediately kzalloc'ed just below.

>  	char *s;
>  
>  	e = kzalloc(sizeof(*e), GFP_KERNEL);

> diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
> index e5eb56a..90db4f3 100644
> --- a/drivers/media/video/hdpvr/hdpvr-core.c
> +++ b/drivers/media/video/hdpvr/hdpvr-core.c
> @@ -295,7 +295,7 @@ static int hdpvr_probe(struct usb_interface *interface,
>  		goto error;
>  	}
>  
> -	dev->workqueue = 0;
> +	dev->workqueue = NULL;


This line can be removed. dev was kzalloc'ed just above so
dev->workqueue is already NULL.

> diff --git a/drivers/net/fddi/skfp/hwmtm.c b/drivers/net/fddi/skfp/hwmtm.c
> index e26398b..41fd280 100644
> --- a/drivers/net/fddi/skfp/hwmtm.c
> +++ b/drivers/net/fddi/skfp/hwmtm.c
> @@ -46,8 +46,8 @@ static char const ID_sccs[] = "@(#)hwmtm.c	1.40 99/05/31 (C) SK" ;
>  	-------------------------------------------------------------
>  */
>  #ifdef COMMON_MB_POOL
> -static	SMbuf *mb_start = 0 ;
> -static	SMbuf *mb_free = 0 ;
> +static	SMbuf *mb_start = NULL;
> +static	SMbuf *mb_free = NULL;


Drop the assignments here. These are globals, so already initialised to
NULL.

> diff --git a/drivers/scsi/be2iscsi/be_cmds.c b/drivers/scsi/be2iscsi/be_cmds.c
> index cdb1536..ef67f9c 100644
> --- a/drivers/scsi/be2iscsi/be_cmds.c
> +++ b/drivers/scsi/be2iscsi/be_cmds.c
> @@ -22,9 +22,9 @@
>  int beiscsi_pci_soft_reset(struct beiscsi_hba *phba)
>  {
>  	u32 sreset;
> -	u8 *pci_reset_offset = 0;
> -	u8 *pci_online0_offset = 0;
> -	u8 *pci_online1_offset = 0;
> +	u8 *pci_reset_offset = NULL;
> +	u8 *pci_online0_offset = NULL;
> +	u8 *pci_online1_offset = NULL;


Drop all of these. They all get re-assigned immediately below.

>  	u32 pconline0 = 0;
>  	u32 pconline1 = 0;
>  	u32 i;
> @@ -74,7 +74,7 @@ int beiscsi_pci_soft_reset(struct beiscsi_hba *phba)
>  int be_chk_reset_complete(struct beiscsi_hba *phba)
>  {
>  	unsigned int num_loop;
> -	u8 *mpu_sem = 0;
> +	u8 *mpu_sem = NULL;


Same here. It gets assigned again unconditionally about 3 lines down.

>  	u32 status;
>  
>  	num_loop = 1000;
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 375756f..a53e797 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -4132,7 +4132,7 @@ static void beiscsi_quiesce(struct beiscsi_hba *phba)
>  	struct hwi_context_memory *phwi_context;
>  	struct be_eq_obj *pbe_eq;
>  	unsigned int i, msix_vec;
> -	u8 *real_offset = 0;
> +	u8 *real_offset = NULL;


This initialisation isn't needed. It gets assigned unconditionally in
the code below before anything attempts to use it.

>  	u32 value = 0;
>  
>  	phwi_ctrlr = phba->phwi_ctrlr;
> @@ -4229,7 +4229,7 @@ static int __devinit beiscsi_dev_probe(struct pci_dev *pcidev,
>  	struct hwi_context_memory *phwi_context;
>  	struct be_eq_obj *pbe_eq;
>  	int ret, num_cpus, i;
> -	u8 *real_offset = 0;
> +	u8 *real_offset = NULL;


Same here. Just remove the initialisation.

>  	u32 value = 0;
>  
>  	ret = beiscsi_enable_pci(pcidev);
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 23a2759..7daba92 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -15898,7 +15898,7 @@ lpfc_drain_txq(struct lpfc_hba *phba)
>  {
>  	LIST_HEAD(completions);
>  	struct lpfc_sli_ring *pring = &phba->sli.ring[LPFC_ELS_RING];
> -	struct lpfc_iocbq *piocbq = 0;
> +	struct lpfc_iocbq *piocbq = NULL;


Initialisation doesn't appear to be necessary.

>  	unsigned long iflags = 0;
>  	char *fail_msg = NULL;
>  	struct lpfc_sglq *sglq;


~Ryan