| Message ID | 4E8C78E8.3010605@hartkopp.net |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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(®s->rx.dlr) & 0xf); > > if (!(frame->can_id & CAN_RTR_FLAG)) { > void __iomem *data = ®s->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(®s->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
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(®s->rx.dlr) & 0xf); >> >> if (!(frame->can_id & CAN_RTR_FLAG)) { >> void __iomem *data = ®s->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(®s->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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(®s->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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(®s->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 = ®s->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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 = ®s->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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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
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----- > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = ®s->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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = ®s->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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = ®s->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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(®s->rx.dlr) & 0xf); if (!(frame->can_id & CAN_RTR_FLAG)) { void __iomem *data = ®s->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(®s->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;
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html