Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi,
Am Freitag, den 06.12.2013, 09:39 +0100 schrieb Frank Haverkamp:
> Hi Arnd,
>
> Am Donnerstag, den 05.12.2013, 21:31 +0100 schrieb Arnd Bergmann:
> > On Thursday 05 December 2013, Frank Haverkamp wrote:
> > > > > 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.
> > >
> > > Interesting. So int is like long architecture specific. I changed it to
> > > be __u64 too, to avoid any problem.
> >
> > The solution is ok, but the problem is different from what you thought:
> >
> > On all architectures that Linux runs on, 'int' is 32 bit. The problem is
> > again the alignment of __u64. On normal architectures, it is naturally
> > aligned, and gcc adds 4 byte padding so that 'sizeof (struct genwqe_mem)'
> > is multiple of the required alignment. On x86-32, the required alignment
> > for the __u64 members is only 4 bytes, so no padding is added.
> >
> > Arnd
I took some time and converted my test-applications, such that I can
compile them as 32-bit binaries. I had to introduce the
compatibility_ioctl to get the driver working with that. Therefore I
post now my new version v10 which has this change as well.
> now I understand. Interesting. I wondered how one could check this
> automatically such that others don't repeat the mistakes I did.
>
> I hope I have fixed all those issues now in my latest posting, or do you
> still see some?
>
> Other than that, is the code ready for inclusion now, or do you still
> like to have other changes done (hopefully not, but if so which ones)?
>
> Thanks
>
> Frank
Regards
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi,
Am Freitag, den 06.12.2013, 09:39 +0100 schrieb Frank Haverkamp:
Hi Arnd,
Am Donnerstag, den 05.12.2013, 21:31 +0100 schrieb Arnd Bergmann:
On Thursday 05 December 2013, Frank Haverkamp wrote:
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.
Interesting. So int is like long architecture specific. I changed it to
be __u64 too, to avoid any problem.
The solution is ok, but the problem is different from what you thought:
On all architectures that Linux runs on, 'int' is 32 bit. The problem is
again the alignment of __u64. On normal architectures, it is naturally
aligned, and gcc adds 4 byte padding so that 'sizeof (struct genwqe_mem)'
is multiple of the required alignment. On x86-32, the required alignment
for the __u64 members is only 4 bytes, so no padding is added.
Arnd
I took some time and converted my test-applications, such that I can
compile them as 32-bit binaries. I had to introduce the
compatibility_ioctl to get the driver working with that. Therefore I
post now my new version v10 which has this change as well.
now I understand. Interesting. I wondered how one could check this
automatically such that others don't repeat the mistakes I did.
I hope I have fixed all those issues now in my latest posting, or do you
still see some?
Other than that, is the code ready for inclusion now, or do you still
like to have other changes done (hopefully not, but if so which ones)?
Thanks
Frank
Regards
Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Arnd,
Am Donnerstag, den 05.12.2013, 21:31 +0100 schrieb Arnd Bergmann:
> On Thursday 05 December 2013, Frank Haverkamp wrote:
> > > > 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.
> >
> > Interesting. So int is like long architecture specific. I changed it to
> > be __u64 too, to avoid any problem.
>
> The solution is ok, but the problem is different from what you thought:
>
> On all architectures that Linux runs on, 'int' is 32 bit. The problem is
> again the alignment of __u64. On normal architectures, it is naturally
> aligned, and gcc adds 4 byte padding so that 'sizeof (struct genwqe_mem)'
> is multiple of the required alignment. On x86-32, the required alignment
> for the __u64 members is only 4 bytes, so no padding is added.
>
> Arnd
>
now I understand. Interesting. I wondered how one could check this
automatically such that others don't repeat the mistakes I did.
I hope I have fixed all those issues now in my latest posting, or do you
still see some?
Other than that, is the code ready for inclusion now, or do you still
like to have other changes done (hopefully not, but if so which ones)?
Thanks
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Arnd,
Am Donnerstag, den 05.12.2013, 21:31 +0100 schrieb Arnd Bergmann:
On Thursday 05 December 2013, Frank Haverkamp wrote:
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.
Interesting. So int is like long architecture specific. I changed it to
be __u64 too, to avoid any problem.
The solution is ok, but the problem is different from what you thought:
On all architectures that Linux runs on, 'int' is 32 bit. The problem is
again the alignment of __u64. On normal architectures, it is naturally
aligned, and gcc adds 4 byte padding so that 'sizeof (struct genwqe_mem)'
is multiple of the required alignment. On x86-32, the required alignment
for the __u64 members is only 4 bytes, so no padding is added.
Arnd
now I understand. Interesting. I wondered how one could check this
automatically such that others don't repeat the mistakes I did.
I hope I have fixed all those issues now in my latest posting, or do you
still see some?
Other than that, is the code ready for inclusion now, or do you still
like to have other changes done (hopefully not, but if so which ones)?
Thanks
Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Thursday 05 December 2013, Frank Haverkamp wrote:
> > > 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.
>
> Interesting. So int is like long architecture specific. I changed it to
> be __u64 too, to avoid any problem.
The solution is ok, but the problem is different from what you thought:
On all architectures that Linux runs on, 'int' is 32 bit. The problem is
again the alignment of __u64. On normal architectures, it is naturally
aligned, and gcc adds 4 byte padding so that 'sizeof (struct genwqe_mem)'
is multiple of the required alignment. On x86-32, the required alignment
for the __u64 members is only 4 bytes, so no padding is added.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Arnd,
Am Donnerstag, den 05.12.2013, 03:38 +0100 schrieb Arnd Bergmann:
> 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;
> > };
> > };
Changed in my next version to:
struct genwqe_reg_io {
__u64 num; /* register offset/address */
__u64 val64;
};
which is sufficient as you pointed out.
>
> 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.
> > > + *
> > > +
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Arnd,
Am Donnerstag, den 05.12.2013, 03:27 +0100 schrieb Arnd Bergmann:
> On Wednesday 04 December 2013, Frank Haverkamp wrote:
> > Am Dienstag, den 03.12.2013, 15:28 +0100 schrieb Frank Haverkamp:
> > > + */
> > > +struct genwqe_mem {
> > > + unsigned long addr;
> > > + unsigned long size;
> > > + int direction;
> > > +};
> > > +
> > > +#define GENWQE_PIN_MEM _IOWR(GENWQE_IOC_CODE, 40, struct
> > > genwqe_mem *)
> > > +#define GENWQE_UNPIN_MEM _IOWR(GENWQE_IOC_CODE, 41, struct
> > > genwqe_mem *)
> > > +
> >
> > Before someone comments on the unsigned long and the 32/64 bit issues
> > with it. I need to fix that.
>
> Also the extraneous '*' in the definitions.
Ok, I will remove the '*' and just put the structs there.
>
> Arnd
>
Regards
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Arnd,
Am Donnerstag, den 05.12.2013, 03:27 +0100 schrieb Arnd Bergmann:
On Wednesday 04 December 2013, Frank Haverkamp wrote:
Am Dienstag, den 03.12.2013, 15:28 +0100 schrieb Frank Haverkamp:
+ */
+struct genwqe_mem {
+ unsigned long addr;
+ unsigned long size;
+ int direction;
+};
+
+#define GENWQE_PIN_MEM _IOWR(GENWQE_IOC_CODE, 40, struct
genwqe_mem *)
+#define GENWQE_UNPIN_MEM _IOWR(GENWQE_IOC_CODE, 41, struct
genwqe_mem *)
+
Before someone comments on the unsigned long and the 32/64 bit issues
with it. I need to fix that.
Also the extraneous '*' in the definitions.
Ok, I will remove the '*' and just put the structs there.
Arnd
Regards
Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Arnd,
Am Donnerstag, den 05.12.2013, 03:38 +0100 schrieb Arnd Bergmann:
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;
};
};
Changed in my next version to:
struct genwqe_reg_io {
__u64 num; /* register offset/address */
__u64 val64;
};
which is sufficient as you pointed out.
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
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Thursday 05 December 2013, Frank Haverkamp wrote:
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.
Interesting. So int is like long architecture specific. I changed it to
be __u64 too, to avoid any problem.
The solution is ok, but the problem is different from what you thought:
On all architectures that Linux runs on, 'int' is 32 bit. The problem is
again the alignment of __u64. On normal architectures, it is naturally
aligned, and gcc adds 4 byte padding so that 'sizeof (struct genwqe_mem)'
is multiple of the required alignment. On x86-32, the required alignment
for the __u64 members is only 4 bytes, so no padding is added.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
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
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Wednesday 04 December 2013, Frank Haverkamp wrote:
> Am Dienstag, den 03.12.2013, 15:28 +0100 schrieb Frank Haverkamp:
> > + */
> > +struct genwqe_mem {
> > + unsigned long addr;
> > + unsigned long size;
> > + int direction;
> > +};
> > +
> > +#define GENWQE_PIN_MEM _IOWR(GENWQE_IOC_CODE, 40, struct
> > genwqe_mem *)
> > +#define GENWQE_UNPIN_MEM _IOWR(GENWQE_IOC_CODE, 41, struct
> > genwqe_mem *)
> > +
>
> Before someone comments on the unsigned long and the 32/64 bit issues
> with it. I need to fix that.
Also the extraneous '*' in the definitions.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
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;
};
};
> +
> +/*
> + * All registers of our card will return values not equal this
> values.
> + * If we see IO_ILLEGAL_VALUE on any of our MMIO register reads, the
> + * card can be considered as unusable. It will need recovery.
> + */
> +#define IO_ILLEGAL_VALUE 0xull
> +
> +/*
> + * Generic DDCB execution interface.
> + *
> + * This interface is a first prototype resulting from discussions we
> + * had with other teams which wanted to use the Genwqe card. It
> allows
> + * to issue a DDCB request in a generic way. The request will block
> + * until it finishes or time out with error.
> + *
> + * Some DDCBs require DMA addresses to be specified in the ASIV
> + * block. The interface provies the capability to let the kernel
> + * driver know where those addresses are by specifying the ATS field,
> + * such that it can replace the user-space addresses with appropriate
> + * DMA addresses or DMA addresses of a scatter gather list which is
> + * dynamically created.
> + *
> + * Our hardware will refuse DDCB execution if the ATS field is not as
> + * expected. That means the DDCB execution engine in the chip knows
> + * where it expects DMA addresses within the ASIV part of the DDCB
> and
> + * will check that against the ATS field definition. Any invalid or
> + * unknown ATS content will lead to DDCB refusal.
> + */
> +
> +/* Genwqe chip Units */
> +#define DDCB_ACFUNC_SLU0x00 /* chip service
> layer unit */
> +#define DDCB_ACFUNC_APP0x01 /* chip
> application */
> +
> +/* DDCB return codes (RETC) */
> +#define DDCB_RETC_IDLE 0x /* Unexecuted/DDCB
> created */
> +#define DDCB_RETC_PENDING 0x0101 /* Pending Execution */
> +#define DDCB_RETC_COMPLETE 0x0102 /* Cmd complete. No
> error */
> +#define DDCB_RETC_FAULT0x0104 /* App Err,
> recoverable */
> +#define DDCB_RETC_ERROR0x0108 /* App Err,
> non-recoverable */
> +#define DDCB_RETC_FORCED_ERROR 0x01ff /* overwritten by
> driver */
> +
> +#define DDCB_RETC_UNEXEC 0x0110 /* Unexe/Removed from
> queue */
> +#define DDCB_RETC_TERM 0x0120 /* Terminated */
> +#define DDCB_RETC_RES0 0x0140 /* Reserved */
> +#define DDCB_RETC_RES1 0x0180 /* Reserved */
> +
> +/* DDCB Command Options (CMDOPT) */
> +#define DDCB_OPT_ECHO_FORCE_NO 0x /* ECHO DDCB */
> +#define DDCB_OPT_ECHO_FORCE_1020x0001 /* force return
> code */
> +#define DDCB_OPT_ECHO_FORCE_1040x0002
> +#define DDCB_OPT_ECHO_FORCE_1080x0003
> +
> +#define DDCB_OPT_ECHO_FORCE_1100x0004 /* only on PF !
> */
> +#define DDCB_OPT_ECHO_FORCE_1200x0005
> +#define DDCB_OPT_ECHO_FORCE_1400x0006
> +#define DDCB_OPT_ECHO_FORCE_1800x0007
> +
> +#define DDCB_OPT_ECHO_COPY_NONE(0 << 5)
> +#define DDCB_OPT_ECHO_COPY_ALL (1 << 5)
> +
> +/* Definitions of Service Layer Commands */
> +#define SLCMD_ECHO_SYNC0x00 /* PF/VF */
> +#define SLCMD_MOVE_FLASH 0x06 /* PF only */
> +#define SLCMD_MOVE_FLASH_FLAGS_MODE0x03 /* bit 0 and 1 used for
> mode */
> +#define SLCMD_MOVE_FLASH_FLAGS_DLOAD 0 /* mode: download */
> +#define SLCMD_MOVE_FLASH_FLAGS_EMUL1 /* mode: emulation */
> +#define SLCMD_MOVE_FLASH_FLAGS_UPLOAD 2 /* mode: upload*/
> +#define SLCMD_MOVE_FLASH_FLAGS_VERIFY 3 /* mode: verify*/
> +#define SLCMD_MOVE_FLASH_FLAG_NOTAP(1 << 2)/* just dump DDCB and
> exit */
> +#define SLCMD_MOVE_FLASH_FLAG_POLL (1 << 3)/* wait for RETC >=
> 0102 */
> +#define SLCMD_MOVE_FLASH_FLAG_PARTITION(1 << 4)
> +#define SLCMD_MOVE_FLASH_FLAG_ERASE(1 << 5)
> +
> +enum genwqe_card_state {
> + GENWQE_CARD_UNUSED = 0,
> + GENWQE_CARD_USED = 1,
> + GENWQE_CARD_FATAL_ERROR = 2,
> + GENWQE_CARD_STATE_MAX,
> +};
> +
> +/* common struct for chip image exchange */
> +struct genwqe_bitstream {
> + __u64 data_addr;/* pointer to image data */
> + __u32 size; /* size of image file */
> + __u32 crc; /* crc of this image */
> + __u8 partition;/* '0', '1',
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi,
Am Dienstag, den 03.12.2013, 15:28 +0100 schrieb Frank Haverkamp:
> + */
> +struct genwqe_mem {
> + unsigned long addr;
> + unsigned long size;
> + int direction;
> +};
> +
> +#define GENWQE_PIN_MEM _IOWR(GENWQE_IOC_CODE, 40, struct
> genwqe_mem *)
> +#define GENWQE_UNPIN_MEM _IOWR(GENWQE_IOC_CODE, 41, struct
> genwqe_mem *)
> +
Before someone comments on the unsigned long and the 32/64 bit issues
with it. I need to fix that.
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Arnd,
thanks for helping to review the code.
Am Dienstag, den 03.12.2013, 16:05 +0100 schrieb Arnd Bergmann:
> On Tuesday 03 December 2013, Frank Haverkamp wrote:
> > Ohh, sorry __u64 of course:
> >
> > /* common struct for chip image exchange */
> > struct genwqe_bitstream {
> > __u64 data_addr;/* pointer to image data */
> > __u32 size; /* size of image file */
> > __u32 crc; /* crc of this image */
> > __u8 partition;/* '0', '1', or 'v' */
> > __u64 target_addr; /* starting address in Flash */
> > __u8 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
> > */
> > };
> >
> > and than I do in my userspace application:
> >
> > load.data_addr = (unsigned long)buf;
> >
> > Is that ok, or must I consider more?
> >
>
> I haven't followed the recent discussions, jumping into the middle here:
> The structure above is not safe for a generic ioctl interface because it
> has different padding on x86-32 and x86-64, where __u64 has different
> alignment.
>
> You can try to avoid the implicit padding by sorting the members by size,
> by making some lignments for 32-bit and 64-bit. I avoid umembers larger or by
> adding explicit padding.
>
> Arnd
>
Ok, let me try to sort my entries a little differently and modify some
sizes, to avoid different alignments for 32-bit and 64-bit. I avoid
using __u8 now such that I always have nice 64-bit blocks. Would the
following version work?
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 */
};
If not, I might have missed something, and I would appreciate if you
could make up an example how a good version should look like.
Thanks
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Arnd,
thanks for helping to review the code.
Am Dienstag, den 03.12.2013, 16:05 +0100 schrieb Arnd Bergmann:
On Tuesday 03 December 2013, Frank Haverkamp wrote:
Ohh, sorry __u64 of course:
/* common struct for chip image exchange */
struct genwqe_bitstream {
__u64 data_addr;/* pointer to image data */
__u32 size; /* size of image file */
__u32 crc; /* crc of this image */
__u8 partition;/* '0', '1', or 'v' */
__u64 target_addr; /* starting address in Flash */
__u8 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
*/
};
and than I do in my userspace application:
load.data_addr = (unsigned long)buf;
Is that ok, or must I consider more?
I haven't followed the recent discussions, jumping into the middle here:
The structure above is not safe for a generic ioctl interface because it
has different padding on x86-32 and x86-64, where __u64 has different
alignment.
You can try to avoid the implicit padding by sorting the members by size,
by making some lignments for 32-bit and 64-bit. I avoid umembers larger or by
adding explicit padding.
Arnd
Ok, let me try to sort my entries a little differently and modify some
sizes, to avoid different alignments for 32-bit and 64-bit. I avoid
using __u8 now such that I always have nice 64-bit blocks. Would the
following version work?
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 */
};
If not, I might have missed something, and I would appreciate if you
could make up an example how a good version should look like.
Thanks
Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi,
Am Dienstag, den 03.12.2013, 15:28 +0100 schrieb Frank Haverkamp:
+ */
+struct genwqe_mem {
+ unsigned long addr;
+ unsigned long size;
+ int direction;
+};
+
+#define GENWQE_PIN_MEM _IOWR(GENWQE_IOC_CODE, 40, struct
genwqe_mem *)
+#define GENWQE_UNPIN_MEM _IOWR(GENWQE_IOC_CODE, 41, struct
genwqe_mem *)
+
Before someone comments on the unsigned long and the 32/64 bit issues
with it. I need to fix that.
Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
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;
};
};
+
+/*
+ * All registers of our card will return values not equal this
values.
+ * If we see IO_ILLEGAL_VALUE on any of our MMIO register reads, the
+ * card can be considered as unusable. It will need recovery.
+ */
+#define IO_ILLEGAL_VALUE 0xull
+
+/*
+ * Generic DDCB execution interface.
+ *
+ * This interface is a first prototype resulting from discussions we
+ * had with other teams which wanted to use the Genwqe card. It
allows
+ * to issue a DDCB request in a generic way. The request will block
+ * until it finishes or time out with error.
+ *
+ * Some DDCBs require DMA addresses to be specified in the ASIV
+ * block. The interface provies the capability to let the kernel
+ * driver know where those addresses are by specifying the ATS field,
+ * such that it can replace the user-space addresses with appropriate
+ * DMA addresses or DMA addresses of a scatter gather list which is
+ * dynamically created.
+ *
+ * Our hardware will refuse DDCB execution if the ATS field is not as
+ * expected. That means the DDCB execution engine in the chip knows
+ * where it expects DMA addresses within the ASIV part of the DDCB
and
+ * will check that against the ATS field definition. Any invalid or
+ * unknown ATS content will lead to DDCB refusal.
+ */
+
+/* Genwqe chip Units */
+#define DDCB_ACFUNC_SLU0x00 /* chip service
layer unit */
+#define DDCB_ACFUNC_APP0x01 /* chip
application */
+
+/* DDCB return codes (RETC) */
+#define DDCB_RETC_IDLE 0x /* Unexecuted/DDCB
created */
+#define DDCB_RETC_PENDING 0x0101 /* Pending Execution */
+#define DDCB_RETC_COMPLETE 0x0102 /* Cmd complete. No
error */
+#define DDCB_RETC_FAULT0x0104 /* App Err,
recoverable */
+#define DDCB_RETC_ERROR0x0108 /* App Err,
non-recoverable */
+#define DDCB_RETC_FORCED_ERROR 0x01ff /* overwritten by
driver */
+
+#define DDCB_RETC_UNEXEC 0x0110 /* Unexe/Removed from
queue */
+#define DDCB_RETC_TERM 0x0120 /* Terminated */
+#define DDCB_RETC_RES0 0x0140 /* Reserved */
+#define DDCB_RETC_RES1 0x0180 /* Reserved */
+
+/* DDCB Command Options (CMDOPT) */
+#define DDCB_OPT_ECHO_FORCE_NO 0x /* ECHO DDCB */
+#define DDCB_OPT_ECHO_FORCE_1020x0001 /* force return
code */
+#define DDCB_OPT_ECHO_FORCE_1040x0002
+#define DDCB_OPT_ECHO_FORCE_1080x0003
+
+#define DDCB_OPT_ECHO_FORCE_1100x0004 /* only on PF !
*/
+#define DDCB_OPT_ECHO_FORCE_1200x0005
+#define DDCB_OPT_ECHO_FORCE_1400x0006
+#define DDCB_OPT_ECHO_FORCE_1800x0007
+
+#define DDCB_OPT_ECHO_COPY_NONE(0 5)
+#define DDCB_OPT_ECHO_COPY_ALL (1 5)
+
+/* Definitions of Service Layer Commands */
+#define SLCMD_ECHO_SYNC0x00 /* PF/VF */
+#define SLCMD_MOVE_FLASH 0x06 /* PF only */
+#define SLCMD_MOVE_FLASH_FLAGS_MODE0x03 /* bit 0 and 1 used for
mode */
+#define SLCMD_MOVE_FLASH_FLAGS_DLOAD 0 /* mode: download */
+#define SLCMD_MOVE_FLASH_FLAGS_EMUL1 /* mode: emulation */
+#define SLCMD_MOVE_FLASH_FLAGS_UPLOAD 2 /* mode: upload*/
+#define SLCMD_MOVE_FLASH_FLAGS_VERIFY 3 /* mode: verify*/
+#define SLCMD_MOVE_FLASH_FLAG_NOTAP(1 2)/* just dump DDCB and
exit */
+#define SLCMD_MOVE_FLASH_FLAG_POLL (1 3)/* wait for RETC =
0102 */
+#define SLCMD_MOVE_FLASH_FLAG_PARTITION(1 4)
+#define SLCMD_MOVE_FLASH_FLAG_ERASE(1 5)
+
+enum genwqe_card_state {
+ GENWQE_CARD_UNUSED = 0,
+ GENWQE_CARD_USED = 1,
+ GENWQE_CARD_FATAL_ERROR = 2,
+ GENWQE_CARD_STATE_MAX,
+};
+
+/* common struct for chip image exchange */
+struct genwqe_bitstream {
+ __u64 data_addr;/* pointer to image data */
+ __u32 size; /* size of image file */
+ __u32 crc; /* crc of this image */
+ __u8 partition;/* '0', '1', or 'v' */
+ __u64 target_addr; /* starting address in Flash
*/
+ __u8 uid;
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Wednesday 04 December 2013, Frank Haverkamp wrote:
Am Dienstag, den 03.12.2013, 15:28 +0100 schrieb Frank Haverkamp:
+ */
+struct genwqe_mem {
+ unsigned long addr;
+ unsigned long size;
+ int direction;
+};
+
+#define GENWQE_PIN_MEM _IOWR(GENWQE_IOC_CODE, 40, struct
genwqe_mem *)
+#define GENWQE_UNPIN_MEM _IOWR(GENWQE_IOC_CODE, 41, struct
genwqe_mem *)
+
Before someone comments on the unsigned long and the 32/64 bit issues
with it. I need to fix that.
Also the extraneous '*' in the definitions.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
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
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Tuesday 03 December 2013, Frank Haverkamp wrote:
> Ohh, sorry __u64 of course:
>
> /* common struct for chip image exchange */
> struct genwqe_bitstream {
> __u64 data_addr;/* pointer to image data */
> __u32 size; /* size of image file */
> __u32 crc; /* crc of this image */
> __u8 partition;/* '0', '1', or 'v' */
> __u64 target_addr; /* starting address in Flash */
> __u8 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
> */
> };
>
> and than I do in my userspace application:
>
> load.data_addr = (unsigned long)buf;
>
> Is that ok, or must I consider more?
>
I haven't followed the recent discussions, jumping into the middle here:
The structure above is not safe for a generic ioctl interface because it
has different padding on x86-32 and x86-64, where __u64 has different
alignment.
You can try to avoid the implicit padding by sorting the members by size,
by making some members larger or by adding explicit padding.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Am Dienstag, den 03.12.2013, 06:30 -0800 schrieb Greg KH:
> On Tue, Dec 03, 2013 at 02:35:17PM +0100, Frank Haverkamp wrote:
> > > > +/* common struct for chip image exchange */
> > > > +struct chip_bitstream {
> > > > + uint8_t __user *pdata; /* pointer to image data */
> > > > + int size; /* size of image file*/
> > >
> > > I think this fails the 32/64bit issue, right?
> >
> > Yes. I replaced those by something like
> >__u32 data_addr;
> > I hope that is fixing the 32/64bit issue.
>
> No, not at all, how are you going to put a 64bit userspace pointer in
> there?
>
Ohh, sorry __u64 of course:
/* common struct for chip image exchange */
struct genwqe_bitstream {
__u64 data_addr;/* pointer to image data */
__u32 size; /* size of image file */
__u32 crc; /* crc of this image */
__u8 partition;/* '0', '1', or 'v' */
__u64 target_addr; /* starting address in Flash */
__u8 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
*/
};
and than I do in my userspace application:
load.data_addr = (unsigned long)buf;
Is that ok, or must I consider more?
Regards
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Tue, Dec 03, 2013 at 02:35:17PM +0100, Frank Haverkamp wrote:
> > > +/* common struct for chip image exchange */
> > > +struct chip_bitstream {
> > > + uint8_t __user *pdata; /* pointer to image data */
> > > + int size; /* size of image file*/
> >
> > I think this fails the 32/64bit issue, right?
>
> Yes. I replaced those by something like
>__u32 data_addr;
> I hope that is fixing the 32/64bit issue.
No, not at all, how are you going to put a 64bit userspace pointer in
there?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Tue, Dec 03, 2013 at 02:49:41PM +0100, Frank Haverkamp wrote:
> Hi Greg,
>
> Am Mittwoch, den 27.11.2013, 11:20 -0800 schrieb Greg KH:
> > On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote:
> > > +/*
> > > + * Flags for extended output (dbg_print)
> > > + * We define different levels of debugging for the appropriate unit.
> > > + */
> > > +#define dbg_card 0x0001
> > > +#define dbg_card_ddcb0x0004
> > > +#define dbg_card_regs0x0008
> > > +#define dbg_card_sglist 0x0400
> > > +#define dbg_card_pinning 0x0800
> > > +
> > > +#define genwqe_dprintk(_cd, dbg_unit, fmt, ...) do {
> > > \
> > > + struct genwqe_dev *__cd = (_cd);\
> > > + if ((_cd)->debug & (dbg_unit)) \
> > > + dev_info(&__cd->pci_dev->dev, fmt, \
> > > + ## __VA_ARGS__); \
> > > + } while (0)
> >
> > Ugh, really? How is a debugging printk being sent out with dev_info()?
> >
> > And why not just use dynamic kernel debugging and not create your own
> > masks and macros? We are trying to make everything use the same
> > infrastructure, please don't create new ones for every individual driver
> > in the kernel, that's a mess.
>
> I removed the genwqe_dprintk function completely. Our debug flag I kept
> for the moment, because I like to use the kernel hexdump functions to
> print my control blocks for debugging:
>
> if (cd->debug & dbg_card_ddcb) {
> dev_dbg(_dev->dev, "FINISHED DDCB#%d\n", req->num);
> genwqe_hexdump(pci_dev, pddcb, sizeof(*pddcb));
> }
>
> Or is there something allowing me to do those hexdumps with the
> dynamic_debug feature too? dynamic_hex_dump() looks like what I want.
Yes, that is what to use.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Greg,
Am Mittwoch, den 27.11.2013, 11:20 -0800 schrieb Greg KH:
> On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote:
> > +/*
> > + * Flags for extended output (dbg_print)
> > + * We define different levels of debugging for the appropriate unit.
> > + */
> > +#define dbg_card 0x0001
> > +#define dbg_card_ddcb 0x0004
> > +#define dbg_card_regs 0x0008
> > +#define dbg_card_sglist0x0400
> > +#define dbg_card_pinning 0x0800
> > +
> > +#define genwqe_dprintk(_cd, dbg_unit, fmt, ...) do {
> > \
> > + struct genwqe_dev *__cd = (_cd);\
> > + if ((_cd)->debug & (dbg_unit)) \
> > + dev_info(&__cd->pci_dev->dev, fmt, \
> > +## __VA_ARGS__); \
> > + } while (0)
>
> Ugh, really? How is a debugging printk being sent out with dev_info()?
>
> And why not just use dynamic kernel debugging and not create your own
> masks and macros? We are trying to make everything use the same
> infrastructure, please don't create new ones for every individual driver
> in the kernel, that's a mess.
I removed the genwqe_dprintk function completely. Our debug flag I kept
for the moment, because I like to use the kernel hexdump functions to
print my control blocks for debugging:
if (cd->debug & dbg_card_ddcb) {
dev_dbg(_dev->dev, "FINISHED DDCB#%d\n", req->num);
genwqe_hexdump(pci_dev, pddcb, sizeof(*pddcb));
}
Or is there something allowing me to do those hexdumps with the
dynamic_debug feature too? dynamic_hex_dump() looks like what I want.
So let me try to use that and get rid of my own debug support.
>
> thanks,
>
> greg k-h
>
Regards
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Greg, Am Mittwoch, den 27.11.2013, 11:16 -0800 schrieb Greg KH: > On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote: > > --- /dev/null > > +++ b/include/linux/genwqe/genwqe_card.h > > @@ -0,0 +1,547 @@ > > +#ifndef __GENWQE_CARD_H__ > > +#define __GENWQE_CARD_H__ > > + > > +/** > > + * IBM Accelerator Family 'GenWQE' > > + * > > + * (C) Copyright IBM Corp. 2013 > > + * > > + * Author: Frank Haverkamp > > + * Author: Joerg-Stephan Vogt > > + * Author: Michael Jung > > + * Author: Michael Ruettger > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License (version 2 only) > > + * as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +/* > > + * User-space API for the GenWQE card. For debugging and test purposes > > + * the register addresses are included here too. > > + */ > > + > > +#ifdef __KERNEL__ > > +# include > > +# include > > +#else > > +# include > > +# include > > +# include > > +# include > > +# > > +# ifndef __user > > +#define __user > > +# endif > > +#endif > > Ick, really? Yes, looks ugly. I got rid of that. > > Anyway, userspace .h files need to be in include/uapi/, not in > include/linux/, please split only the userspace side of this file out to > that directory, the other bits that do not need to go to userspace can > probably go back down in your local directory, right? Sure. I moved the file over, found some constants/struct names with sub-optimal prefixes, and fixed those too. > > > + > > +/* Basename of sysfs, debugfs and /dev interfaces */ > > +#define GENWQE_DEVNAME "genwqe" > > + > > +#define GENWQE_TYPE_ALTERA_230 0x00 /* GenWQE4 Stratix-IV-230 > > */ > > +#define GENWQE_TYPE_ALTERA_530 0x01 /* GenWQE4 Stratix-IV-530 > > */ > > +#define GENWQE_TYPE_ALTERA_A4 0x02 /* GenWQE5 A4 Stratix-V-A4 > > */ > > +#define GENWQE_TYPE_ALTERA_A7 0x03 /* GenWQE5 A7 Stratix-V-A7 > > */ > > + > > +/* MMIO Unit offsets: Each UnitID occupies a defined address range */ > > +#define UID_OFFS(uid) ((uid) << 24) > > + > > +#define SLU_OFFS UID_OFFS(0) > > +#define HSU_OFFS UID_OFFS(1) > > +#define APP_OFFS UID_OFFS(2) > > +#define MEMC0_OFFS UID_OFFS(3) > > +#define MEMC1_OFFS UID_OFFS(4) > > +#define ETH0_OFFS UID_OFFS(5) > > +#define ETH1_OFFS UID_OFFS(6) > > +#define GENWQE_MAX_UNITS 3 > > + > > +/* Common offsets per UnitID */ > > +#define IO_EXTENDED_ERROR_POINTER 0x0048 > > +#define IO_ERROR_INJECT_SELECTOR 0x0060 > > +#define IO_EXTENDED_DIAG_SELECTOR 0x0070 > > +#define IO_EXTENDED_DIAG_READ_MBX 0x0078 > > +#define IO_EXTENDED_DIAG_MAP(ring) (0x0500 | ((ring) << 3)) > > + > > +#define EXTENDED_DIAG_SELECTOR(ring, trace) (((ring) << 8) | (trace)) > > + > > +/* UnitID 0: Service Layer Unit (SLU) */ > > + > > +/* SLU: Unit Configuration Register */ > > +#define IO_SLU_UNITCFG 0x > > +#define SLU_UNITCFG_TYPE_MASK0x0ff0 /* 27:20 */ > > + > > +/* SLU: Fault Isolation Register (FIR) (ac_slu_fir) */ > > +#define IO_SLU_FIR 0x0008 /* read only, wr direct */ > > +#define IO_SLU_FIR_CLR 0x0010 /* read and clear */ > > + > > +/* SLU: First Error Capture Register (FEC/WOF) */ > > +#define IO_SLU_FEC 0x0018 > > + > > +#define IO_SLU_ERR_ACT_MASK0x0020 > > +#define IO_SLU_ERR_ATTN_MASK 0x0028 > > +#define IO_SLU_FIRX1_ACT_MASK 0x0030 > > +#define IO_SLU_FIRX0_ACT_MASK 0x0038 > > +#define IO_SLU_SEC_LEM_DEBUG_OVR 0x0040 > > +#define IO_SLU_EXTENDED_ERR_PTR0x0048 > > +#define IO_SLU_COMMON_CONFIG 0x0060 > > + > > +/* SLU: Flash FIR */ > > +#define IO_SLU_FLASH_FIR 0x0108 > > + > > +/* SLU: SLC FIR (This section needs to be updated for A5) */ > > +#define IO_SLU_SLC_FIR 0x0110 > > + > > +/* SLU: RIU Secondary Trap Register */ > > +#define IO_SLU_RIU_TRAP0x0280 > > + > > +/* SLU: Flash FEC */ > > +#define IO_SLU_FLASH_FEC 0x0308 > > + > > +/* SLU: SLC secondary FEC */ > > +#define IO_SLU_SLC_FEC 0x0310 > > + > > +#define W1CLR_OFFS 0x0040 > > +#define W1SET_OFFS 0x0080 > > + > > +/* > > + * The Virtual Function's Access is from offset 0x0001 > > + * The Physical Function's Access is from offset 0x0005 > > +
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Greg, Am Mittwoch, den 27.11.2013, 11:16 -0800 schrieb Greg KH: On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote: --- /dev/null +++ b/include/linux/genwqe/genwqe_card.h @@ -0,0 +1,547 @@ +#ifndef __GENWQE_CARD_H__ +#define __GENWQE_CARD_H__ + +/** + * IBM Accelerator Family 'GenWQE' + * + * (C) Copyright IBM Corp. 2013 + * + * Author: Frank Haverkamp ha...@linux.vnet.ibm.com + * Author: Joerg-Stephan Vogt jsv...@de.ibm.com + * Author: Michael Jung mij...@de.ibm.com + * Author: Michael Ruettger mich...@ibmra.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License (version 2 only) + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * User-space API for the GenWQE card. For debugging and test purposes + * the register addresses are included here too. + */ + +#ifdef __KERNEL__ +# include linux/types.h +# include linux/ioctl.h +#else +# include stdint.h +# include asm/ioctl.h +# include stddef.h +# include string.h +# +# ifndef __user +#define __user +# endif +#endif Ick, really? Yes, looks ugly. I got rid of that. Anyway, userspace .h files need to be in include/uapi/, not in include/linux/, please split only the userspace side of this file out to that directory, the other bits that do not need to go to userspace can probably go back down in your local directory, right? Sure. I moved the file over, found some constants/struct names with sub-optimal prefixes, and fixed those too. + +/* Basename of sysfs, debugfs and /dev interfaces */ +#define GENWQE_DEVNAME genwqe + +#define GENWQE_TYPE_ALTERA_230 0x00 /* GenWQE4 Stratix-IV-230 */ +#define GENWQE_TYPE_ALTERA_530 0x01 /* GenWQE4 Stratix-IV-530 */ +#define GENWQE_TYPE_ALTERA_A4 0x02 /* GenWQE5 A4 Stratix-V-A4 */ +#define GENWQE_TYPE_ALTERA_A7 0x03 /* GenWQE5 A7 Stratix-V-A7 */ + +/* MMIO Unit offsets: Each UnitID occupies a defined address range */ +#define UID_OFFS(uid) ((uid) 24) + +#define SLU_OFFS UID_OFFS(0) +#define HSU_OFFS UID_OFFS(1) +#define APP_OFFS UID_OFFS(2) +#define MEMC0_OFFS UID_OFFS(3) +#define MEMC1_OFFS UID_OFFS(4) +#define ETH0_OFFS UID_OFFS(5) +#define ETH1_OFFS UID_OFFS(6) +#define GENWQE_MAX_UNITS 3 + +/* Common offsets per UnitID */ +#define IO_EXTENDED_ERROR_POINTER 0x0048 +#define IO_ERROR_INJECT_SELECTOR 0x0060 +#define IO_EXTENDED_DIAG_SELECTOR 0x0070 +#define IO_EXTENDED_DIAG_READ_MBX 0x0078 +#define IO_EXTENDED_DIAG_MAP(ring) (0x0500 | ((ring) 3)) + +#define EXTENDED_DIAG_SELECTOR(ring, trace) (((ring) 8) | (trace)) + +/* UnitID 0: Service Layer Unit (SLU) */ + +/* SLU: Unit Configuration Register */ +#define IO_SLU_UNITCFG 0x +#define SLU_UNITCFG_TYPE_MASK0x0ff0 /* 27:20 */ + +/* SLU: Fault Isolation Register (FIR) (ac_slu_fir) */ +#define IO_SLU_FIR 0x0008 /* read only, wr direct */ +#define IO_SLU_FIR_CLR 0x0010 /* read and clear */ + +/* SLU: First Error Capture Register (FEC/WOF) */ +#define IO_SLU_FEC 0x0018 + +#define IO_SLU_ERR_ACT_MASK0x0020 +#define IO_SLU_ERR_ATTN_MASK 0x0028 +#define IO_SLU_FIRX1_ACT_MASK 0x0030 +#define IO_SLU_FIRX0_ACT_MASK 0x0038 +#define IO_SLU_SEC_LEM_DEBUG_OVR 0x0040 +#define IO_SLU_EXTENDED_ERR_PTR0x0048 +#define IO_SLU_COMMON_CONFIG 0x0060 + +/* SLU: Flash FIR */ +#define IO_SLU_FLASH_FIR 0x0108 + +/* SLU: SLC FIR (This section needs to be updated for A5) */ +#define IO_SLU_SLC_FIR 0x0110 + +/* SLU: RIU Secondary Trap Register */ +#define IO_SLU_RIU_TRAP0x0280 + +/* SLU: Flash FEC */ +#define IO_SLU_FLASH_FEC 0x0308 + +/* SLU: SLC secondary FEC */ +#define IO_SLU_SLC_FEC 0x0310 + +#define W1CLR_OFFS 0x0040 +#define W1SET_OFFS 0x0080 + +/* + * The Virtual Function's Access is from offset 0x0001 + * The Physical Function's Access is from offset 0x0005 + * Single Shared Registers exists only at offset 0x0006 + */ + +/* + * SLC: Queue Virtual Window Window for
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Greg,
Am Mittwoch, den 27.11.2013, 11:20 -0800 schrieb Greg KH:
On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote:
+/*
+ * Flags for extended output (dbg_print)
+ * We define different levels of debugging for the appropriate unit.
+ */
+#define dbg_card 0x0001
+#define dbg_card_ddcb 0x0004
+#define dbg_card_regs 0x0008
+#define dbg_card_sglist0x0400
+#define dbg_card_pinning 0x0800
+
+#define genwqe_dprintk(_cd, dbg_unit, fmt, ...) do {
\
+ struct genwqe_dev *__cd = (_cd);\
+ if ((_cd)-debug (dbg_unit)) \
+ dev_info(__cd-pci_dev-dev, fmt, \
+## __VA_ARGS__); \
+ } while (0)
Ugh, really? How is a debugging printk being sent out with dev_info()?
And why not just use dynamic kernel debugging and not create your own
masks and macros? We are trying to make everything use the same
infrastructure, please don't create new ones for every individual driver
in the kernel, that's a mess.
I removed the genwqe_dprintk function completely. Our debug flag I kept
for the moment, because I like to use the kernel hexdump functions to
print my control blocks for debugging:
if (cd-debug dbg_card_ddcb) {
dev_dbg(pci_dev-dev, FINISHED DDCB#%d\n, req-num);
genwqe_hexdump(pci_dev, pddcb, sizeof(*pddcb));
}
Or is there something allowing me to do those hexdumps with the
dynamic_debug feature too? dynamic_hex_dump() looks like what I want.
So let me try to use that and get rid of my own debug support.
thanks,
greg k-h
Regards
Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Tue, Dec 03, 2013 at 02:49:41PM +0100, Frank Haverkamp wrote:
Hi Greg,
Am Mittwoch, den 27.11.2013, 11:20 -0800 schrieb Greg KH:
On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote:
+/*
+ * Flags for extended output (dbg_print)
+ * We define different levels of debugging for the appropriate unit.
+ */
+#define dbg_card 0x0001
+#define dbg_card_ddcb0x0004
+#define dbg_card_regs0x0008
+#define dbg_card_sglist 0x0400
+#define dbg_card_pinning 0x0800
+
+#define genwqe_dprintk(_cd, dbg_unit, fmt, ...) do {
\
+ struct genwqe_dev *__cd = (_cd);\
+ if ((_cd)-debug (dbg_unit)) \
+ dev_info(__cd-pci_dev-dev, fmt, \
+ ## __VA_ARGS__); \
+ } while (0)
Ugh, really? How is a debugging printk being sent out with dev_info()?
And why not just use dynamic kernel debugging and not create your own
masks and macros? We are trying to make everything use the same
infrastructure, please don't create new ones for every individual driver
in the kernel, that's a mess.
I removed the genwqe_dprintk function completely. Our debug flag I kept
for the moment, because I like to use the kernel hexdump functions to
print my control blocks for debugging:
if (cd-debug dbg_card_ddcb) {
dev_dbg(pci_dev-dev, FINISHED DDCB#%d\n, req-num);
genwqe_hexdump(pci_dev, pddcb, sizeof(*pddcb));
}
Or is there something allowing me to do those hexdumps with the
dynamic_debug feature too? dynamic_hex_dump() looks like what I want.
Yes, that is what to use.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Tue, Dec 03, 2013 at 02:35:17PM +0100, Frank Haverkamp wrote:
+/* common struct for chip image exchange */
+struct chip_bitstream {
+ uint8_t __user *pdata; /* pointer to image data */
+ int size; /* size of image file*/
I think this fails the 32/64bit issue, right?
Yes. I replaced those by something like
__u32 data_addr;
I hope that is fixing the 32/64bit issue.
No, not at all, how are you going to put a 64bit userspace pointer in
there?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Am Dienstag, den 03.12.2013, 06:30 -0800 schrieb Greg KH:
On Tue, Dec 03, 2013 at 02:35:17PM +0100, Frank Haverkamp wrote:
+/* common struct for chip image exchange */
+struct chip_bitstream {
+ uint8_t __user *pdata; /* pointer to image data */
+ int size; /* size of image file*/
I think this fails the 32/64bit issue, right?
Yes. I replaced those by something like
__u32 data_addr;
I hope that is fixing the 32/64bit issue.
No, not at all, how are you going to put a 64bit userspace pointer in
there?
Ohh, sorry __u64 of course:
/* common struct for chip image exchange */
struct genwqe_bitstream {
__u64 data_addr;/* pointer to image data */
__u32 size; /* size of image file */
__u32 crc; /* crc of this image */
__u8 partition;/* '0', '1', or 'v' */
__u64 target_addr; /* starting address in Flash */
__u8 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
*/
};
and than I do in my userspace application:
load.data_addr = (unsigned long)buf;
Is that ok, or must I consider more?
Regards
Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Tuesday 03 December 2013, Frank Haverkamp wrote:
Ohh, sorry __u64 of course:
/* common struct for chip image exchange */
struct genwqe_bitstream {
__u64 data_addr;/* pointer to image data */
__u32 size; /* size of image file */
__u32 crc; /* crc of this image */
__u8 partition;/* '0', '1', or 'v' */
__u64 target_addr; /* starting address in Flash */
__u8 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
*/
};
and than I do in my userspace application:
load.data_addr = (unsigned long)buf;
Is that ok, or must I consider more?
I haven't followed the recent discussions, jumping into the middle here:
The structure above is not safe for a generic ioctl interface because it
has different padding on x86-32 and x86-64, where __u64 has different
alignment.
You can try to avoid the implicit padding by sorting the members by size,
by making some members larger or by adding explicit padding.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote:
> +/*
> + * Flags for extended output (dbg_print)
> + * We define different levels of debugging for the appropriate unit.
> + */
> +#define dbg_card 0x0001
> +#define dbg_card_ddcb0x0004
> +#define dbg_card_regs0x0008
> +#define dbg_card_sglist 0x0400
> +#define dbg_card_pinning 0x0800
> +
> +#define genwqe_dprintk(_cd, dbg_unit, fmt, ...) do { \
> + struct genwqe_dev *__cd = (_cd);\
> + if ((_cd)->debug & (dbg_unit)) \
> + dev_info(&__cd->pci_dev->dev, fmt, \
> + ## __VA_ARGS__); \
> + } while (0)
Ugh, really? How is a debugging printk being sent out with dev_info()?
And why not just use dynamic kernel debugging and not create your own
masks and macros? We are trying to make everything use the same
infrastructure, please don't create new ones for every individual driver
in the kernel, that's a mess.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote: > --- /dev/null > +++ b/include/linux/genwqe/genwqe_card.h > @@ -0,0 +1,547 @@ > +#ifndef __GENWQE_CARD_H__ > +#define __GENWQE_CARD_H__ > + > +/** > + * IBM Accelerator Family 'GenWQE' > + * > + * (C) Copyright IBM Corp. 2013 > + * > + * Author: Frank Haverkamp > + * Author: Joerg-Stephan Vogt > + * Author: Michael Jung > + * Author: Michael Ruettger > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License (version 2 only) > + * as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +/* > + * User-space API for the GenWQE card. For debugging and test purposes > + * the register addresses are included here too. > + */ > + > +#ifdef __KERNEL__ > +# include > +# include > +#else > +# include > +# include > +# include > +# include > +# > +# ifndef __user > +#define __user > +# endif > +#endif Ick, really? Anyway, userspace .h files need to be in include/uapi/, not in include/linux/, please split only the userspace side of this file out to that directory, the other bits that do not need to go to userspace can probably go back down in your local directory, right? > + > +/* Basename of sysfs, debugfs and /dev interfaces */ > +#define GENWQE_DEVNAME "genwqe" > + > +#define GENWQE_TYPE_ALTERA_230 0x00 /* GenWQE4 Stratix-IV-230 > */ > +#define GENWQE_TYPE_ALTERA_530 0x01 /* GenWQE4 Stratix-IV-530 > */ > +#define GENWQE_TYPE_ALTERA_A40x02 /* GenWQE5 A4 Stratix-V-A4 > */ > +#define GENWQE_TYPE_ALTERA_A70x03 /* GenWQE5 A7 Stratix-V-A7 > */ > + > +/* MMIO Unit offsets: Each UnitID occupies a defined address range */ > +#define UID_OFFS(uid)((uid) << 24) > + > +#define SLU_OFFS UID_OFFS(0) > +#define HSU_OFFS UID_OFFS(1) > +#define APP_OFFS UID_OFFS(2) > +#define MEMC0_OFFS UID_OFFS(3) > +#define MEMC1_OFFS UID_OFFS(4) > +#define ETH0_OFFSUID_OFFS(5) > +#define ETH1_OFFSUID_OFFS(6) > +#define GENWQE_MAX_UNITS 3 > + > +/* Common offsets per UnitID */ > +#define IO_EXTENDED_ERROR_POINTER0x0048 > +#define IO_ERROR_INJECT_SELECTOR 0x0060 > +#define IO_EXTENDED_DIAG_SELECTOR0x0070 > +#define IO_EXTENDED_DIAG_READ_MBX0x0078 > +#define IO_EXTENDED_DIAG_MAP(ring) (0x0500 | ((ring) << 3)) > + > +#define EXTENDED_DIAG_SELECTOR(ring, trace) (((ring) << 8) | (trace)) > + > +/* UnitID 0: Service Layer Unit (SLU) */ > + > +/* SLU: Unit Configuration Register */ > +#define IO_SLU_UNITCFG 0x > +#define SLU_UNITCFG_TYPE_MASK 0x0ff0 /* 27:20 */ > + > +/* SLU: Fault Isolation Register (FIR) (ac_slu_fir) */ > +#define IO_SLU_FIR 0x0008 /* read only, wr direct */ > +#define IO_SLU_FIR_CLR 0x0010 /* read and clear */ > + > +/* SLU: First Error Capture Register (FEC/WOF) */ > +#define IO_SLU_FEC 0x0018 > + > +#define IO_SLU_ERR_ACT_MASK 0x0020 > +#define IO_SLU_ERR_ATTN_MASK 0x0028 > +#define IO_SLU_FIRX1_ACT_MASK0x0030 > +#define IO_SLU_FIRX0_ACT_MASK0x0038 > +#define IO_SLU_SEC_LEM_DEBUG_OVR 0x0040 > +#define IO_SLU_EXTENDED_ERR_PTR 0x0048 > +#define IO_SLU_COMMON_CONFIG 0x0060 > + > +/* SLU: Flash FIR */ > +#define IO_SLU_FLASH_FIR 0x0108 > + > +/* SLU: SLC FIR (This section needs to be updated for A5) */ > +#define IO_SLU_SLC_FIR 0x0110 > + > +/* SLU: RIU Secondary Trap Register */ > +#define IO_SLU_RIU_TRAP 0x0280 > + > +/* SLU: Flash FEC */ > +#define IO_SLU_FLASH_FEC 0x0308 > + > +/* SLU: SLC secondary FEC */ > +#define IO_SLU_SLC_FEC 0x0310 > + > +#define W1CLR_OFFS 0x0040 > +#define W1SET_OFFS 0x0080 > + > +/* > + * The Virtual Function's Access is from offset 0x0001 > + * The Physical Function's Access is from offset 0x0005 > + * Single Shared Registers exists only at offset 0x0006 > + */ > + > +/* > + * SLC: Queue Virtual Window Window for accessing into a specific VF > + * queue. When accessing the 0x1 space using the 0x5 address > + * segment, the value indicated here is used to specify which VF > + * register is decoded. This register, and the 0x5 register space > + * can only be accessed by the PF. Example, if this register is
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote: --- /dev/null +++ b/include/linux/genwqe/genwqe_card.h @@ -0,0 +1,547 @@ +#ifndef __GENWQE_CARD_H__ +#define __GENWQE_CARD_H__ + +/** + * IBM Accelerator Family 'GenWQE' + * + * (C) Copyright IBM Corp. 2013 + * + * Author: Frank Haverkamp ha...@linux.vnet.ibm.com + * Author: Joerg-Stephan Vogt jsv...@de.ibm.com + * Author: Michael Jung mij...@de.ibm.com + * Author: Michael Ruettger mich...@ibmra.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License (version 2 only) + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * User-space API for the GenWQE card. For debugging and test purposes + * the register addresses are included here too. + */ + +#ifdef __KERNEL__ +# include linux/types.h +# include linux/ioctl.h +#else +# include stdint.h +# include asm/ioctl.h +# include stddef.h +# include string.h +# +# ifndef __user +#define __user +# endif +#endif Ick, really? Anyway, userspace .h files need to be in include/uapi/, not in include/linux/, please split only the userspace side of this file out to that directory, the other bits that do not need to go to userspace can probably go back down in your local directory, right? + +/* Basename of sysfs, debugfs and /dev interfaces */ +#define GENWQE_DEVNAME genwqe + +#define GENWQE_TYPE_ALTERA_230 0x00 /* GenWQE4 Stratix-IV-230 */ +#define GENWQE_TYPE_ALTERA_530 0x01 /* GenWQE4 Stratix-IV-530 */ +#define GENWQE_TYPE_ALTERA_A40x02 /* GenWQE5 A4 Stratix-V-A4 */ +#define GENWQE_TYPE_ALTERA_A70x03 /* GenWQE5 A7 Stratix-V-A7 */ + +/* MMIO Unit offsets: Each UnitID occupies a defined address range */ +#define UID_OFFS(uid)((uid) 24) + +#define SLU_OFFS UID_OFFS(0) +#define HSU_OFFS UID_OFFS(1) +#define APP_OFFS UID_OFFS(2) +#define MEMC0_OFFS UID_OFFS(3) +#define MEMC1_OFFS UID_OFFS(4) +#define ETH0_OFFSUID_OFFS(5) +#define ETH1_OFFSUID_OFFS(6) +#define GENWQE_MAX_UNITS 3 + +/* Common offsets per UnitID */ +#define IO_EXTENDED_ERROR_POINTER0x0048 +#define IO_ERROR_INJECT_SELECTOR 0x0060 +#define IO_EXTENDED_DIAG_SELECTOR0x0070 +#define IO_EXTENDED_DIAG_READ_MBX0x0078 +#define IO_EXTENDED_DIAG_MAP(ring) (0x0500 | ((ring) 3)) + +#define EXTENDED_DIAG_SELECTOR(ring, trace) (((ring) 8) | (trace)) + +/* UnitID 0: Service Layer Unit (SLU) */ + +/* SLU: Unit Configuration Register */ +#define IO_SLU_UNITCFG 0x +#define SLU_UNITCFG_TYPE_MASK 0x0ff0 /* 27:20 */ + +/* SLU: Fault Isolation Register (FIR) (ac_slu_fir) */ +#define IO_SLU_FIR 0x0008 /* read only, wr direct */ +#define IO_SLU_FIR_CLR 0x0010 /* read and clear */ + +/* SLU: First Error Capture Register (FEC/WOF) */ +#define IO_SLU_FEC 0x0018 + +#define IO_SLU_ERR_ACT_MASK 0x0020 +#define IO_SLU_ERR_ATTN_MASK 0x0028 +#define IO_SLU_FIRX1_ACT_MASK0x0030 +#define IO_SLU_FIRX0_ACT_MASK0x0038 +#define IO_SLU_SEC_LEM_DEBUG_OVR 0x0040 +#define IO_SLU_EXTENDED_ERR_PTR 0x0048 +#define IO_SLU_COMMON_CONFIG 0x0060 + +/* SLU: Flash FIR */ +#define IO_SLU_FLASH_FIR 0x0108 + +/* SLU: SLC FIR (This section needs to be updated for A5) */ +#define IO_SLU_SLC_FIR 0x0110 + +/* SLU: RIU Secondary Trap Register */ +#define IO_SLU_RIU_TRAP 0x0280 + +/* SLU: Flash FEC */ +#define IO_SLU_FLASH_FEC 0x0308 + +/* SLU: SLC secondary FEC */ +#define IO_SLU_SLC_FEC 0x0310 + +#define W1CLR_OFFS 0x0040 +#define W1SET_OFFS 0x0080 + +/* + * The Virtual Function's Access is from offset 0x0001 + * The Physical Function's Access is from offset 0x0005 + * Single Shared Registers exists only at offset 0x0006 + */ + +/* + * SLC: Queue Virtual Window Window for accessing into a specific VF + * queue. When accessing the 0x1 space using the 0x5 address + * segment, the value indicated here is used to specify which VF + * register is decoded. This register, and the 0x5 register space + * can only be accessed by the PF. Example, if this register is
Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
On Wed, Nov 06, 2013 at 01:45:38PM +0100, Frank Haverkamp wrote:
+/*
+ * Flags for extended output (dbg_print)
+ * We define different levels of debugging for the appropriate unit.
+ */
+#define dbg_card 0x0001
+#define dbg_card_ddcb0x0004
+#define dbg_card_regs0x0008
+#define dbg_card_sglist 0x0400
+#define dbg_card_pinning 0x0800
+
+#define genwqe_dprintk(_cd, dbg_unit, fmt, ...) do { \
+ struct genwqe_dev *__cd = (_cd);\
+ if ((_cd)-debug (dbg_unit)) \
+ dev_info(__cd-pci_dev-dev, fmt, \
+ ## __VA_ARGS__); \
+ } while (0)
Ugh, really? How is a debugging printk being sent out with dev_info()?
And why not just use dynamic kernel debugging and not create your own
masks and macros? We are trying to make everything use the same
infrastructure, please don't create new ones for every individual driver
in the kernel, that's a mess.
thanks,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
