orinoco driver question

orinoco driver question

[Mariusz Kozlowski]

Hi there,

	Hope that's not a problem to ask some 'newbie' kernel coding stuff here. Here 
it goes:

There are those two orinoco ioctl's

- orinoco_ioctl_setport3
- orinoco_ioctl_getport3

Both take 'char *extra' as an argument to set/get 'priv->prefer_port3'. The 
argument value to orinoco_ioctl_setport3 can be either 0 (IEEE ad-hoc mode) 
or 1 (Lucent proprietary ad-hoc mode) the rest is -EINVAL. I don't get why 
there is a need for an extra 'int' variable and casts in the code. 
Using 'char *extra' seems to be fine there. To visualize what I mean here is 
the patch:

--- linux-2.6.19-rc4-orig/drivers/net/wireless/orinoco.c        2006-11-02 
23:52:39.000000000 +0100
+++ linux-2.6.19-rc4/drivers/net/wireless/orinoco.c     2006-11-03 
16:02:45.000000000 +0100
@@ -3658,14 +3658,13 @@ static int orinoco_ioctl_setport3(struct
                                  char *extra)
 {
        struct orinoco_private *priv = netdev_priv(dev);
-       int val = *( (int *) extra );
        int err = 0;
        unsigned long flags;
 
        if (orinoco_lock(priv, &flags) != 0)
                return -EBUSY;
 
-       switch (val) {
+       switch (*extra) {
        case 0: /* Try to do IEEE ad-hoc mode */
                if (! priv->has_ibss) {
                        err = -EINVAL;
@@ -3704,9 +3703,8 @@ static int orinoco_ioctl_getport3(struct
                                  char *extra)
 {
        struct orinoco_private *priv = netdev_priv(dev);
-       int *val = (int *) extra;
 
-       *val = priv->prefer_port3;
+       *extra = (char)priv->prefer_port3;
        return 0;
 }

I don't think this patch decreases code readability. This is just an example 
but if there are more functions like this doesn't removing 'redundant' (?) 
variables make the code better?

Regards,

	Mariusz Kozlowski

Re: orinoco driver question

[Andreas Schwab]

Mariusz Kozlowski <m.kozlowski@tuxland.pl> writes:

> I don't think this patch decreases code readability.

It breaks the interface.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: orinoco driver question

[Mariusz Kozlowski]

> > I don't think this patch decreases code readability.
>
> It breaks the interface.

You mean this part:

-       *val = priv->prefer_port3;
+       *extra = (char)priv->prefer_port3;

It can be done another way. 

-       *val = priv->prefer_port3;
+	*(int *)extra = priv->prefer_port3;

Where can I read about this 'interface'?

Regards,

	Mariusz

Re: orinoco driver question

[Michael Buesch]

On Friday 03 November 2006 16:13, Mariusz Kozlowski wrote:
> Hi there,
> 
> 	Hope that's not a problem to ask some 'newbie' kernel coding stuff here. Here 
> it goes:
> 
> There are those two orinoco ioctl's
> 
> - orinoco_ioctl_setport3
> - orinoco_ioctl_getport3
> 
> Both take 'char *extra' as an argument to set/get 'priv->prefer_port3'. The 
> argument value to orinoco_ioctl_setport3 can be either 0 (IEEE ad-hoc mode) 
> or 1 (Lucent proprietary ad-hoc mode) the rest is -EINVAL. I don't get why 
> there is a need for an extra 'int' variable and casts in the code. 
> Using 'char *extra' seems to be fine there. To visualize what I mean here is 
> the patch:
> 
> --- linux-2.6.19-rc4-orig/drivers/net/wireless/orinoco.c        2006-11-02 
> 23:52:39.000000000 +0100
> +++ linux-2.6.19-rc4/drivers/net/wireless/orinoco.c     2006-11-03 
> 16:02:45.000000000 +0100
> @@ -3658,14 +3658,13 @@ static int orinoco_ioctl_setport3(struct
>                                   char *extra)
>  {
>         struct orinoco_private *priv = netdev_priv(dev);
> -       int val = *( (int *) extra );
>         int err = 0;
>         unsigned long flags;
>  
>         if (orinoco_lock(priv, &flags) != 0)
>                 return -EBUSY;
>  
> -       switch (val) {
> +       switch (*extra) {
>         case 0: /* Try to do IEEE ad-hoc mode */
>                 if (! priv->has_ibss) {
>                         err = -EINVAL;
> @@ -3704,9 +3703,8 @@ static int orinoco_ioctl_getport3(struct
>                                   char *extra)
>  {
>         struct orinoco_private *priv = netdev_priv(dev);
> -       int *val = (int *) extra;
>  
> -       *val = priv->prefer_port3;
> +       *extra = (char)priv->prefer_port3;
>         return 0;
>  }
> 
> I don't think this patch decreases code readability. This is just an example 
> but if there are more functions like this doesn't removing 'redundant' (?) 
> variables make the code better?

This breaks on bigendian systems.

-- 
Greetings Michael.