sparse spin_unlock warning

[richard]

I've been running sparse, the kernel code checking tool, on our code
base. it generates some warnings but most of them are not going to be a
problem.
The only one that might possibly be an issue is in hfa384x.c used when
building for the non usb case.

the warning is :-
linux-wlan-ng-devel/src/prism2/driver/hfa384x.c:1687:2: warning: context imbalance in 'hfa384x_cmd_notify'
- different lock contexts for basic block

and the code block :-
if ( result ) {
WLAN_LOG_DEBUG(1,
"copy_to_bap(%04x, 0, %d) failed, result=0x%x\n",
hw->infofid, len, result);
result = -EIO;
goto failed;
}

result = hfa384x_docmd_wait(hw, &cmd);
spin_unlock_bh(&hw->cmdlock);

failed:
spin_unlock_bh(&hw->cmdlock);

DBFEXIT;
return result;

which will call the spin_unlock twice in the successful case.

As I only have a usb adaptor I'm not sure if this is a real issue, but
it might be worth looking at if you've got a pcmcia or pci card.

HTH
Cheers
Richard

[Pavel Roskin]

Post by richard
I've been running sparse, the kernel code checking tool, on our code
base. it generates some warnings but most of them are not going to be a
problem.
The only one that might possibly be an issue is in hfa384x.c used when
building for the non usb case.

Good catch!

I'm attaching a patch that fixes all sparse warnings if sparse is called
without -D__CHECK_ENDIAN__.

It's mostly trivial stuff like making functions static. Actually, some
functions are defined as static first, and them as non-static. That's
not good.

Hardware memory addresses were dereferenced directly in prism2_plx.c.
That's not portable.

One warning was for cast from pointer to signed integer. In one case, a
header wasn't included, which prevented the compiler from verifying the
implementation prototypes against the header.

The biggest change was annotating "data" field in p80211ioctl_req as
__user. Since __user applies to the pointer, the standard fix would be
to just to make the field the pointer. But I remember SPARC users
complained about it. caddr_t can be wider than a pointer. So I made
that filed a union, which required adjustments in many parts of the
code. I also had to change wlan_compat.h to make __user defined for
userspace programs.

Fixing endianess would be a separate patch, as it would require many
more patches.

The patch is compressed to avoid possible message size limit.

[richard]

Post by Pavel Roskin

Post by richard
I've been running sparse, the kernel code checking tool, on our code
base. it generates some warnings but most of them are not going to be a
problem.
The only one that might possibly be an issue is in hfa384x.c used when
building for the non usb case.

Good catch!
I'm attaching a patch that fixes all sparse warnings if sparse is called
without -D__CHECK_ENDIAN__.
It's mostly trivial stuff like making functions static. Actually, some
functions are defined as static first, and them as non-static. That's
not good.
Hardware memory addresses were dereferenced directly in prism2_plx.c.
That's not portable.
One warning was for cast from pointer to signed integer. In one case, a
header wasn't included, which prevented the compiler from verifying the
implementation prototypes against the header.
The biggest change was annotating "data" field in p80211ioctl_req as
__user. Since __user applies to the pointer, the standard fix would be
to just to make the field the pointer. But I remember SPARC users
complained about it. caddr_t can be wider than a pointer. So I made
that filed a union, which required adjustments in many parts of the
code. I also had to change wlan_compat.h to make __user defined for
userspace programs.
Fixing endianess would be a separate patch, as it would require many
more patches.
The patch is compressed to avoid possible message size limit.

Hi Pavel,

wow -- that was quick :)
I've applied your patch here and it all runs fine on a usb adaptor.
looks good to me
Cheers
Richard

[Pavel Roskin]

Post by richard
Hi Pavel,
wow -- that was quick :)
I've applied your patch here and it all runs fine on a usb adaptor.
looks good to me

Actually, I'm unsure about the trick with union for the pointer. If
caddr_t is 64-bit and pointers are 32-bit on a big-endian platform, the
cast to caddr_t would do the right thing by placing zeroes into the
first 4 bytes and the pointer into the following 4 bytes. Using the
union, the address will go to the first 4 bytes.

On the other hand, I'm not sure the current code would work either,
since caddr_t is defined as a pointer everywhere.

Passing pointers from 32-bit userspace to 64-bit kernel is a known
problem, and I don't think there is a universal solution to it. I
remember a discussion concerning this issue in linux-wlan-ng, and I
remember that using caddr_t was considered a solution. But I doubt that
it really solved anything.

I don't have a big-endian 64-bit system for testing, and I don't have
much time to spend on this problem. I can resubmit the patch without
the caddr_t change.

[Solomon Peachy]

Post by Pavel Roskin
I don't have a big-endian 64-bit system for testing, and I don't have
much time to spend on this problem. I can resubmit the patch without
the caddr_t change.

I'd like this, please. (incidentally, I never got the first patch..)

- Solomon

[Pavel Roskin]

Post by Solomon Peachy

Post by Pavel Roskin
I don't have a big-endian 64-bit system for testing, and I don't have
much time to spend on this problem. I can resubmit the patch without
the caddr_t change.

I'd like this, please. (incidentally, I never got the first patch..)

Attached.