[PATCH net] mscan: zero accidentally copied register content

[PATCH net] mscan: zero accidentally copied register content

[Oliver Hartkopp]

Due to the 16 bit access to mscan registers there's too much data copied to
the zero initialized CAN frame when having an odd number of bytes to copy.
This patch clears the data byte read from the invalid register entry.

Reported-by: Andre Naujoks <nautsch@gmail.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

---

Hello Wolf[gang|ram],

from an error report from Andre Naujoks i tracked down the problem of
uninitialized data in (normally) initialized CAN frames to the mscan driver.

Regards,
Oliver


diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
index 92feac6..1b60fbe 100644
--- a/drivers/net/can/mscan/mscan.c
+++ b/drivers/net/can/mscan/mscan.c
@@ -327,20 +327,23 @@ static void mscan_get_rx_frame(struct net_device *dev, struct can_frame *frame)
 	frame->can_dlc = get_can_dlc(in_8(&regs->rx.dlr) & 0xf);
 
 	if (!(frame->can_id & CAN_RTR_FLAG)) {
 		void __iomem *data = &regs->rx.dsr1_0;
 		u16 *payload = (u16 *)frame->data;
 
 		for (i = 0; i < (frame->can_dlc + 1) / 2; i++) {
 			*payload++ = in_be16(data);
 			data += 2 + _MSCAN_RESERVED_DSR_SIZE;
 		}
+		/* zero accidentally copied register content at odd DLCs */
+		if (frame->can_dlc & 1)
+			frame->data[frame->can_dlc] = 0;
 	}
 
 	out_8(&regs->canrflg, MSCAN_RXF);
 }
 
 static void mscan_get_err_frame(struct net_device *dev, struct can_frame *frame,
 				u8 canrflg)
 {
 	struct mscan_priv *priv = netdev_priv(dev);
 	struct mscan_regs *regs = (struct mscan_regs *)priv->reg_base;

Re: [PATCH net] mscan: zero accidentally copied register content

[Wolfram Sang]

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

On Wed, Oct 05, 2011 at 05:34:00PM +0200, Oliver Hartkopp wrote:
> Due to the 16 bit access to mscan registers there's too much data copied to
> the zero initialized CAN frame when having an odd number of bytes to copy.
> This patch clears the data byte read from the invalid register entry.
> 
> Reported-by: Andre Naujoks <nautsch@gmail.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> ---
> 
> Hello Wolf[gang|ram],
> 
> from an error report from Andre Naujoks i tracked down the problem of
> uninitialized data in (normally) initialized CAN frames to the mscan driver.
> 
> Regards,
> Oliver
> 
> 
> diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
> index 92feac6..1b60fbe 100644
> --- a/drivers/net/can/mscan/mscan.c
> +++ b/drivers/net/can/mscan/mscan.c
> @@ -327,20 +327,23 @@ static void mscan_get_rx_frame(struct net_device *dev, struct can_frame *frame)
>  	frame->can_dlc = get_can_dlc(in_8(&regs->rx.dlr) & 0xf);
>  
>  	if (!(frame->can_id & CAN_RTR_FLAG)) {
>  		void __iomem *data = &regs->rx.dsr1_0;
>  		u16 *payload = (u16 *)frame->data;
>  
>  		for (i = 0; i < (frame->can_dlc + 1) / 2; i++) {
>  			*payload++ = in_be16(data);
>  			data += 2 + _MSCAN_RESERVED_DSR_SIZE;
>  		}
> +		/* zero accidentally copied register content at odd DLCs */
> +		if (frame->can_dlc & 1)
> +			frame->data[frame->can_dlc] = 0;
>  	}
>  
>  	out_8(&regs->canrflg, MSCAN_RXF);

Nice catch, but wouldn't it be more elegant to never have an invalid byte
in the first place?

if (can_dlc & 1)
	*payload = in_be16() & mask;

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH net] mscan: zero accidentally copied register content

[Oliver Hartkopp]

On 10/05/11 17:51, Wolfram Sang wrote:

