Messages in this thread |  | | From | Arnd Bergmann <> | Subject | Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery | Date | Thu, 5 Dec 2013 03:38:54 +0100 |
| |
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
|  |