Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

[Andreas Werner]

On Mon, Aug 08, 2016 at 03:06:33PM +0200, Kurt Van Dijck wrote:
> 
> --- Original message ---
> > Date:	Mon, 8 Aug 2016 14:28:39 +0200
> > From: Wolfgang Grandegger <wg@xxxxxxxxxxxxxx>
> > 
> [...]
> > >>>+
> > >>>+	if (!(cf->can_id & CAN_RTR_FLAG)) {
> > >>>+		writel(data[0], &cf_buf->data[0]);
> > >>>+		writel(data[1], &cf_buf->data[1]);
> > >>
> > >>Why do you not check cf->can_dlc here as well. And is the extra copy
> > >>necessary.
> > >>
> > >
> > >Yes, I agree with you. The extra copy could be also avoided.
> > >
> > >>>+
> > >>>+		stats->tx_bytes += cf->can_dlc;
> > >>>+	}
> > >>
> > >>If I look to other drivers, they write the data even in case of RTR.
> > >>
> > >
> > >But why?
> > >
> > >A RTR does not have any data, therefore there is no need to write the data.
> > >Only the length is required as the request size.
> > 
> > Yes; I'm wondering as well.
> > 
> > >
> > >If there is a reason behind writing the data of a RTR frame, I can
> > >change that, but for now there is no reason.
> > 
> > Yep.
> 
> I _think_ that copying the data without checking the RTR bit clearly
> avoids a condition and might produce faster code on some machines.
> In any case, it reads easier.
> I'm not sure how that interacts with caches etc etc.
> 
> On the other hand, giving unused data is a bad habit that may reveal
> security information on some places, so better avoid it.
> 
> Kurt

Hi Kurt,
thanks for your comment.

In my opinion, I really prever to NOT copying such data if the RTR flag ist set.

Regards
Andy