Re: [PATCH v3 1/2] spi: add support for DLN-2 USB-SPI adapter

[Johan Hovold <johan@kernel.org>]

On Wed, Nov 19, 2014 at 11:53:44AM +0200, Laurentiu Palcu wrote:
> On Tue, Nov 18, 2014 at 06:06:27PM +0100, Johan Hovold wrote:
> > On Tue, Nov 18, 2014 at 02:09:58PM +0200, Laurentiu Palcu wrote:
> > 
> > > +/*
> > > + * Copy the data to DLN2 buffer and change the byte order to LE, requested by
> > > + * DLN2 module. SPI core makes sure that the data length is a multiple of word
> > > + * size.
> > > + */
> > > +static int dln2_spi_copy_to_buf(u8 *dln2_buf, const u8 *src, u16 len, u8 bpw)
> > > +{
> > > +#ifdef __LITTLE_ENDIAN
> > > +	memcpy(dln2_buf, src, len);
> > > +#else
> > > +	if (bpw <= 8) {
> > > +		memcpy(dln2_buf, src, len);
> > > +	} else if (bpw <= 16) {
> > > +		__le16 *d = (__le16 *)dln2_buf;
> > > +		u16 *s = (u16 *)src;
> > > +
> > > +		len = len / 2;
> > > +		while (len--)
> > > +			put_unaligned_le16(get_unaligned(s++), d++);
> > 
> > You said the dln2 buffer was properly aligned, right? Then you don't
> > need to use put_unaligned_lexx here...
>
> I know, and I was very close not to use it, but, as you said, others
> might not be as careful

That was in a different context. :)

> and if Diolan FW changes anything in the
> header's structure in the future, the alignment will be off. This is
> just a precaution. I know it's a little bit costly but this will affect
> only BE machines and only if bits-per-word is bigger than 8.

I don't think you should worry about hypothetical changes to the
protocol.

You verified the that headers are a multiple of four, so no need for the
unaligned macros.

> > > +	} else {
> > > +		__le32 *d = (__le32 *)dln2_buf;
> > > +		u32 *s = (u32 *)src;
> > > +
> > > +		len = len / 4;
> > > +		while (len--)
> > > +			put_unaligned_le32(get_unaligned(s++), d++);
> > > +	}
> > > +#endif
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * Copy the data from DLN2 buffer and convert to CPU byte order since the DLN2
> > > + * buffer is LE ordered. SPI core makes sure that the data length is a multiple
> > > + * of word size.
> > > + */
> > > +static int dln2_spi_copy_from_buf(u8 *dest, const u8 *dln2_buf, u16 len, u8 bpw)
> > > +{
> > > +#ifdef __LITTLE_ENDIAN
> > > +	memcpy(dest, dln2_buf, len);
> > > +#else
> > > +	if (bpw <= 8) {
> > > +		memcpy(dest, dln2_buf, len);
> > > +	} else if (bpw <= 16) {
> > > +		u16 *d = (u16 *)dest;
> > > +		__le16 *s = (__le16 *)dln2_buf;
> > > +
> > > +		len = len / 2;
> > > +		while (len--)
> > > +			put_unaligned(get_unaligned_le16(s++), d++)
> > 
> > ...or get_unaligned_lexx here.
> > 
> > > +	} else {
> > > +		u32 *d = (u32 *)dest;
> > > +		__le32 *s = (__le32 *)dln2_buf;
> > > +
> > > +		len = len / 4;
> > > +		while (len--)
> > > +			put_unaligned(get_unaligned_le32(s++), d++)
> > > +	}
> > > +#endif
> > > +
> > > +	return 0;
> > > +}
> > 
> > Did you check the alignment of the SPI buffers as well? I'd assume they
> > were DMA-able and thus properly aligned, and then you do not need to use
> > any unaligned helpers above at all.
>
> I did... The spi_transfer struct documentation says that those tx_buf
> and rx_buf buffers are dma-safe but, apparently, I found drivers that do
> not really use alligned buffers for transfers. Look, for example, at
> max1111.c. Those buffers don't look aligned to me...

That looks like a bug to me. DMA-buffers should never be allocated as
part of a larger structure.

> Well, this driver
> is probably not the best example since BPW is 8, but you got my point.
> Other drivers, indeed, use the ____cacheline_aligned attribute and
> should be dma-safe.

I think you can remove the unaligned macros altogether.

Johan