> On Wed, Oct 05, 2011 at 05:34:00PM +0200, Oliver Hartkopp wrote:
>> Due to the 16 bit access to mscan registers there's too much data copied to
>> the zero initialized CAN frame when having an odd number of bytes to copy.
>> This patch clears the data byte read from the invalid register entry.
>>
>> Reported-by: Andre Naujoks <nautsch@gmail.com>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>
>> ---
>>
>> Hello Wolf[gang|ram],
>>
>> from an error report from Andre Naujoks i tracked down the problem of
>> uninitialized data in (normally) initialized CAN frames to the mscan driver.
>>
>> Regards,
>> Oliver
>>
>>
>> diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
>> index 92feac6..1b60fbe 100644
>> --- a/drivers/net/can/mscan/mscan.c
>> +++ b/drivers/net/can/mscan/mscan.c
>> @@ -327,20 +327,23 @@ static void mscan_get_rx_frame(struct net_device *dev, struct can_frame *frame)
>>  	frame->can_dlc = get_can_dlc(in_8(&regs->rx.dlr) & 0xf);
>>  
>>  	if (!(frame->can_id & CAN_RTR_FLAG)) {
>>  		void __iomem *data = &regs->rx.dsr1_0;
>>  		u16 *payload = (u16 *)frame->data;
>>  
>>  		for (i = 0; i < (frame->can_dlc + 1) / 2; i++) {
>>  			*payload++ = in_be16(data);
>>  			data += 2 + _MSCAN_RESERVED_DSR_SIZE;
>>  		}
>> +		/* zero accidentally copied register content at odd DLCs */
>> +		if (frame->can_dlc & 1)
>> +			frame->data[frame->can_dlc] = 0;
>>  	}
>>  
>>  	out_8(&regs->canrflg, MSCAN_RXF);
> 
> Nice catch, but wouldn't it be more elegant to never have an invalid byte
> in the first place?
> 
> if (can_dlc & 1)
> 	*payload = in_be16() & mask;
> 


Hm, then i would rather think about changing the for() statement and to read
byte-by-byte instead of the current in_be16() usage with the 16bit access
drawbacks ...

Regards,
Oliver

Re: [PATCH net] mscan: zero accidentally copied register content

[Oliver Hartkopp]

On 10/05/11 18:10, Oliver Hartkopp wrote:

> On 10/05/11 17:51, Wolfram Sang wrote:

>>> +		/* zero accidentally copied register content at odd DLCs */
>>> +		if (frame->can_dlc & 1)
>>> +			frame->data[frame->can_dlc] = 0;
>>>  	}
>>>  
>>>  	out_8(&regs->canrflg, MSCAN_RXF);
>>
>> Nice catch, but wouldn't it be more elegant to never have an invalid byte
>> in the first place?
>>
>> if (can_dlc & 1)
>> 	*payload = in_be16() & mask;
>>
> 
> 
> Hm, then i would rather think about changing the for() statement and to read
> byte-by-byte instead of the current in_be16() usage with the 16bit access
> drawbacks ...
> 


I think if one would like to rework the 16bit register access (which is used
in the rx path /and/ in the tx path also) this should go via net-next after
some discussion and testing.

IMHO this fix is small and clear and especially not risky. I wonder if
reworking the 16 bit register access is worth the effort?

Regards,
Oliver

Re: [PATCH net] mscan: zero accidentally copied register content

[Wolfgang Grandegger]

Hi Oliver,

On 10/06/2011 09:02 AM, Oliver Hartkopp wrote:
> On 10/05/11 18:10, Oliver Hartkopp wrote:
> 
>> On 10/05/11 17:51, Wolfram Sang wrote:
> 
>>>> +		/* zero accidentally copied register content at odd DLCs */
>>>> +		if (frame->can_dlc & 1)
>>>> +			frame->data[frame->can_dlc] = 0;
>>>>  	}
>>>>  
>>>>  	out_8(&regs->canrflg, MSCAN_RXF);
>>>
>>> Nice catch, but wouldn't it be more elegant to never have an invalid byte
>>> in the first place?
>>>
>>> if (can_dlc & 1)
>>> 	*payload = in_be16() & mask;
>>>
>>
>>
>> Hm, then i would rather think about changing the for() statement and to read
>> byte-by-byte instead of the current in_be16() usage with the 16bit access
>> drawbacks ...
>>
> 
> 
> I think if one would like to rework the 16bit register access (which is used
> in the rx path /and/ in the tx path also) this should go via net-next after
> some discussion and testing.

