Typecast problems in SKFP driver

Typecast problems in SKFP driver

[Loic Grenie]

[-- Attachment #1: Type: text/plain, Size: 214 bytes --]

    I've just compiled the 2.6.26-git5 kernel and the SKFP driver
    compilation produces complaints about typecasts. These complaints
    disappear with the following patch.

      Thanks,

          Loïc Grenié

[-- Attachment #2: skfp.diff --]
[-- Type: text/x-diff, Size: 2497 bytes --]

diff --git a/drivers/net/skfp/ess.c b/drivers/net/skfp/ess.c
index 889f987..d3a8afc 100644
--- a/drivers/net/skfp/ess.c
+++ b/drivers/net/skfp/ess.c
@@ -510,7 +510,7 @@ static void ess_send_response(struct s_smc *smc, struct smt_header *sm,
 	chg->path.para.p_type = SMT_P320B ;
 	chg->path.para.p_len = sizeof(struct smt_p_320b) - PARA_LEN ;
 	chg->path.mib_index = SBAPATHINDEX ;
-	chg->path.path_pad = (u_short)NULL ;
+	chg->path.path_pad = (u_short)(long)NULL ;
 	chg->path.path_index = PRIMARY_RING ;
 
 	/* set P320F */
@@ -606,7 +606,7 @@ static void ess_send_alc_req(struct s_smc *smc)
 	req->path.para.p_type = SMT_P320B ;
 	req->path.para.p_len = sizeof(struct smt_p_320b) - PARA_LEN ;
 	req->path.mib_index = SBAPATHINDEX ;
-	req->path.path_pad = (u_short)NULL ;
+	req->path.path_pad = (u_short)(long)NULL ;
 	req->path.path_index = PRIMARY_RING ;
 
 	/* set P0017 */
@@ -636,7 +636,7 @@ static void ess_send_alc_req(struct s_smc *smc)
 	/* set P19 */
 	req->a_addr.para.p_type = SMT_P0019 ;
 	req->a_addr.para.p_len = sizeof(struct smt_p_0019) - PARA_LEN ;
-	req->a_addr.sba_pad = (u_short)NULL ;
+	req->a_addr.sba_pad = (u_short)(long)NULL ;
 	req->a_addr.alloc_addr = null_addr ;
 
 	/* set P1A */
diff --git a/drivers/net/skfp/pmf.c b/drivers/net/skfp/pmf.c
index ea85de9..5798be4 100644
--- a/drivers/net/skfp/pmf.c
+++ b/drivers/net/skfp/pmf.c
@@ -44,17 +44,17 @@ static SMbuf *smt_build_pmf_response(struct s_smc *smc, struct smt_header *req,
 				     int set, int local);
 static int port_to_mib(struct s_smc *smc, int p);
 
-#define MOFFSS(e)	((int)&(((struct fddi_mib *)0)->e))
-#define MOFFSA(e)	((int) (((struct fddi_mib *)0)->e))
+#define MOFFSS(e)	((long)&(((struct fddi_mib *)0)->e))
+#define MOFFSA(e)	((long) (((struct fddi_mib *)0)->e))
 
-#define MOFFMS(e)	((int)&(((struct fddi_mib_m *)0)->e))
-#define MOFFMA(e)	((int) (((struct fddi_mib_m *)0)->e))
+#define MOFFMS(e)	((long)&(((struct fddi_mib_m *)0)->e))
+#define MOFFMA(e)	((long) (((struct fddi_mib_m *)0)->e))
 
-#define MOFFAS(e)	((int)&(((struct fddi_mib_a *)0)->e))
-#define MOFFAA(e)	((int) (((struct fddi_mib_a *)0)->e))
+#define MOFFAS(e)	((long)&(((struct fddi_mib_a *)0)->e))
+#define MOFFAA(e)	((long) (((struct fddi_mib_a *)0)->e))
 
-#define MOFFPS(e)	((int)&(((struct fddi_mib_p *)0)->e))
-#define MOFFPA(e)	((int) (((struct fddi_mib_p *)0)->e))
+#define MOFFPS(e)	((long)&(((struct fddi_mib_p *)0)->e))
+#define MOFFPA(e)	((long) (((struct fddi_mib_p *)0)->e))
 
 
 #define AC_G	0x01		/* Get */

Re: Typecast problems in SKFP driver

[Mikael Pettersson]

Loic Grenie writes:
 >     I've just compiled the 2.6.26-git5 kernel and the SKFP driver
 >     compilation produces complaints about typecasts. These complaints
 >     disappear with the following patch.

NAK, this needs to be done by someone with better understanding of C.

 > 
 >       Thanks,
 > 
 >           Loïc Grenié
 > diff --git a/drivers/net/skfp/ess.c b/drivers/net/skfp/ess.c
 > index 889f987..d3a8afc 100644
 > --- a/drivers/net/skfp/ess.c
 > +++ b/drivers/net/skfp/ess.c
 > @@ -510,7 +510,7 @@ static void ess_send_response(struct s_smc *smc, struct smt_header *sm,
 >  	chg->path.para.p_type = SMT_P320B ;
 >  	chg->path.para.p_len = sizeof(struct smt_p_320b) - PARA_LEN ;
 >  	chg->path.mib_index = SBAPATHINDEX ;
 > -	chg->path.path_pad = (u_short)NULL ;
 > +	chg->path.path_pad = (u_short)(long)NULL ;
 >  	chg->path.path_index = PRIMARY_RING ;

Putting NULL (a pointer type) in an u_short field is utterly
and completely broken. My guess is that the code really wants:

	chg->path.path_pad = 0;

 >  	/* set P320F */
 > @@ -606,7 +606,7 @@ static void ess_send_alc_req(struct s_smc *smc)
 >  	req->path.para.p_type = SMT_P320B ;
 >  	req->path.para.p_len = sizeof(struct smt_p_320b) - PARA_LEN ;
 >  	req->path.mib_index = SBAPATHINDEX ;
 > -	req->path.path_pad = (u_short)NULL ;
 > +	req->path.path_pad = (u_short)(long)NULL ;
 >  	req->path.path_index = PRIMARY_RING ;

ditto

 >  
 >  	/* set P0017 */
 > @@ -636,7 +636,7 @@ static void ess_send_alc_req(struct s_smc *smc)
 >  	/* set P19 */
 >  	req->a_addr.para.p_type = SMT_P0019 ;
 >  	req->a_addr.para.p_len = sizeof(struct smt_p_0019) - PARA_LEN ;
 > -	req->a_addr.sba_pad = (u_short)NULL ;
 > +	req->a_addr.sba_pad = (u_short)(long)NULL ;
 >  	req->a_addr.alloc_addr = null_addr ;

ditto

 >  
 >  	/* set P1A */
 > diff --git a/drivers/net/skfp/pmf.c b/drivers/net/skfp/pmf.c
 > index ea85de9..5798be4 100644
 > --- a/drivers/net/skfp/pmf.c
 > +++ b/drivers/net/skfp/pmf.c
 > @@ -44,17 +44,17 @@ static SMbuf *smt_build_pmf_response(struct s_smc *smc, struct smt_header *req,
 >  				     int set, int local);
 >  static int port_to_mib(struct s_smc *smc, int p);
 >  
 > -#define MOFFSS(e)	((int)&(((struct fddi_mib *)0)->e))
 > -#define MOFFSA(e)	((int) (((struct fddi_mib *)0)->e))
 > +#define MOFFSS(e)	((long)&(((struct fddi_mib *)0)->e))
 > +#define MOFFSA(e)	((long) (((struct fddi_mib *)0)->e))

NAK, use offsetof().

How is the MOFFSA() used? As written it makes no sense at all
as evaluating it as an r-value will oops.

 >  
 > -#define MOFFMS(e)	((int)&(((struct fddi_mib_m *)0)->e))
 > -#define MOFFMA(e)	((int) (((struct fddi_mib_m *)0)->e))
 > +#define MOFFMS(e)	((long)&(((struct fddi_mib_m *)0)->e))
 > +#define MOFFMA(e)	((long) (((struct fddi_mib_m *)0)->e))

ditto

 >  
 > -#define MOFFAS(e)	((int)&(((struct fddi_mib_a *)0)->e))
 > -#define MOFFAA(e)	((int) (((struct fddi_mib_a *)0)->e))
 > +#define MOFFAS(e)	((long)&(((struct fddi_mib_a *)0)->e))
 > +#define MOFFAA(e)	((long) (((struct fddi_mib_a *)0)->e))

ditto

 >  
 > -#define MOFFPS(e)	((int)&(((struct fddi_mib_p *)0)->e))
 > -#define MOFFPA(e)	((int) (((struct fddi_mib_p *)0)->e))
 > +#define MOFFPS(e)	((long)&(((struct fddi_mib_p *)0)->e))
 > +#define MOFFPA(e)	((long) (((struct fddi_mib_p *)0)->e))

ditto

To be fair, the original code is crap, but your changes don't
make it any better.

Re: Typecast problems in SKFP driver

[Loïc Grenié]

2008/7/17 Mikael Pettersson <mikpe@it.uu.se>:
> Loic Grenie writes:
>  >     I've just compiled the 2.6.26-git5 kernel and the SKFP driver
>  >     compilation produces complaints about typecasts. These complaints
>  >     disappear with the following patch.
>
> NAK, this needs to be done by someone with better understanding of C.

     or, better, by someone with a better understanding of the driver.

>  >
>  >       Thanks,
>  >
>  >           Loïc Grenié
>  > diff --git a/drivers/net/skfp/ess.c b/drivers/net/skfp/ess.c
>  > index 889f987..d3a8afc 100644
>  > --- a/drivers/net/skfp/ess.c
>  > +++ b/drivers/net/skfp/ess.c
>  > @@ -510,7 +510,7 @@ static void ess_send_response(struct s_smc *smc, struct smt_header *sm,
>  >      chg->path.para.p_type = SMT_P320B ;
>  >      chg->path.para.p_len = sizeof(struct smt_p_320b) - PARA_LEN ;
>  >      chg->path.mib_index = SBAPATHINDEX ;
>  > -    chg->path.path_pad = (u_short)NULL ;
>  > +    chg->path.path_pad = (u_short)(long)NULL ;
>  >      chg->path.path_index = PRIMARY_RING ;
>
> Putting NULL (a pointer type) in an u_short field is utterly
> and completely broken. My guess is that the code really wants:
>
>        chg->path.path_pad = 0;

    Indeed. I do not even understand why it was done that way in the
  first place.

>  >      /* set P320F */
>  > @@ -606,7 +606,7 @@ static void ess_send_alc_req(struct s_smc *smc)
>  >      req->path.para.p_type = SMT_P320B ;
>  >      req->path.para.p_len = sizeof(struct smt_p_320b) - PARA_LEN ;
>  >      req->path.mib_index = SBAPATHINDEX ;
>  > -    req->path.path_pad = (u_short)NULL ;
>  > +    req->path.path_pad = (u_short)(long)NULL ;
>  >      req->path.path_index = PRIMARY_RING ;
>
> ditto
>
>  >
>  >      /* set P0017 */
>  > @@ -636,7 +636,7 @@ static void ess_send_alc_req(struct s_smc *smc)
>  >      /* set P19 */
>  >      req->a_addr.para.p_type = SMT_P0019 ;
>  >      req->a_addr.para.p_len = sizeof(struct smt_p_0019) - PARA_LEN ;
>  > -    req->a_addr.sba_pad = (u_short)NULL ;
>  > +    req->a_addr.sba_pad = (u_short)(long)NULL ;
>  >      req->a_addr.alloc_addr = null_addr ;
>
> ditto
>
>  >
>  >      /* set P1A */
>  > diff --git a/drivers/net/skfp/pmf.c b/drivers/net/skfp/pmf.c
>  > index ea85de9..5798be4 100644
>  > --- a/drivers/net/skfp/pmf.c
>  > +++ b/drivers/net/skfp/pmf.c
>  > @@ -44,17 +44,17 @@ static SMbuf *smt_build_pmf_response(struct s_smc *smc, struct smt_header *req,
>  >                                   int set, int local);
>  >  static int port_to_mib(struct s_smc *smc, int p);
>  >
>  > -#define MOFFSS(e)   ((int)&(((struct fddi_mib *)0)->e))
>  > -#define MOFFSA(e)   ((int) (((struct fddi_mib *)0)->e))
>  > +#define MOFFSS(e)   ((long)&(((struct fddi_mib *)0)->e))
>  > +#define MOFFSA(e)   ((long) (((struct fddi_mib *)0)->e))
>
> NAK, use offsetof().
>
> How is the MOFFSA() used? As written it makes no sense at all
> as evaluating it as an r-value will oops.

     I think it is used when e is an array (unchecked).

>  >
>  > -#define MOFFMS(e)   ((int)&(((struct fddi_mib_m *)0)->e))
>  > -#define MOFFMA(e)   ((int) (((struct fddi_mib_m *)0)->e))
>  > +#define MOFFMS(e)   ((long)&(((struct fddi_mib_m *)0)->e))
>  > +#define MOFFMA(e)   ((long) (((struct fddi_mib_m *)0)->e))
>
> ditto
>
>  >
>  > -#define MOFFAS(e)   ((int)&(((struct fddi_mib_a *)0)->e))
>  > -#define MOFFAA(e)   ((int) (((struct fddi_mib_a *)0)->e))
>  > +#define MOFFAS(e)   ((long)&(((struct fddi_mib_a *)0)->e))
>  > +#define MOFFAA(e)   ((long) (((struct fddi_mib_a *)0)->e))
>
> ditto
>
>  >
>  > -#define MOFFPS(e)   ((int)&(((struct fddi_mib_p *)0)->e))
>  > -#define MOFFPA(e)   ((int) (((struct fddi_mib_p *)0)->e))
>  > +#define MOFFPS(e)   ((long)&(((struct fddi_mib_p *)0)->e))
>  > +#define MOFFPA(e)   ((long) (((struct fddi_mib_p *)0)->e))
>
> ditto
>
> To be fair, the original code is crap, but your changes don't
> make it any better.
>

     I agree... Maybe someone with stronger guts than mine will have
  read about it and will correct it for some next release.

      Thanks once again,

          Loïc