Hi,
This patch fixes the following warning
drivers/staging/otus/ioctl.c: In function `usbdrv_wpa_ioctl':
drivers/staging/otus/ioctl.c:2269: warning: ISO C90 forbids mixed
declarations and code
drivers/staging/otus/ioctl.c: In function `usbdrv_ioctl':
drivers/staging/otus/ioctl.c:2448: warning: ISO C90 forbids mixed
declarations and code
Thanks,
--
Pranith.
Signed-off-by: D Pranith Kumar <[email protected]>
diff --git a/drivers/staging/otus/ioctl.c b/drivers/staging/otus/ioctl.c
index ce04218..5e8bd94 100644
--- a/drivers/staging/otus/ioctl.c
+++ b/drivers/staging/otus/ioctl.c
@@ -2045,6 +2045,7 @@ int usbdrv_wpa_ioctl(struct net_device *dev, struct athr_wlan_param *zdparm)
struct zsKeyInfo keyInfo;
struct usbdrv_private *macp = dev->ml_priv;
u16_t vapId = 0;
+ int ii;
/* zmw_get_wlan_dev(dev); */
@@ -2168,7 +2169,6 @@ int usbdrv_wpa_ioctl(struct net_device *dev, struct athr_wlan_param *zdparm)
/* DUMP key context */
/* #ifdef WPA_DEBUG */
if (keyInfo.keyLength > 0) {
- int ii;
printk(KERN_WARNING
"Otus: Key Context:\n");
for (ii = 0; ii < keyInfo.keyLength; ) {
@@ -2266,7 +2266,6 @@ int usbdrv_wpa_ioctl(struct net_device *dev, struct athr_wlan_param *zdparm)
/* zfiWlanSetWpaIe(dev, zdparm->u.generic_elem.data,
* zdparm->u.generic_elem.len);
*/
- int ii;
u8_t len = zdparm->u.generic_elem.len;
u8_t *wpaie = (u8_t *)zdparm->u.generic_elem.data;
@@ -2401,7 +2400,7 @@ int usbdrv_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
struct athr_wlan_param zdparm;
struct usbdrv_private *macp = dev->ml_priv;
- int err = 0;
+ int err = 0, val = 0;
int changed = 0;
/* regp = macp->regp; */
@@ -2445,7 +2444,7 @@ int usbdrv_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
err = -EPERM;
break;
}
- int val = *((int *) wrq->u.name);
+ val = *((int *) wrq->u.name);
if ((val < 0) || (val > 2)) {
err = -EINVAL;
break;
Pranith Kumar wrote:
> Hi,
>
> This patch fixes the following warning
>
> drivers/staging/otus/ioctl.c: In function `usbdrv_wpa_ioctl':
> drivers/staging/otus/ioctl.c:2269: warning: ISO C90 forbids mixed
> declarations and code
> drivers/staging/otus/ioctl.c: In function `usbdrv_ioctl':
> drivers/staging/otus/ioctl.c:2448: warning: ISO C90 forbids mixed
> declarations and code
Can you please supersede the above patch with this one?? This fixes
a bunch of other warnings too.
Thanks,
Pranith.
Signed-off-by: D Pranith Kumar <[email protected]>
diff --git a/drivers/staging/otus/ioctl.c b/drivers/staging/otus/ioctl.c
index ce04218..dd32705 100644
--- a/drivers/staging/otus/ioctl.c
+++ b/drivers/staging/otus/ioctl.c
@@ -2045,6 +2045,7 @@ int usbdrv_wpa_ioctl(struct net_device *dev, struct athr_wlan_param *zdparm)
struct zsKeyInfo keyInfo;
struct usbdrv_private *macp = dev->ml_priv;
u16_t vapId = 0;
+ int ii;
/* zmw_get_wlan_dev(dev); */
@@ -2168,7 +2169,6 @@ int usbdrv_wpa_ioctl(struct net_device *dev, struct athr_wlan_param *zdparm)
/* DUMP key context */
/* #ifdef WPA_DEBUG */
if (keyInfo.keyLength > 0) {
- int ii;
printk(KERN_WARNING
"Otus: Key Context:\n");
for (ii = 0; ii < keyInfo.keyLength; ) {
@@ -2266,7 +2266,6 @@ int usbdrv_wpa_ioctl(struct net_device *dev, struct athr_wlan_param *zdparm)
/* zfiWlanSetWpaIe(dev, zdparm->u.generic_elem.data,
* zdparm->u.generic_elem.len);
*/
- int ii;
u8_t len = zdparm->u.generic_elem.len;
u8_t *wpaie = (u8_t *)zdparm->u.generic_elem.data;
@@ -2401,7 +2400,7 @@ int usbdrv_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
struct athr_wlan_param zdparm;
struct usbdrv_private *macp = dev->ml_priv;
- int err = 0;
+ int err = 0, val = 0;
int changed = 0;
/* regp = macp->regp; */
@@ -2445,7 +2444,7 @@ int usbdrv_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
err = -EPERM;
break;
}
- int val = *((int *) wrq->u.name);
+ val = *((int *) wrq->u.name);
if ((val < 0) || (val > 2)) {
err = -EINVAL;
break;
diff --git a/drivers/staging/rt2860/common/ba_action.c b/drivers/staging/rt2860/common/ba_action.c
index 591d1e2..40dc19a 100644
--- a/drivers/staging/rt2860/common/ba_action.c
+++ b/drivers/staging/rt2860/common/ba_action.c
@@ -1512,7 +1512,8 @@ void convert_reordering_packet_to_preAMSDU_or_802_3_packet(
RTPKT_TO_OSPKT(pRxPkt)->dev = get_netdev_from_bssid(pAd, FromWhichBSSID);
RTPKT_TO_OSPKT(pRxPkt)->data = pRxBlk->pData;
RTPKT_TO_OSPKT(pRxPkt)->len = pRxBlk->DataSize;
- RTPKT_TO_OSPKT(pRxPkt)->tail = RTPKT_TO_OSPKT(pRxPkt)->data + RTPKT_TO_OSPKT(pRxPkt)->len;
+ RTPKT_TO_OSPKT(pRxPkt)->tail = (UCHAR *) (RTPKT_TO_OSPKT(pRxPkt)->data
+ + RTPKT_TO_OSPKT(pRxPkt)->len);
//
// copy 802.3 header, if necessary
diff --git a/drivers/staging/rt2860/rt_linux.c b/drivers/staging/rt2860/rt_linux.c
index f3c128c..a44559b 100644
--- a/drivers/staging/rt2860/rt_linux.c
+++ b/drivers/staging/rt2860/rt_linux.c
@@ -607,7 +607,7 @@ PNDIS_PACKET ClonePacket(
pClonedPkt->dev = pRxPkt->dev;
pClonedPkt->data = pData;
pClonedPkt->len = DataSize;
- pClonedPkt->tail = pClonedPkt->data + pClonedPkt->len;
+ pClonedPkt->tail = (UCHAR *)(pClonedPkt->data + pClonedPkt->len);
ASSERT(DataSize < 1530);
}
return pClonedPkt;
@@ -629,7 +629,7 @@ void update_os_packet_info(
pOSPkt->dev = get_netdev_from_bssid(pAd, FromWhichBSSID);
pOSPkt->data = pRxBlk->pData;
pOSPkt->len = pRxBlk->DataSize;
- pOSPkt->tail = pOSPkt->data + pOSPkt->len;
+ pOSPkt->tail = (UCHAR *) (pOSPkt->data + pOSPkt->len);
}
@@ -649,7 +649,7 @@ void wlan_802_11_to_802_3_packet(
pOSPkt->dev = get_netdev_from_bssid(pAd, FromWhichBSSID);
pOSPkt->data = pRxBlk->pData;
pOSPkt->len = pRxBlk->DataSize;
- pOSPkt->tail = pOSPkt->data + pOSPkt->len;
+ pOSPkt->tail = (UCHAR *) (pOSPkt->data + pOSPkt->len);
//
// copy 802.3 header
diff --git a/drivers/staging/slicoss/slicoss.c b/drivers/staging/slicoss/slicoss.c
index 6f5d0bf..c35d81e 100644
--- a/drivers/staging/slicoss/slicoss.c
+++ b/drivers/staging/slicoss/slicoss.c
@@ -1872,7 +1872,7 @@ static int slic_card_download(struct adapter *adapter)
__iomem struct slic_regs *slic_regs = adapter->slic_regs;
u32 instruction;
u32 baseaddress;
- u32 failure;
+ /* u32 failure; */
u32 i;
u32 numsects = 0;
u32 sectsize[3];
diff --git a/drivers/staging/sxg/sxg.c b/drivers/staging/sxg/sxg.c
index 076b3f7..30bbf76 100644
--- a/drivers/staging/sxg/sxg.c
+++ b/drivers/staging/sxg/sxg.c
@@ -2150,7 +2150,7 @@ void sxg_set_interrupt_aggregation(struct adapter_t *adapter)
static int sxg_entry_open(struct net_device *dev)
{
struct adapter_t *adapter = (struct adapter_t *) netdev_priv(dev);
- int status;
+ int status = STATUS_FAILURE;
static int turn;
int sxg_initial_rcv_data_buffers = SXG_INITIAL_RCV_DATA_BUFFERS;
int i;
Pranith Kumar wrote:
> @@ -629,7 +629,7 @@ void update_os_packet_info(
> pOSPkt->dev = get_netdev_from_bssid(pAd, FromWhichBSSID);
> pOSPkt->data = pRxBlk->pData;
> pOSPkt->len = pRxBlk->DataSize;
> - pOSPkt->tail = pOSPkt->data + pOSPkt->len;
> + pOSPkt->tail = (UCHAR *) (pOSPkt->data + pOSPkt->len);
> }
>
>
This is what I meant with "a warning should stay there as long as the
underlying problem isn't fixed".
This code uses defined types which are foreign to Linux. We don't
define UCHAR in Linux. /This/ needs to be fixed in the entire driver.
Until this is not done, there is no reason to add this pointer type cast
merely to quieten gcc.
--
Stefan Richter
-=====-==--= -==- -=---
http://arcgraph.de/sr/
Stefan Richter wrote:
>
> This is what I meant with "a warning should stay there as long as the
> underlying problem isn't fixed".
>
> This code uses defined types which are foreign to Linux. We don't
> define UCHAR in Linux. /This/ needs to be fixed in the entire driver.
> Until this is not done, there is no reason to add this pointer type cast
> merely to quieten gcc.
Hi Stefan,
Thanks for your comment. Going through the header, I found the following defines
typedef unsigned char UINT8;
typedef unsigned short UINT16;
typedef unsigned int UINT32;
typedef unsigned long long UINT64;
typedef int INT32;
typedef long long INT64;
typedef unsigned char * PUINT8;
typedef unsigned short * PUINT16;
typedef unsigned int * PUINT32;
typedef unsigned long long * PUINT64;
typedef int * PINT32;
typedef long long * PINT64;
typedef signed char CHAR;
typedef signed short SHORT;
typedef signed int INT;
typedef signed long LONG;
typedef signed long long LONGLONG;
typedef unsigned char UCHAR;
typedef unsigned short USHORT;
typedef unsigned int UINT;
typedef unsigned long ULONG;
typedef unsigned long long ULONGLONG;
typedef unsigned char BOOLEAN;
typedef void VOID;
typedef VOID * PVOID;
typedef CHAR * PCHAR;
typedef UCHAR * PUCHAR;
typedef USHORT * PUSHORT;
typedef LONG * PLONG;
typedef ULONG * PULONG;
typedef UINT * PUINT;
Now if I delete all these defines and do a search and replace for those types, is it OK?
There might be an argument that the current state is much cleaner.
What should I do?
Thanks,
Pranith.
Pranith Kumar wrote:
> Stefan Richter wrote:
...
>> This code uses defined types which are foreign to Linux. We don't
>> define UCHAR in Linux. /This/ needs to be fixed in the entire driver.
>> Until this is not done, there is no reason to add this pointer type cast
...
> Thanks for your comment. Going through the header, I found the following
> defines
>
> typedef unsigned char UINT8;
> typedef unsigned short UINT16;
...
> Now if I delete all these defines and do a search and replace for those
> types, is it OK?
>
> There might be an argument that the current state is much cleaner.
Plain C's unsigned char and friends should be preferred. Pointer types
like PVOID should be replaced by straight-forward "void *" and so on,
and BOOLEAN can be replaced by "bool" (with "true" and "false" as
possible values).
In cases where the particular size of variables or struct members
matters, there are u8, s16 etc. available.
Furthermore, when variables don't represent CPU endian (host endian)
data but actually big endian or little endian, then we annotate the
types with __be32, __le32 etc.. *However*, adding these endian
annotations (*if* the driver in question has to deal with endianess
other than CPU) should usually better be done in another separate patch
or series of patches. This is because the process of adding the endia
annotations has a chance of unveiling sloppy or non-protable or even
buggy code, and then some more intensive and non-trivial work on the
code will be required.
Lastly, if there is a binary interface to userspace (e.g. character
device file interface), then we use types like __u32 in header files
which are going to be used by userspace programs.
Now, the type replacement will of course shuffle the text flow (line
lengths...), so the question will arise whether to adjust whitespace
(line wraps) in the same patch set. However, the code which you looked
at also has other trivial deviations from kernel style. Notably, the
use of CamelCase names rather than all-lowercase with underscores.
Example: Something like pClonedPkt should become p_cloned_pkt or better
p_cloned_packet or cloned_packet (if the p prefix didn't mean anything
else than that it was a pointer), or if it is clear from context,
something brief like cp.
Such style adjustments also need to happen; if you are interested in
doing that, then you can plan ahead and hold off with whitespace
adjusting changes (indentation, line wraps...) until after you did those
other changes regarding type names and variable/ function/ macro names.
In any case, before starting some bigger style adjustments, check the
TODO file of the respective staging driver and notify Greg and
[email protected], so that clashes with ongoing work by
others can be avoided.
Moreover, you should probably _not_ start such work on wireless drivers
in staging like rt2860. See the notes in the TODO files of these
drivers; the real work is going on at other drivers for the same
hardware in the normal Linux wireless development tree.
--
Stefan Richter
-=====-==--= -==- -=---
http://arcgraph.de/sr/
I wrote:
> However, the code which you looked
> at also has other trivial deviations from kernel style. Notably, the
> use of CamelCase names rather than all-lowercase with underscores.
PS: Name changes are already at least one level more difficult than 1:1
type replacements, because giving _good_ names to variables/ functions
etc. requires deeper understanding of what the code does.
> if you are interested in doing that, then you can plan ahead and hold
> off with whitespace adjusting changes (indentation, line wraps...)
> until after you did those other changes
But that's only an 'if'. If you rather only want to work on a single
logical step for now, e.g. type replacement, then that's surely very
welcome too. (My only concern about the "Fix warning..." was that
making the warnings go away does not lead to actual improvements, or
worse, covers up actual issues with the code. I.e. I didn't want to
press you in to fixing those, just to take care _not_ to paper over them.)
--
Stefan Richter
-=====-==--= -==- -=---
http://arcgraph.de/sr/