Why do you want to change 16-bit accesses in general? They are faster
than two 8 bit accesses. 

> IMHO this fix is small and clear and especially not risky. I wonder if
> reworking the 16 bit register access is worth the effort?

I would prefer:

 	if (!(frame->can_id & CAN_RTR_FLAG)) {
 		void __iomem *data = &regs->rx.dsr1_0;
 		u16 *payload = (u16 *)frame->data;
 
 		for (i = 0; i < frame->can_dlc / 2; i++) {
 			*payload++ = in_be16(data);
 			data += 2 + _MSCAN_RESERVED_DSR_SIZE;
 		}
		/* copy remaining byte */
		if (frame->can_dlc & 1)
			frame->data[frame->can_dlc - 1] = in_8(data);
 	}

Wolfgang.

Re: [PATCH net] mscan: zero accidentally copied register content

[Wolfram Sang]

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


> Why do you want to change 16-bit accesses in general? They are faster
> than two 8 bit accesses. 

Yup, was thinking the same.

> 
> > IMHO this fix is small and clear and especially not risky. I wonder if
> > reworking the 16 bit register access is worth the effort?
> 
> I would prefer:
> 
>  	if (!(frame->can_id & CAN_RTR_FLAG)) {
>  		void __iomem *data = &regs->rx.dsr1_0;
>  		u16 *payload = (u16 *)frame->data;
>  
>  		for (i = 0; i < frame->can_dlc / 2; i++) {
>  			*payload++ = in_be16(data);
>  			data += 2 + _MSCAN_RESERVED_DSR_SIZE;
>  		}
> 		/* copy remaining byte */

if any

> 		if (frame->can_dlc & 1)
> 			frame->data[frame->can_dlc - 1] = in_8(data);

Ack.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH net] mscan: zero accidentally copied register content

[Oliver Hartkopp]

On 10/06/11 11:24, Wolfram Sang wrote:

> 
>> Why do you want to change 16-bit accesses in general? They are faster
>> than two 8 bit accesses. 
> 
> Yup, was thinking the same.


Ah, i did not get this from your code example

	if (can_dlc & 1)
		*payload = in_be16() & mask;


which probably does the same as Wolfgangs more obvious suggestion

	if (frame->can_dlc & 1)
		frame->data[frame->can_dlc - 1] = in_8(data);


:-)

As my patch could be done without real testing, as i did not change the
register access and only fixed the result ...

	if (frame->can_dlc & 1)
		frame->data[frame->can_dlc] = 0;


... it would be nice if e.g. Wolfgang could send his patch after some testing,
as i currently don't have access to my MPC5200 hardware here.

Tnx & best regards,
Oliver

Re: [PATCH net] mscan: zero accidentally copied register content

[Wolfram Sang]

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


> Ah, i did not get this from your code example
> 
> 	if (can_dlc & 1)
> 		*payload = in_be16() & mask;

Sorry, I thought it was obvious that I was using pseudo_code here :)

> ... it would be nice if e.g. Wolfgang could send his patch after some testing,
> as i currently don't have access to my MPC5200 hardware here.

What about Andre?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH net] mscan: zero accidentally copied register content

[Andre Naujoks]

Hi.

2011/10/6 Wolfram Sang <w.sang@pengutronix.de>
>
> > Ah, i did not get this from your code example
> >
> >       if (can_dlc & 1)
> >               *payload = in_be16() & mask;
>
> Sorry, I thought it was obvious that I was using pseudo_code here :)
>
> > ... it would be nice if e.g. Wolfgang could send his patch after some testing,
> > as i currently don't have access to my MPC5200 hardware here.
>
> What about Andre?

I am currently trying to test the first version Oliver sent. I am
currently having some problems
with my hardware and I don't know what it is yet.

I will give Wolfgangs suggestion a try after my problems are sorted out here.

Regards
  Andre

