On Mon, Mar 05, 2012 at 11:04:49AM +0530, santosh prasad nayak wrote:
> Dan,
>
> Your fix may introduce new bug.
>
> hw->phy.autoneg_advertised = tmp
>
> Assigning signed integer to unsigned short leads to bit truncation.
In this case it doesn't. It's going to be 0x2f or less. I
obviously checked this before I submitted the patch.
> Is it safe for both Big-endian and little endian format ?
It's CPU endian in both cases.
>
> AutoNeg is initialized with a Negative value (OPTION_UNSET)
> Won't it create any issue with above assignment ?
>
Nope. It gets set to 0x2f inside pch_gbe_validate_option().
>
> The simpler fix is to make "autoneg_advertised" signed integer.
>
> struct pch_gbe_phy_info {
> u32 addr;
> u32 id;
> u32 revision;
> u32 reset_delay_us;
> u16 autoneg_advertised; // ==> int autoneg_advertised
> };
>
The better fix would be to change pch_gbe_validate_option() so it
isn't so easy to call improperly. I would have done that except
that probably we won't introduce many more callers, it seemed
like a lot of work and I don't have the hardware to test the
results.
regards,
dan carpenter
Attachment:
signature.asc
Description: Digital signature
