2008/7/17 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/