>
> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk6NtoAACgkQD27XaX1/VRudMQCeMX3yMc+Au65BwKDocJNXtG/d
> RxgAoJnY/p0csrHa0o/7SpnQdtEhEOQn
> =Sxlb
> -----END PGP SIGNATURE-----
>

Re: [PATCH net] mscan: zero accidentally copied register content

[Oliver Hartkopp]

On 10/06/11 11:09, Wolfgang Grandegger wrote:

> On 10/06/2011 09:02 AM, Oliver Hartkopp wrote:
>>
>> I think if one would like to rework the 16bit register access (which is used
>> in the rx path /and/ in the tx path also) this should go via net-next after
>> some discussion and testing.
> 
> Why do you want to change 16-bit accesses in general? They are faster
> than two 8 bit accesses. 
> 
>> IMHO this fix is small and clear and especially not risky. I wonder if
>> reworking the 16 bit register access is worth the effort?
> 
> I would prefer:
> 
>  	if (!(frame->can_id & CAN_RTR_FLAG)) {
>  		void __iomem *data = &regs->rx.dsr1_0;
>  		u16 *payload = (u16 *)frame->data;
>  
>  		for (i = 0; i < frame->can_dlc / 2; i++) {
>  			*payload++ = in_be16(data);
>  			data += 2 + _MSCAN_RESERVED_DSR_SIZE;
>  		}
> 		/* copy remaining byte */
> 		if (frame->can_dlc & 1)
> 			frame->data[frame->can_dlc - 1] = in_8(data);
>  	}


Besides the fact that Andre is going to test this idea from Wolfgang now, are
you really sure that it must be

	in_8(data)

and not

	in_8(data+1)

???

And that data definitely points to the right place?

I would prefer to be really cautious with these big endian 16 bit registers!

Therefore my fix with

+		/* zero accidentally copied register content at odd DLCs */
+		if (frame->can_dlc & 1)
+			frame->data[frame->can_dlc] = 0;

only repairing the result looks much more defensive to me.

Regards,
Oliver

Re: [PATCH net] mscan: zero accidentally copied register content

[Andre Naujoks]

2011/10/6 Oliver Hartkopp <socketcan@hartkopp.net>:
> On 10/06/11 11:09, Wolfgang Grandegger wrote:
>
>> On 10/06/2011 09:02 AM, Oliver Hartkopp wrote:
>>>
>>> I think if one would like to rework the 16bit register access (which is used
>>> in the rx path /and/ in the tx path also) this should go via net-next after
>>> some discussion and testing.
>>
>> Why do you want to change 16-bit accesses in general? They are faster
>> than two 8 bit accesses.
>>
>>> IMHO this fix is small and clear and especially not risky. I wonder if
>>> reworking the 16 bit register access is worth the effort?
>>
>> I would prefer:
>>
>>       if (!(frame->can_id & CAN_RTR_FLAG)) {
>>               void __iomem *data = &regs->rx.dsr1_0;
>>               u16 *payload = (u16 *)frame->data;
>>
>>               for (i = 0; i < frame->can_dlc / 2; i++) {
>>                       *payload++ = in_be16(data);
>>                       data += 2 + _MSCAN_RESERVED_DSR_SIZE;
>>               }
>>               /* copy remaining byte */
>>               if (frame->can_dlc & 1)
>>                       frame->data[frame->can_dlc - 1] = in_8(data);
>>       }
>
>
> Besides the fact that Andre is going to test this idea from Wolfgang now, are
> you really sure that it must be
>
>        in_8(data)
>
> and not
>
>        in_8(data+1)
>
> ???
>
> And that data definitely points to the right place?
>
> I would prefer to be really cautious with these big endian 16 bit registers!
>
> Therefore my fix with
>
> +               /* zero accidentally copied register content at odd DLCs */
> +               if (frame->can_dlc & 1)
> +                       frame->data[frame->can_dlc] = 0;
>
> only repairing the result looks much more defensive to me.

First things first: Both ways seem to work correctly. At least on the
MPC5200 I have here.

But I am with Oliver on this one. The solution looks much simpler and
endianess errors are not possible. If the few CPU cycles are worth it
on the other hand, then Wolfgangs version is probably preferable. I
don't have access to this kind of hardware on a little endian machine
to test it, though.

