On Wednesday 04 December 2013, Frank Haverkamp wrote:
> Hi Arnd & Greg,
>
> please let me know if my following changes are ok:
>
> Am Dienstag, den 03.12.2013, 15:28 +0100 schrieb Frank Haverkamp:
>
> > +/* Read/write from/to registers */
> > +struct genwqe_regs_io {
> > + __u32 num; /* register offset/address */
> > + union {
> > + __u64 val64;
> > + __u32 val32;
> > + __u16 define;
> > + };
> > +};
>
> Here I am using now:
>
> struct genwqe_regs_io {
> __u64 num; /* register offset/address */
> union {
> __u64 val64;
> __u32 val32;
> __u16 define;
> };
> };
This is not a bug anymore, but it seems pointless to use a union
there rather than just a __u64 for the value.
> Here I reordered and resized the members like this:
>
> struct genwqe_bitstream {
> __u64 data_addr; /* pointer to image data */
> __u32 size; /* size of image file */
> __u32 crc; /* crc of this image */
> __u64 target_addr; /* starting address in Flash */
> __u32 partition; /* '0', '1', or 'v' */
> __u32 uid; /* 1=host/x=dram */
>
> __u64 slu_id; /* informational/sim: SluID */
> __u64 app_id; /* informational/sim: AppID */
>
> __u16 retc; /* returned from processing */
> __u16 attn; /* attention code from processing */
> __u32 progress; /* progress code from processing */
> };
Yes, this is fine.
> > +struct genwqe_debug_data {
> > + char driver_version[64];
> > + __u64 slu_unitcfg;
> > + __u64 app_unitcfg;
> > +
> > + __u8 ddcb_before[DDCB_LENGTH];
> > + __u8 ddcb_prev[DDCB_LENGTH];
> > + __u8 ddcb_finished[DDCB_LENGTH];
> > +};
> > +
>
> This I hope is ok. DDCB_LENGTH is 256.
Yes.
>
> Was this already ok? My new version looks as follows:
The old version was wrong.
> struct genwqe_ddcb_cmd {
> /* START of data copied to/from driver */
> __u64 next_addr; /* chaining genwqe_ddcb_cmd */
> __u64 flags; /* reserved */
>
> __u8 acfunc; /* accelerators functional unit */
> __u8 cmd; /* command to execute */
> __u8 asiv_length; /* used parameter length */
> __u8 asv_length; /* length of valid return values */
> __u16 cmdopts; /* command options */
> __u16 retc; /* return code from processing */
> __u16 attn; /* attention code from processing */
> __u16 vcrc; /* variant crc16 */
> __u32 progress; /* progress code from processing */
>
> __u64 deque_ts; /* dequeue time stamp */
> __u64 cmplt_ts; /* completion time stamp */
> __u64 disp_ts; /* SW processing start */
>
> /* move to end and avoid copy-back */
> __u64 ddata_addr; /* collect debug data */
>
> /* command specific values */
> __u8 asv[DDCB_ASV_LENGTH];
>
> /* END of data copied from driver */
> union {
> struct {
> __u64 ats;
> __u8 asiv[DDCB_ASIV_LENGTH_ATS];
> };
> /* used for flash update to keep it backward compatible */
> __u8 __asiv[DDCB_ASIV_LENGTH];
> };
> /* END of data copied to driver */
> };
>
> Trying to group the data in 64bit chunks even nicer than I had it
> before.
Yes, this works, although I would argue that it is too complex to be a nice
interface.
> > +/**
> > + * struct genwqe_mem - Memory pinning/unpinning information
> > + * @addr: virtual user space address
> > + * @size: size of the area pin/dma-map/unmap
> > + * direction: 0: read/1: read and write
> > + *
> > + * Avoid pinning and unpinning of memory pages dynamically. Instead
> > + * the idea is to pin the whole buffer space required for DDCB
> > + * opertionas in advance. The driver will reuse this pinning and the
> > + * memory associated with it to setup the sglists for the DDCB
> > + * requests without the need to allocate and free memory or map and
> > + * unmap to get the DMA addresses.
> > + *
> > + * The inverse operation needs to be called after the pinning is not
> > + * needed anymore. The pinnings else the pinnings will get removed
> > + * after the device is closed. Note that pinnings will required
> > + * memory.
> > + */
> > +struct genwqe_mem {
> > + unsigned long addr;
> > + unsigned long size;
> > + int direction;
> > +};
>
> Was wrong, as already pointed out before. It is now:
>
> struct genwqe_mem {
> __u64 addr;
> __u64 size;
> int direction;
> };
>
> I hope the int is ok here.
No, it's not. The problem is that sizeof(struct genwqe_mem) is now 24 on
most architectures (including x86-64) and 20 on x86-32. The size gets
encoded into the ioctl number, at least after you fix this part:
> > +
> > +#define GENWQE_PIN_MEM _IOWR(GENWQE_IOC_CODE, 40, struct
> > genwqe_mem *)
> > +#define GENWQE_UNPIN_MEM _IOWR(GENWQE_IOC_CODE, 41, struct
> > genwqe_mem *)
... which is also broken because sizeof(struct genwqe_mem *) is 8 on
64-bit and 4 on 32-bit architectures. The argument to _IOWR() is
supposed to be struct, not a pointer. I thought we would actually
cause a build-time warning about this bug (I wrote the code to do that)
but I may be misremembering which bugs we can actually catch.
> > +/*
> > + * Generic synchronous DDCB execution interface.
> > + * Synchronously execute a DDCB.
> > + *
> > + * Return: 0 on success or negative error code.
> > + * -EINVAL: Invalid parameters (ASIV_LEN, ASV_LEN, illegal
> > fixups
> > + * no mappings found/could not create mappings
> > + * -EFAULT: illegal addresses in fixups, purging failed
> > + * -EBADMSG: enqueing failed, retc != DDCB_RETC_COMPLETE
> > + */
> > +#define GENWQE_EXECUTE_DDCB \
> > + _IOWR(GENWQE_IOC_CODE, 50, struct genwqe_ddcb_cmd *)
> > +
> > +#define
> > GENWQE_EXECUTE_RAW_DDCB \
> > + _IOWR(GENWQE_IOC_CODE, 51, struct genwqe_ddcb_cmd *)
> > +
> > +/* Service Layer functions (PF only) */
> > +#define GENWQE_SLU_UPDATE _IOWR(GENWQE_IOC_CODE, 80, struct
> > genwqe_bistream *)
> > +#define GENWQE_SLU_READ _IOWR(GENWQE_IOC_CODE, 81, struct
> > genwqe_bistream *)
Same bug for all of these.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
