Return-Path: MIME-Version: 1.0 In-Reply-To: <0154F030-52BE-4935-8150-1A96737E7413@holtmann.org> References: <1395815788-24666-1-git-send-email-surendra.tux@gmail.com> <0154F030-52BE-4935-8150-1A96737E7413@holtmann.org> Date: Wed, 26 Mar 2014 10:33:16 -0700 Message-ID: Subject: Re: [PATCH] drivers:bluetooth:ath3k.c Fixed sparse warning for cast to restricted __le32 From: Surendra Patil To: Marcel Holtmann Cc: "Gustavo F. Padovan" , Johan Hedberg , Joe Perches , bt , linux-kernel@vger.kernel.org Content-Type: multipart/alternative; boundary=047d7b5d839121d34a04f585dd85 List-ID: --047d7b5d839121d34a04f585dd85 Content-Type: text/plain; charset=ISO-8859-1 Thanks Marcel for the feedback, In my previous patch joe/johan suggested me to make rom_version to __le32 , when i made changes i got an warning while compilation as " rom_version expects __le32 getting unsigned int " hence i added second changes cpu_to_32(). I am confused about what should be the proper change ? I have only built the module on x86 with sparse. On Wed, Mar 26, 2014 at 12:21 AM, Marcel Holtmann wrote: > Hi Surendra, > > > To fix the sparse warning "cast to restricted __le32" marked > > "rom_version" to __le32 instead of unsigned int in "struct > ath3k_version" > > and added cpu_to_le32() for the expression assigning int value to > "rom_version". > > Successfully built the module without warnings and errors on x86 machine > with sparse. > > > > Signed-off-by: Surendra Patil > > --- > > drivers/bluetooth/ath3k.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c > > index be571fe..5564207 100644 > > --- a/drivers/bluetooth/ath3k.c > > +++ b/drivers/bluetooth/ath3k.c > > @@ -50,7 +50,7 @@ > > #define ATH3K_NAME_LEN 0xFF > > > > struct ath3k_version { > > - unsigned int rom_version; > > + __le32 rom_version; > > unsigned int build_version; > > unsigned int ram_version; > > unsigned char ref_clock; > > so I think this struct should be __packed in the first place and use > strictly typed variables. > > Seems like all fields have an endian issue. > > > @@ -375,7 +375,7 @@ static int ath3k_load_patch(struct usb_device *udev) > > return ret; > > } > > > > - pt_version.rom_version = *(int *)(firmware->data + firmware->size > - 8); > > + pt_version.rom_version = cpu_to_le32(*(int *)(firmware->data + > firmware->size - 8)); > > pt_version.build_version = *(int *) > > (firmware->data + firmware->size - 4); > > I have no idea what this code is doing since that is just crazy. The > variables are unsigned int and are now casted into int and not to mention > that it is unaligned access. Has this code every been run on anything other > than x86 at all. Does it happen to just work by pure luck. > > Regards > > Marcel > > -- Best, Surendra Patil --047d7b5d839121d34a04f585dd85 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

Thanks Marcel for the feedback,

In my p= revious patch joe/johan suggested me to make rom_version to __le32 , when i= made changes i got an warning while compilation as "=A0rom_versio= n expects __le32 getting unsigned int=A0" hence i added second = changes cpu_to_32().

I am confused about what should be =A0the proper change= ? I have only built the module on x86 with sparse.

On Wed, Mar 26, 2014 at= 12:21 AM, Marcel Holtmann <marcel@holtmann.org> wrote:

Hi Surendra,

> To fix the sparse warning "cast to restricted __le32" marked=
> "rom_version" =A0to __le32 instead of unsigned int in "= struct ath3k_version"
> and added cpu_to_le32() for the expression assigning int value to &quo= t;rom_version".
> Successfully built the module without warnings and errors on x86 machi= ne with sparse.
>
> Signed-off-by: Surendra Patil <surendra.tux@gmail.com>
> ---
> drivers/bluetooth/ath3k.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
> index be571fe..5564207 100644
> --- a/drivers/bluetooth/ath3k.c
> +++ b/drivers/bluetooth/ath3k.c
> @@ -50,7 +50,7 @@
> #define ATH3K_NAME_LEN =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A00xFF
>
> struct ath3k_version {
> - =A0 =A0 unsigned int =A0 =A0rom_version;
> + =A0 =A0 __le32 =A0 =A0 =A0 =A0 =A0rom_version;
> =A0 =A0 =A0 unsigned int =A0 =A0build_version;
> =A0 =A0 =A0 unsigned int =A0 =A0ram_version;
> =A0 =A0 =A0 unsigned char =A0 ref_clock;

so I think this struct should be __packed in the first place and use = strictly typed variables.

Seems like all fields have an endian issue.

> @@ -375,7 +375,7 @@ static int ath3k_load_patch(struct usb_device *ude= v)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ret;
> =A0 =A0 =A0 }
>
> - =A0 =A0 pt_version.rom_version =3D *(int *)(firmware->data + firm= ware->size - 8);
> + =A0 =A0 pt_version.rom_version =3D cpu_to_le32(*(int *)(firmware->= ;data + firmware->size - 8));
> =A0 =A0 =A0 pt_version.build_version =3D *(int *)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 (firmware->data + firmware->size - 4= );

I have no idea what this code is doing since that is just crazy. The = variables are unsigned int and are now casted into int and not to mention t= hat it is unaligned access. Has this code every been run on anything other = than x86 at all. Does it happen to just work by pure luck.

Regards

Marcel

--

Best,

Surendra Patil

--047d7b5d839121d34a04f585dd85--