Greetings
  Andre

>
> Regards,
> Oliver
>

Re: [PATCH net] mscan: zero accidentally copied register content

[Wolfgang Grandegger]

On 10/06/2011 05:03 PM, Andre Naujoks wrote:
> 2011/10/6 Oliver Hartkopp <socketcan@hartkopp.net>:
>> On 10/06/11 11:09, Wolfgang Grandegger wrote:
>>
>>> On 10/06/2011 09:02 AM, Oliver Hartkopp wrote:
>>>>
>>>> I think if one would like to rework the 16bit register access (which is used
>>>> in the rx path /and/ in the tx path also) this should go via net-next after
>>>> some discussion and testing.
>>>
>>> Why do you want to change 16-bit accesses in general? They are faster
>>> than two 8 bit accesses.
>>>
>>>> IMHO this fix is small and clear and especially not risky. I wonder if
>>>> reworking the 16 bit register access is worth the effort?
>>>
>>> I would prefer:
>>>
>>>       if (!(frame->can_id & CAN_RTR_FLAG)) {
>>>               void __iomem *data = &regs->rx.dsr1_0;
>>>               u16 *payload = (u16 *)frame->data;
>>>
>>>               for (i = 0; i < frame->can_dlc / 2; i++) {
>>>                       *payload++ = in_be16(data);
>>>                       data += 2 + _MSCAN_RESERVED_DSR_SIZE;
>>>               }
>>>               /* copy remaining byte */
>>>               if (frame->can_dlc & 1)
>>>                       frame->data[frame->can_dlc - 1] = in_8(data);
>>>       }
>>
>>
>> Besides the fact that Andre is going to test this idea from Wolfgang now, are
>> you really sure that it must be
>>
>>        in_8(data)

That should be the right byte.

>>
>> and not
>>
>>        in_8(data+1)
>>
>> ???
>>
>> And that data definitely points to the right place?
>>
>> I would prefer to be really cautious with these big endian 16 bit registers!
>>
>> Therefore my fix with
>>
>> +               /* zero accidentally copied register content at odd DLCs */
>> +               if (frame->can_dlc & 1)
>> +                       frame->data[frame->can_dlc] = 0;
>>
>> only repairing the result looks much more defensive to me.
> 
> First things first: Both ways seem to work correctly. At least on the
> MPC5200 I have here.
> 
> But I am with Oliver on this one. The solution looks much simpler and
> endianess errors are not possible. If the few CPU cycles are worth it
> on the other hand, then Wolfgangs version is probably preferable. I
> don't have access to this kind of hardware on a little endian machine
> to test it, though.

Well, copying just the relevant bytes seem much more straight-forward
than removing accidentally copied bytes later-on. You do not need to
care about little endian. The MSCAN is only available on PowerPC SOCs,
which are big endian.

I'm going to test and post a patch tomorrow.

Wolfgang.

Re: [PATCH net] mscan: zero accidentally copied register content

[Oliver Hartkopp]

On 10/06/11 20:24, Wolfgang Grandegger wrote:


> Well, copying just the relevant bytes seem much more straight-forward
> than removing accidentally copied bytes later-on. You do not need to
> care about little endian. The MSCAN is only available on PowerPC SOCs,
> which are big endian.
> 
> I'm going to test and post a patch tomorrow.


Thanks.

My patch is then superseded by this one

"mscan: too much data copied to CAN frame due to 16 bit accesses"

http://patchwork.ozlabs.org/patch/118364/

Tnx,
Oliver

Re: [PATCH net] mscan: zero accidentally copied register content

[Marc Kleine-Budde]

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

On 10/05/2011 05:34 PM, Oliver Hartkopp wrote:
> Due to the 16 bit access to mscan registers there's too much data copied to
> the zero initialized CAN frame when having an odd number of bytes to copy.
> This patch clears the data byte read from the invalid register entry.
> 
> Reported-by: Andre Naujoks <nautsch@gmail.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

This problem have some other drivers, too, e.g. the at91 and the flexcan
driver both copy unconditionally all 8 bytes from the hardware. However,
I don't know if the hardware sets the remaining bytes to zero.

cheers, Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]