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
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.
2008/7/17 Mikael Pettersson <[email protected]>:
> 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