re: brcmsmac: use container_of instead of cast

re: brcmsmac: use container_of instead of cast

[Dan Carpenter]

The patch ed1dd81464f5: "brcmsmac: use container_of instead of cast" 
from Jun 30, 2012, leads to the following Smatch warning:
drivers/net/wireless/brcm80211/brcmsmac/aiutils.c:543 ai_detach()
	 warn: can 'sii' even be NULL?

   533  /* may be called with core in reset */
   534  void ai_detach(struct si_pub *sih)
   535  {
   536          struct si_info *sii;
   537  
   538          struct si_pub *si_local = NULL;
   539          memcpy(&si_local, &sih, sizeof(struct si_pub **));
   540  
   541          sii = container_of(sih, struct si_info, pub);
   542  
   543          if (sii == NULL)
   544                  return;
   545  
   546          kfree(sii);
   547  }


Smatch complains because container_of() of does pointer math and the
check for NULL only works when ->pub is first member of the si_info
struct.

Are you sure you want to free sii and not sih?

Also kfree() checks for NULL.

The memcpy() is pointless.

The white space is not right.  There should be a blank line after the
variable declarations.

regards,
dan carpenter

Re: brcmsmac: use container_of instead of cast

[Hauke Mehrtens]

On 07/12/2012 01:20 PM, Dan Carpenter wrote:
> The patch ed1dd81464f5: "brcmsmac: use container_of instead of cast" 
> from Jun 30, 2012, leads to the following Smatch warning:
> drivers/net/wireless/brcm80211/brcmsmac/aiutils.c:543 ai_detach()
> 	 warn: can 'sii' even be NULL?
> 
>    533  /* may be called with core in reset */
>    534  void ai_detach(struct si_pub *sih)
>    535  {
>    536          struct si_info *sii;
>    537  
>    538          struct si_pub *si_local = NULL;
>    539          memcpy(&si_local, &sih, sizeof(struct si_pub **));

I do not get what this line is used for, it looks like unneeded code,
si_local is never read.

>    540  
>    541          sii = container_of(sih, struct si_info, pub);
>    542  
>    543          if (sii == NULL)
>    544                  return;
>    545  
>    546          kfree(sii);
>    547  }
> 
> 
> Smatch complains because container_of() of does pointer math and the
> check for NULL only works when ->pub is first member of the si_info
> struct.

For now pub is the first member of the struct, but the null check should
be done on sih and it should be moved before the container_of line.

> Are you sure you want to free sii and not sih?
Yes sii should be freed, it is allocated in the function ai_attach()
above and there sii is casted to sih, so this only works if pub is the
first member.

> Also kfree() checks for NULL.
> 
> The memcpy() is pointless.
> 
> The white space is not right.  There should be a blank line after the
> variable declarations.
> 
> regards,
> dan carpenter

Hauke