* [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data
@ 2018-04-25 21:30 Arnd Bergmann
2018-05-03 12:56 ` Sakari Ailus
2018-05-07 13:17 ` Laurent Pinchart
0 siblings, 2 replies; 9+ messages in thread
From: Arnd Bergmann @ 2018-04-25 21:30 UTC (permalink / raw)
To: Laurent Pinchart, Mauro Carvalho Chehab
Cc: Arnd Bergmann, Sakari Ailus, linux-media, linux-kernel
C libraries with 64-bit time_t use an incompatible format for
struct omap3isp_stat_data. This changes the kernel code to
support either version, by moving over the normal handling
to the 64-bit variant, and adding compatiblity code to handle
the old binary format with the existing ioctl command code.
Fortunately, the command code includes the size of the structure,
so the difference gets handled automatically. In the process of
eliminating the references to 'struct timeval' from the kernel,
I also change the way the timestamp is generated internally,
basically by open-coding the v4l2_get_timestamp() call.
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Sakari Ailus <sakari.ailus@iki.fi>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I submitted this one in November and asked again in January,
still waiting for a review so it can be applied.
---
drivers/media/platform/omap3isp/isph3a_aewb.c | 2 ++
drivers/media/platform/omap3isp/isph3a_af.c | 2 ++
drivers/media/platform/omap3isp/isphist.c | 2 ++
drivers/media/platform/omap3isp/ispstat.c | 21 +++++++++++++++++++--
drivers/media/platform/omap3isp/ispstat.h | 4 +++-
include/uapi/linux/omap3isp.h | 22 ++++++++++++++++++++++
6 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
index d44626f20ac6..3c82dea4d375 100644
--- a/drivers/media/platform/omap3isp/isph3a_aewb.c
+++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
@@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
return omap3isp_stat_config(stat, arg);
case VIDIOC_OMAP3ISP_STAT_REQ:
return omap3isp_stat_request_statistics(stat, arg);
+ case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
+ return omap3isp_stat_request_statistics_time32(stat, arg);
case VIDIOC_OMAP3ISP_STAT_EN: {
unsigned long *en = arg;
return omap3isp_stat_enable(stat, !!*en);
diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
index 99bd6cc21d86..4da25c84f0c6 100644
--- a/drivers/media/platform/omap3isp/isph3a_af.c
+++ b/drivers/media/platform/omap3isp/isph3a_af.c
@@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
return omap3isp_stat_config(stat, arg);
case VIDIOC_OMAP3ISP_STAT_REQ:
return omap3isp_stat_request_statistics(stat, arg);
+ case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
+ return omap3isp_stat_request_statistics_time32(stat, arg);
case VIDIOC_OMAP3ISP_STAT_EN: {
int *en = arg;
return omap3isp_stat_enable(stat, !!*en);
diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c
index a4ed5d140d48..d4be3d0e06f9 100644
--- a/drivers/media/platform/omap3isp/isphist.c
+++ b/drivers/media/platform/omap3isp/isphist.c
@@ -435,6 +435,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
return omap3isp_stat_config(stat, arg);
case VIDIOC_OMAP3ISP_STAT_REQ:
return omap3isp_stat_request_statistics(stat, arg);
+ case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
+ return omap3isp_stat_request_statistics_time32(stat, arg);
case VIDIOC_OMAP3ISP_STAT_EN: {
int *en = arg;
return omap3isp_stat_enable(stat, !!*en);
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 47cbc7e3d825..5967dfd0a9f7 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -18,6 +18,7 @@
#include <linux/dma-mapping.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
+#include <linux/timekeeping.h>
#include "isp.h"
@@ -237,7 +238,7 @@ static int isp_stat_buf_queue(struct ispstat *stat)
if (!stat->active_buf)
return STAT_NO_BUF;
- v4l2_get_timestamp(&stat->active_buf->ts);
+ ktime_get_ts64(&stat->active_buf->ts);
stat->active_buf->buf_size = stat->buf_size;
if (isp_stat_buf_check_magic(stat, stat->active_buf)) {
@@ -500,7 +501,8 @@ int omap3isp_stat_request_statistics(struct ispstat *stat,
return PTR_ERR(buf);
}
- data->ts = buf->ts;
+ data->ts.tv_sec = buf->ts.tv_sec;
+ data->ts.tv_usec = buf->ts.tv_nsec / NSEC_PER_USEC;
data->config_counter = buf->config_counter;
data->frame_number = buf->frame_number;
data->buf_size = buf->buf_size;
@@ -512,6 +514,21 @@ int omap3isp_stat_request_statistics(struct ispstat *stat,
return 0;
}
+int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
+ struct omap3isp_stat_data_time32 *data)
+{
+ struct omap3isp_stat_data data64;
+ int ret;
+
+ ret = omap3isp_stat_request_statistics(stat, &data64);
+
+ data->ts.tv_sec = data64.ts.tv_sec;
+ data->ts.tv_usec = data64.ts.tv_usec;
+ memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
+
+ return ret;
+}
+
/*
* omap3isp_stat_config - Receives new statistic engine configuration.
* @new_conf: Pointer to config structure.
diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h
index 6d9b0244f320..923b38cfc682 100644
--- a/drivers/media/platform/omap3isp/ispstat.h
+++ b/drivers/media/platform/omap3isp/ispstat.h
@@ -39,7 +39,7 @@ struct ispstat_buffer {
struct sg_table sgt;
void *virt_addr;
dma_addr_t dma_addr;
- struct timeval ts;
+ struct timespec64 ts;
u32 buf_size;
u32 frame_number;
u16 config_counter;
@@ -130,6 +130,8 @@ struct ispstat_generic_config {
int omap3isp_stat_config(struct ispstat *stat, void *new_conf);
int omap3isp_stat_request_statistics(struct ispstat *stat,
struct omap3isp_stat_data *data);
+int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
+ struct omap3isp_stat_data_time32 *data);
int omap3isp_stat_init(struct ispstat *stat, const char *name,
const struct v4l2_subdev_ops *sd_ops);
void omap3isp_stat_cleanup(struct ispstat *stat);
diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h
index 1a920145db04..87b55755f4ff 100644
--- a/include/uapi/linux/omap3isp.h
+++ b/include/uapi/linux/omap3isp.h
@@ -55,6 +55,8 @@
_IOWR('V', BASE_VIDIOC_PRIVATE + 5, struct omap3isp_h3a_af_config)
#define VIDIOC_OMAP3ISP_STAT_REQ \
_IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data)
+#define VIDIOC_OMAP3ISP_STAT_REQ_TIME32 \
+ _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data_time32)
#define VIDIOC_OMAP3ISP_STAT_EN \
_IOWR('V', BASE_VIDIOC_PRIVATE + 7, unsigned long)
@@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
* @config_counter: Number of the configuration associated with the data.
*/
struct omap3isp_stat_data {
+#ifdef __KERNEL__
+ struct {
+ __s64 tv_sec;
+ __s64 tv_usec;
+ } ts;
+#else
struct timeval ts;
+#endif
void __user *buf;
__u32 buf_size;
__u16 frame_number;
@@ -173,6 +182,19 @@ struct omap3isp_stat_data {
__u16 config_counter;
};
+#ifdef __KERNEL__
+struct omap3isp_stat_data_time32 {
+ struct {
+ __s32 tv_sec;
+ __s32 tv_usec;
+ } ts;
+ __u32 buf;
+ __u32 buf_size;
+ __u16 frame_number;
+ __u16 cur_frame;
+ __u16 config_counter;
+};
+#endif
/* Histogram related structs */
--
2.9.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2018-04-25 21:30 [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data Arnd Bergmann
@ 2018-05-03 12:56 ` Sakari Ailus
2018-05-03 22:06 ` Arnd Bergmann
2018-05-07 13:17 ` Laurent Pinchart
1 sibling, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2018-05-03 12:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel
Hi Arnd,
Thanks for the patch and apologies for the delays.
On Wed, Apr 25, 2018 at 11:30:10PM +0200, Arnd Bergmann wrote:
> C libraries with 64-bit time_t use an incompatible format for
> struct omap3isp_stat_data. This changes the kernel code to
> support either version, by moving over the normal handling
> to the 64-bit variant, and adding compatiblity code to handle
> the old binary format with the existing ioctl command code.
>
> Fortunately, the command code includes the size of the structure,
> so the difference gets handled automatically. In the process of
> eliminating the references to 'struct timeval' from the kernel,
> I also change the way the timestamp is generated internally,
> basically by open-coding the v4l2_get_timestamp() call.
>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I submitted this one in November and asked again in January,
> still waiting for a review so it can be applied.
> ---
> drivers/media/platform/omap3isp/isph3a_aewb.c | 2 ++
> drivers/media/platform/omap3isp/isph3a_af.c | 2 ++
> drivers/media/platform/omap3isp/isphist.c | 2 ++
> drivers/media/platform/omap3isp/ispstat.c | 21 +++++++++++++++++++--
> drivers/media/platform/omap3isp/ispstat.h | 4 +++-
> include/uapi/linux/omap3isp.h | 22 ++++++++++++++++++++++
> 6 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
> index d44626f20ac6..3c82dea4d375 100644
> --- a/drivers/media/platform/omap3isp/isph3a_aewb.c
> +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
> @@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
> return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> unsigned long *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
> index 99bd6cc21d86..4da25c84f0c6 100644
> --- a/drivers/media/platform/omap3isp/isph3a_af.c
> +++ b/drivers/media/platform/omap3isp/isph3a_af.c
> @@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
> return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> int *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c
> index a4ed5d140d48..d4be3d0e06f9 100644
> --- a/drivers/media/platform/omap3isp/isphist.c
> +++ b/drivers/media/platform/omap3isp/isphist.c
> @@ -435,6 +435,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
> return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> int *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
> index 47cbc7e3d825..5967dfd0a9f7 100644
> --- a/drivers/media/platform/omap3isp/ispstat.c
> +++ b/drivers/media/platform/omap3isp/ispstat.c
> @@ -18,6 +18,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <linux/timekeeping.h>
>
> #include "isp.h"
>
> @@ -237,7 +238,7 @@ static int isp_stat_buf_queue(struct ispstat *stat)
> if (!stat->active_buf)
> return STAT_NO_BUF;
>
> - v4l2_get_timestamp(&stat->active_buf->ts);
> + ktime_get_ts64(&stat->active_buf->ts);
>
> stat->active_buf->buf_size = stat->buf_size;
> if (isp_stat_buf_check_magic(stat, stat->active_buf)) {
> @@ -500,7 +501,8 @@ int omap3isp_stat_request_statistics(struct ispstat *stat,
> return PTR_ERR(buf);
> }
>
> - data->ts = buf->ts;
> + data->ts.tv_sec = buf->ts.tv_sec;
> + data->ts.tv_usec = buf->ts.tv_nsec / NSEC_PER_USEC;
> data->config_counter = buf->config_counter;
> data->frame_number = buf->frame_number;
> data->buf_size = buf->buf_size;
> @@ -512,6 +514,21 @@ int omap3isp_stat_request_statistics(struct ispstat *stat,
> return 0;
> }
>
> +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> + struct omap3isp_stat_data_time32 *data)
> +{
> + struct omap3isp_stat_data data64;
> + int ret;
> +
> + ret = omap3isp_stat_request_statistics(stat, &data64);
> +
> + data->ts.tv_sec = data64.ts.tv_sec;
> + data->ts.tv_usec = data64.ts.tv_usec;
> + memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
> +
> + return ret;
> +}
> +
> /*
> * omap3isp_stat_config - Receives new statistic engine configuration.
> * @new_conf: Pointer to config structure.
> diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h
> index 6d9b0244f320..923b38cfc682 100644
> --- a/drivers/media/platform/omap3isp/ispstat.h
> +++ b/drivers/media/platform/omap3isp/ispstat.h
> @@ -39,7 +39,7 @@ struct ispstat_buffer {
> struct sg_table sgt;
> void *virt_addr;
> dma_addr_t dma_addr;
> - struct timeval ts;
> + struct timespec64 ts;
> u32 buf_size;
> u32 frame_number;
> u16 config_counter;
> @@ -130,6 +130,8 @@ struct ispstat_generic_config {
> int omap3isp_stat_config(struct ispstat *stat, void *new_conf);
> int omap3isp_stat_request_statistics(struct ispstat *stat,
> struct omap3isp_stat_data *data);
> +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> + struct omap3isp_stat_data_time32 *data);
> int omap3isp_stat_init(struct ispstat *stat, const char *name,
> const struct v4l2_subdev_ops *sd_ops);
> void omap3isp_stat_cleanup(struct ispstat *stat);
> diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h
> index 1a920145db04..87b55755f4ff 100644
> --- a/include/uapi/linux/omap3isp.h
> +++ b/include/uapi/linux/omap3isp.h
> @@ -55,6 +55,8 @@
> _IOWR('V', BASE_VIDIOC_PRIVATE + 5, struct omap3isp_h3a_af_config)
> #define VIDIOC_OMAP3ISP_STAT_REQ \
> _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data)
> +#define VIDIOC_OMAP3ISP_STAT_REQ_TIME32 \
> + _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data_time32)
> #define VIDIOC_OMAP3ISP_STAT_EN \
> _IOWR('V', BASE_VIDIOC_PRIVATE + 7, unsigned long)
>
> @@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
> * @config_counter: Number of the configuration associated with the data.
> */
> struct omap3isp_stat_data {
> +#ifdef __KERNEL__
> + struct {
> + __s64 tv_sec;
> + __s64 tv_usec;
Any particular reason for __s64 here instead of e.g. long or __s32? Kernel
appears to use long in the timespec64 definition.
> + } ts;
> +#else
> struct timeval ts;
> +#endif
> void __user *buf;
> __u32 buf_size;
> __u16 frame_number;
> @@ -173,6 +182,19 @@ struct omap3isp_stat_data {
> __u16 config_counter;
> };
>
> +#ifdef __KERNEL__
> +struct omap3isp_stat_data_time32 {
> + struct {
> + __s32 tv_sec;
> + __s32 tv_usec;
> + } ts;
> + __u32 buf;
> + __u32 buf_size;
> + __u16 frame_number;
> + __u16 cur_frame;
> + __u16 config_counter;
> +};
> +#endif
>
> /* Histogram related structs */
>
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2018-05-03 12:56 ` Sakari Ailus
@ 2018-05-03 22:06 ` Arnd Bergmann
2018-05-07 10:18 ` Sakari Ailus
0 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2018-05-03 22:06 UTC (permalink / raw)
To: Sakari Ailus
Cc: Laurent Pinchart, Mauro Carvalho Chehab,
Linux Media Mailing List, Linux Kernel Mailing List
On Thu, May 3, 2018 at 8:56 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Wed, Apr 25, 2018 at 11:30:10PM +0200, Arnd Bergmann wrote:
>> @@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
>> * @config_counter: Number of the configuration associated with the data.
>> */
>> struct omap3isp_stat_data {
>> +#ifdef __KERNEL__
>> + struct {
>> + __s64 tv_sec;
>> + __s64 tv_usec;
>
> Any particular reason for __s64 here instead of e.g. long or __s32? Kernel
> appears to use long in the timespec64 definition.
The user space 'timeval' definition is 16 bytes wide, with the layout
designed to be compatible between 32-bit and 64-bit, so it has to be like
this to match what user spaces sees with the old header files and a new
libc.
We don't yet know what the exact definition of timeval will be in all
libc implementations, but if they have a 32-bit tv_user field, it needs
padding next to it so the lower 32 bits are in the same place as they
would be using that 64-bit field I used.
Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2018-05-03 22:06 ` Arnd Bergmann
@ 2018-05-07 10:18 ` Sakari Ailus
0 siblings, 0 replies; 9+ messages in thread
From: Sakari Ailus @ 2018-05-07 10:18 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Laurent Pinchart, Mauro Carvalho Chehab,
Linux Media Mailing List, Linux Kernel Mailing List
On Thu, May 03, 2018 at 06:06:58PM -0400, Arnd Bergmann wrote:
> On Thu, May 3, 2018 at 8:56 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > On Wed, Apr 25, 2018 at 11:30:10PM +0200, Arnd Bergmann wrote:
> >> @@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
> >> * @config_counter: Number of the configuration associated with the data.
> >> */
> >> struct omap3isp_stat_data {
> >> +#ifdef __KERNEL__
> >> + struct {
> >> + __s64 tv_sec;
> >> + __s64 tv_usec;
> >
> > Any particular reason for __s64 here instead of e.g. long or __s32? Kernel
> > appears to use long in the timespec64 definition.
>
> The user space 'timeval' definition is 16 bytes wide, with the layout
> designed to be compatible between 32-bit and 64-bit, so it has to be like
> this to match what user spaces sees with the old header files and a new
> libc.
>
> We don't yet know what the exact definition of timeval will be in all
> libc implementations, but if they have a 32-bit tv_user field, it needs
tv_usec?
> padding next to it so the lower 32 bits are in the same place as they
> would be using that 64-bit field I used.
I presume the definition would be endianness dependent then. I have no
objections though, if you think this is the way to go.
I'll apply the patch.
Thanks.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2018-04-25 21:30 [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data Arnd Bergmann
2018-05-03 12:56 ` Sakari Ailus
@ 2018-05-07 13:17 ` Laurent Pinchart
2018-05-07 13:19 ` Sakari Ailus
1 sibling, 1 reply; 9+ messages in thread
From: Laurent Pinchart @ 2018-05-07 13:17 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel
Hi Arnd,
Thank you for the patch.
On Thursday, 26 April 2018 00:30:10 EEST Arnd Bergmann wrote:
> C libraries with 64-bit time_t use an incompatible format for
> struct omap3isp_stat_data. This changes the kernel code to
> support either version, by moving over the normal handling
> to the 64-bit variant, and adding compatiblity code to handle
> the old binary format with the existing ioctl command code.
>
> Fortunately, the command code includes the size of the structure,
> so the difference gets handled automatically. In the process of
> eliminating the references to 'struct timeval' from the kernel,
> I also change the way the timestamp is generated internally,
> basically by open-coding the v4l2_get_timestamp() call.
>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I submitted this one in November and asked again in January,
> still waiting for a review so it can be applied.
> ---
> drivers/media/platform/omap3isp/isph3a_aewb.c | 2 ++
> drivers/media/platform/omap3isp/isph3a_af.c | 2 ++
> drivers/media/platform/omap3isp/isphist.c | 2 ++
> drivers/media/platform/omap3isp/ispstat.c | 21 +++++++++++++++++++--
> drivers/media/platform/omap3isp/ispstat.h | 4 +++-
> include/uapi/linux/omap3isp.h | 22 ++++++++++++++++++++++
> 6 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c
> b/drivers/media/platform/omap3isp/isph3a_aewb.c index
> d44626f20ac6..3c82dea4d375 100644
> --- a/drivers/media/platform/omap3isp/isph3a_aewb.c
> +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
> @@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd,
> unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> unsigned long *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isph3a_af.c
> b/drivers/media/platform/omap3isp/isph3a_af.c index
> 99bd6cc21d86..4da25c84f0c6 100644
> --- a/drivers/media/platform/omap3isp/isph3a_af.c
> +++ b/drivers/media/platform/omap3isp/isph3a_af.c
> @@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd,
> unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> int *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isphist.c
> b/drivers/media/platform/omap3isp/isphist.c index
> a4ed5d140d48..d4be3d0e06f9 100644
> --- a/drivers/media/platform/omap3isp/isphist.c
> +++ b/drivers/media/platform/omap3isp/isphist.c
> @@ -435,6 +435,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned
> int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> int *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/ispstat.c
> b/drivers/media/platform/omap3isp/ispstat.c index
> 47cbc7e3d825..5967dfd0a9f7 100644
> --- a/drivers/media/platform/omap3isp/ispstat.c
> +++ b/drivers/media/platform/omap3isp/ispstat.c
> @@ -18,6 +18,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <linux/timekeeping.h>
Could you please keep the headers alphabetically sorted ?
>
> #include "isp.h"
>
> @@ -237,7 +238,7 @@ static int isp_stat_buf_queue(struct ispstat *stat)
> if (!stat->active_buf)
> return STAT_NO_BUF;
>
> - v4l2_get_timestamp(&stat->active_buf->ts);
> + ktime_get_ts64(&stat->active_buf->ts);
>
> stat->active_buf->buf_size = stat->buf_size;
> if (isp_stat_buf_check_magic(stat, stat->active_buf)) {
> @@ -500,7 +501,8 @@ int omap3isp_stat_request_statistics(struct ispstat
> *stat, return PTR_ERR(buf);
> }
>
> - data->ts = buf->ts;
> + data->ts.tv_sec = buf->ts.tv_sec;
> + data->ts.tv_usec = buf->ts.tv_nsec / NSEC_PER_USEC;
> data->config_counter = buf->config_counter;
> data->frame_number = buf->frame_number;
> data->buf_size = buf->buf_size;
> @@ -512,6 +514,21 @@ int omap3isp_stat_request_statistics(struct ispstat
> *stat, return 0;
> }
>
> +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> + struct omap3isp_stat_data_time32 *data)
> +{
> + struct omap3isp_stat_data data64;
> + int ret;
> +
> + ret = omap3isp_stat_request_statistics(stat, &data64);
> +
> + data->ts.tv_sec = data64.ts.tv_sec;
> + data->ts.tv_usec = data64.ts.tv_usec;
> + memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
> +
> + return ret;
We could return immediately after omap3isp_stat_request_statistics() if the
function fails, but that's no big deal, the error path is clearly a cold path.
> +}
> +
> /*
> * omap3isp_stat_config - Receives new statistic engine configuration.
> * @new_conf: Pointer to config structure.
> diff --git a/drivers/media/platform/omap3isp/ispstat.h
> b/drivers/media/platform/omap3isp/ispstat.h index
> 6d9b0244f320..923b38cfc682 100644
> --- a/drivers/media/platform/omap3isp/ispstat.h
> +++ b/drivers/media/platform/omap3isp/ispstat.h
> @@ -39,7 +39,7 @@ struct ispstat_buffer {
> struct sg_table sgt;
> void *virt_addr;
> dma_addr_t dma_addr;
> - struct timeval ts;
> + struct timespec64 ts;
> u32 buf_size;
> u32 frame_number;
> u16 config_counter;
> @@ -130,6 +130,8 @@ struct ispstat_generic_config {
> int omap3isp_stat_config(struct ispstat *stat, void *new_conf);
> int omap3isp_stat_request_statistics(struct ispstat *stat,
> struct omap3isp_stat_data *data);
> +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> + struct omap3isp_stat_data_time32 *data);
> int omap3isp_stat_init(struct ispstat *stat, const char *name,
> const struct v4l2_subdev_ops *sd_ops);
> void omap3isp_stat_cleanup(struct ispstat *stat);
> diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h
> index 1a920145db04..87b55755f4ff 100644
> --- a/include/uapi/linux/omap3isp.h
> +++ b/include/uapi/linux/omap3isp.h
> @@ -55,6 +55,8 @@
> _IOWR('V', BASE_VIDIOC_PRIVATE + 5, struct omap3isp_h3a_af_config)
> #define VIDIOC_OMAP3ISP_STAT_REQ \
> _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data)
> +#define VIDIOC_OMAP3ISP_STAT_REQ_TIME32 \
> + _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data_time32)
> #define VIDIOC_OMAP3ISP_STAT_EN \
> _IOWR('V', BASE_VIDIOC_PRIVATE + 7, unsigned long)
>
> @@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
> * @config_counter: Number of the configuration associated with the data.
> */
> struct omap3isp_stat_data {
> +#ifdef __KERNEL__
> + struct {
> + __s64 tv_sec;
> + __s64 tv_usec;
> + } ts;
I share Sakari's comment about this method implying a little-endian system,
but as the SoCs that integrate this device are all little-endian, that's not a
problem in practice.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
If you agree with the small comment about header ordering, let's get this
patch finally merged.
> +#else
> struct timeval ts;
> +#endif
> void __user *buf;
> __u32 buf_size;
> __u16 frame_number;
> @@ -173,6 +182,19 @@ struct omap3isp_stat_data {
> __u16 config_counter;
> };
>
> +#ifdef __KERNEL__
> +struct omap3isp_stat_data_time32 {
> + struct {
> + __s32 tv_sec;
> + __s32 tv_usec;
> + } ts;
> + __u32 buf;
> + __u32 buf_size;
> + __u16 frame_number;
> + __u16 cur_frame;
> + __u16 config_counter;
> +};
> +#endif
>
> /* Histogram related structs */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2018-05-07 13:17 ` Laurent Pinchart
@ 2018-05-07 13:19 ` Sakari Ailus
2018-05-07 20:36 ` Arnd Bergmann
0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2018-05-07 13:19 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Arnd Bergmann, Mauro Carvalho Chehab, linux-media, linux-kernel
On Mon, May 07, 2018 at 04:17:32PM +0300, Laurent Pinchart wrote:
> Hi Arnd,
>
> Thank you for the patch.
>
> On Thursday, 26 April 2018 00:30:10 EEST Arnd Bergmann wrote:
> > C libraries with 64-bit time_t use an incompatible format for
> > struct omap3isp_stat_data. This changes the kernel code to
> > support either version, by moving over the normal handling
> > to the 64-bit variant, and adding compatiblity code to handle
> > the old binary format with the existing ioctl command code.
> >
> > Fortunately, the command code includes the size of the structure,
> > so the difference gets handled automatically. In the process of
> > eliminating the references to 'struct timeval' from the kernel,
> > I also change the way the timestamp is generated internally,
> > basically by open-coding the v4l2_get_timestamp() call.
> >
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > I submitted this one in November and asked again in January,
> > still waiting for a review so it can be applied.
> > ---
> > drivers/media/platform/omap3isp/isph3a_aewb.c | 2 ++
> > drivers/media/platform/omap3isp/isph3a_af.c | 2 ++
> > drivers/media/platform/omap3isp/isphist.c | 2 ++
> > drivers/media/platform/omap3isp/ispstat.c | 21 +++++++++++++++++++--
> > drivers/media/platform/omap3isp/ispstat.h | 4 +++-
> > include/uapi/linux/omap3isp.h | 22 ++++++++++++++++++++++
> > 6 files changed, 50 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c
> > b/drivers/media/platform/omap3isp/isph3a_aewb.c index
> > d44626f20ac6..3c82dea4d375 100644
> > --- a/drivers/media/platform/omap3isp/isph3a_aewb.c
> > +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
> > @@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd,
> > unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> > case VIDIOC_OMAP3ISP_STAT_REQ:
> > return omap3isp_stat_request_statistics(stat, arg);
> > + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> > + return omap3isp_stat_request_statistics_time32(stat, arg);
> > case VIDIOC_OMAP3ISP_STAT_EN: {
> > unsigned long *en = arg;
> > return omap3isp_stat_enable(stat, !!*en);
> > diff --git a/drivers/media/platform/omap3isp/isph3a_af.c
> > b/drivers/media/platform/omap3isp/isph3a_af.c index
> > 99bd6cc21d86..4da25c84f0c6 100644
> > --- a/drivers/media/platform/omap3isp/isph3a_af.c
> > +++ b/drivers/media/platform/omap3isp/isph3a_af.c
> > @@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd,
> > unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> > case VIDIOC_OMAP3ISP_STAT_REQ:
> > return omap3isp_stat_request_statistics(stat, arg);
> > + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> > + return omap3isp_stat_request_statistics_time32(stat, arg);
> > case VIDIOC_OMAP3ISP_STAT_EN: {
> > int *en = arg;
> > return omap3isp_stat_enable(stat, !!*en);
> > diff --git a/drivers/media/platform/omap3isp/isphist.c
> > b/drivers/media/platform/omap3isp/isphist.c index
> > a4ed5d140d48..d4be3d0e06f9 100644
> > --- a/drivers/media/platform/omap3isp/isphist.c
> > +++ b/drivers/media/platform/omap3isp/isphist.c
> > @@ -435,6 +435,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned
> > int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> > case VIDIOC_OMAP3ISP_STAT_REQ:
> > return omap3isp_stat_request_statistics(stat, arg);
> > + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> > + return omap3isp_stat_request_statistics_time32(stat, arg);
> > case VIDIOC_OMAP3ISP_STAT_EN: {
> > int *en = arg;
> > return omap3isp_stat_enable(stat, !!*en);
> > diff --git a/drivers/media/platform/omap3isp/ispstat.c
> > b/drivers/media/platform/omap3isp/ispstat.c index
> > 47cbc7e3d825..5967dfd0a9f7 100644
> > --- a/drivers/media/platform/omap3isp/ispstat.c
> > +++ b/drivers/media/platform/omap3isp/ispstat.c
> > @@ -18,6 +18,7 @@
> > #include <linux/dma-mapping.h>
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > +#include <linux/timekeeping.h>
>
> Could you please keep the headers alphabetically sorted ?
>
> >
> > #include "isp.h"
> >
> > @@ -237,7 +238,7 @@ static int isp_stat_buf_queue(struct ispstat *stat)
> > if (!stat->active_buf)
> > return STAT_NO_BUF;
> >
> > - v4l2_get_timestamp(&stat->active_buf->ts);
> > + ktime_get_ts64(&stat->active_buf->ts);
> >
> > stat->active_buf->buf_size = stat->buf_size;
> > if (isp_stat_buf_check_magic(stat, stat->active_buf)) {
> > @@ -500,7 +501,8 @@ int omap3isp_stat_request_statistics(struct ispstat
> > *stat, return PTR_ERR(buf);
> > }
> >
> > - data->ts = buf->ts;
> > + data->ts.tv_sec = buf->ts.tv_sec;
> > + data->ts.tv_usec = buf->ts.tv_nsec / NSEC_PER_USEC;
> > data->config_counter = buf->config_counter;
> > data->frame_number = buf->frame_number;
> > data->buf_size = buf->buf_size;
> > @@ -512,6 +514,21 @@ int omap3isp_stat_request_statistics(struct ispstat
> > *stat, return 0;
> > }
> >
> > +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> > + struct omap3isp_stat_data_time32 *data)
> > +{
> > + struct omap3isp_stat_data data64;
> > + int ret;
> > +
> > + ret = omap3isp_stat_request_statistics(stat, &data64);
> > +
> > + data->ts.tv_sec = data64.ts.tv_sec;
> > + data->ts.tv_usec = data64.ts.tv_usec;
> > + memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
> > +
> > + return ret;
>
> We could return immediately after omap3isp_stat_request_statistics() if the
> function fails, but that's no big deal, the error path is clearly a cold path.
>
> > +}
> > +
> > /*
> > * omap3isp_stat_config - Receives new statistic engine configuration.
> > * @new_conf: Pointer to config structure.
> > diff --git a/drivers/media/platform/omap3isp/ispstat.h
> > b/drivers/media/platform/omap3isp/ispstat.h index
> > 6d9b0244f320..923b38cfc682 100644
> > --- a/drivers/media/platform/omap3isp/ispstat.h
> > +++ b/drivers/media/platform/omap3isp/ispstat.h
> > @@ -39,7 +39,7 @@ struct ispstat_buffer {
> > struct sg_table sgt;
> > void *virt_addr;
> > dma_addr_t dma_addr;
> > - struct timeval ts;
> > + struct timespec64 ts;
> > u32 buf_size;
> > u32 frame_number;
> > u16 config_counter;
> > @@ -130,6 +130,8 @@ struct ispstat_generic_config {
> > int omap3isp_stat_config(struct ispstat *stat, void *new_conf);
> > int omap3isp_stat_request_statistics(struct ispstat *stat,
> > struct omap3isp_stat_data *data);
> > +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> > + struct omap3isp_stat_data_time32 *data);
> > int omap3isp_stat_init(struct ispstat *stat, const char *name,
> > const struct v4l2_subdev_ops *sd_ops);
> > void omap3isp_stat_cleanup(struct ispstat *stat);
> > diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h
> > index 1a920145db04..87b55755f4ff 100644
> > --- a/include/uapi/linux/omap3isp.h
> > +++ b/include/uapi/linux/omap3isp.h
> > @@ -55,6 +55,8 @@
> > _IOWR('V', BASE_VIDIOC_PRIVATE + 5, struct omap3isp_h3a_af_config)
> > #define VIDIOC_OMAP3ISP_STAT_REQ \
> > _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data)
> > +#define VIDIOC_OMAP3ISP_STAT_REQ_TIME32 \
> > + _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data_time32)
> > #define VIDIOC_OMAP3ISP_STAT_EN \
> > _IOWR('V', BASE_VIDIOC_PRIVATE + 7, unsigned long)
> >
> > @@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
> > * @config_counter: Number of the configuration associated with the data.
> > */
> > struct omap3isp_stat_data {
> > +#ifdef __KERNEL__
> > + struct {
> > + __s64 tv_sec;
> > + __s64 tv_usec;
> > + } ts;
>
> I share Sakari's comment about this method implying a little-endian system,
> but as the SoCs that integrate this device are all little-endian, that's not a
> problem in practice.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> If you agree with the small comment about header ordering, let's get this
> patch finally merged.
I can make the change locally in my tree, no need to resend.
Thanks.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2018-05-07 13:19 ` Sakari Ailus
@ 2018-05-07 20:36 ` Arnd Bergmann
2018-05-07 21:33 ` Sakari Ailus
0 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2018-05-07 20:36 UTC (permalink / raw)
To: Sakari Ailus
Cc: Laurent Pinchart, Mauro Carvalho Chehab,
Linux Media Mailing List, Linux Kernel Mailing List
On Mon, May 7, 2018 at 9:19 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Mon, May 07, 2018 at 04:17:32PM +0300, Laurent Pinchart wrote:
>> On Thursday, 26 April 2018 00:30:10 EEST Arnd Bergmann wrote:
>> > +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
>> > + struct omap3isp_stat_data_time32 *data)
>> > +{
>> > + struct omap3isp_stat_data data64;
>> > + int ret;
>> > +
>> > + ret = omap3isp_stat_request_statistics(stat, &data64);
>> > +
>> > + data->ts.tv_sec = data64.ts.tv_sec;
>> > + data->ts.tv_usec = data64.ts.tv_usec;
>> > + memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
>> > +
>> > + return ret;
>>
>> We could return immediately after omap3isp_stat_request_statistics() if the
>> function fails, but that's no big deal, the error path is clearly a cold path.
I looked at it again and briefly thought that it would leak kernel stack
data in my version and changing it would be required to avoid that,
but I do see now that the absence of the INFO_FL_ALWAYS_COPY
flag makes it safe after all.
I agree that returning early here would be nicer here, I'll leave it up to
Sakari to fold in that change if he likes.
>> > @@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
>> > * @config_counter: Number of the configuration associated with the data.
>> > */
>> > struct omap3isp_stat_data {
>> > +#ifdef __KERNEL__
>> > + struct {
>> > + __s64 tv_sec;
>> > + __s64 tv_usec;
>> > + } ts;
>>
>> I share Sakari's comment about this method implying a little-endian system,
>> but as the SoCs that integrate this device are all little-endian, that's not a
>> problem in practice.
To clarify: the version I have here does *not* imply a little-endian system,
it is supposed to work on both little-endian and big-endian builds, and
endianess is not a property of the SoC either -- you should be able to
build a big-endian kernel and run it on OMAP3 (aside from bugs in other
drivers). Using 'long' here instead of __s64 would however make this
interface broken on big-endian builds since the glibc definition of timeval
does include extra padding on big-endian machines to make the structure
compatible between 32-bit and 64-bit ABIs.
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> If you agree with the small comment about header ordering, let's get this
>> patch finally merged.
>
> I can make the change locally in my tree, no need to resend.
>
> Thanks.
Thanks a lot!
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2018-05-07 20:36 ` Arnd Bergmann
@ 2018-05-07 21:33 ` Sakari Ailus
2018-05-08 0:08 ` Arnd Bergmann
0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2018-05-07 21:33 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Laurent Pinchart, Mauro Carvalho Chehab,
Linux Media Mailing List, Linux Kernel Mailing List
On Mon, May 07, 2018 at 04:36:45PM -0400, Arnd Bergmann wrote:
> On Mon, May 7, 2018 at 9:19 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > On Mon, May 07, 2018 at 04:17:32PM +0300, Laurent Pinchart wrote:
> >> On Thursday, 26 April 2018 00:30:10 EEST Arnd Bergmann wrote:
> >> > +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> >> > + struct omap3isp_stat_data_time32 *data)
> >> > +{
> >> > + struct omap3isp_stat_data data64;
> >> > + int ret;
> >> > +
> >> > + ret = omap3isp_stat_request_statistics(stat, &data64);
> >> > +
> >> > + data->ts.tv_sec = data64.ts.tv_sec;
> >> > + data->ts.tv_usec = data64.ts.tv_usec;
> >> > + memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
> >> > +
> >> > + return ret;
> >>
> >> We could return immediately after omap3isp_stat_request_statistics() if the
> >> function fails, but that's no big deal, the error path is clearly a cold path.
>
> I looked at it again and briefly thought that it would leak kernel stack
> data in my version and changing it would be required to avoid that,
> but I do see now that the absence of the INFO_FL_ALWAYS_COPY
> flag makes it safe after all.
>
> I agree that returning early here would be nicer here, I'll leave it up to
> Sakari to fold in that change if he likes.
I agree with the change; actually the data64 struct will be left untouched
if there's an error so changing this doesn't seem to make a difference.
Private IOCTLs have always_copy == false, so the argument struct isn't
copied back to the kernel.
The diff is here. Let me know if something went wrong...
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index dfee8eaf2226..47353fee26c3 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -519,12 +519,14 @@ int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
int ret;
ret = omap3isp_stat_request_statistics(stat, &data64);
+ if (ret)
+ return ret;
data->ts.tv_sec = data64.ts.tv_sec;
data->ts.tv_usec = data64.ts.tv_usec;
memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
- return ret;
+ return 0;
}
/*
>
> >> > @@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
> >> > * @config_counter: Number of the configuration associated with the data.
> >> > */
> >> > struct omap3isp_stat_data {
> >> > +#ifdef __KERNEL__
> >> > + struct {
> >> > + __s64 tv_sec;
> >> > + __s64 tv_usec;
> >> > + } ts;
> >>
> >> I share Sakari's comment about this method implying a little-endian system,
> >> but as the SoCs that integrate this device are all little-endian, that's not a
> >> problem in practice.
>
> To clarify: the version I have here does *not* imply a little-endian system,
> it is supposed to work on both little-endian and big-endian builds, and
> endianess is not a property of the SoC either -- you should be able to
> build a big-endian kernel and run it on OMAP3 (aside from bugs in other
> drivers). Using 'long' here instead of __s64 would however make this
> interface broken on big-endian builds since the glibc definition of timeval
> does include extra padding on big-endian machines to make the structure
> compatible between 32-bit and 64-bit ABIs.
Ah, there you go. :-)
>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>
> >> If you agree with the small comment about header ordering, let's get this
> >> patch finally merged.
> >
> > I can make the change locally in my tree, no need to resend.
> >
> > Thanks.
>
> Thanks a lot!
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2018-05-07 21:33 ` Sakari Ailus
@ 2018-05-08 0:08 ` Arnd Bergmann
0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2018-05-08 0:08 UTC (permalink / raw)
To: Sakari Ailus
Cc: Laurent Pinchart, Mauro Carvalho Chehab,
Linux Media Mailing List, Linux Kernel Mailing List
On Mon, May 7, 2018 at 5:33 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Mon, May 07, 2018 at 04:36:45PM -0400, Arnd Bergmann wrote:
>> On Mon, May 7, 2018 at 9:19 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> > On Mon, May 07, 2018 at 04:17:32PM +0300, Laurent Pinchart wrote:
>> >> On Thursday, 26 April 2018 00:30:10 EEST Arnd Bergmann wrote:
>> >> > +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
>> >> > + struct omap3isp_stat_data_time32 *data)
>> >> > +{
>> >> > + struct omap3isp_stat_data data64;
>> >> > + int ret;
>> >> > +
>> >> > + ret = omap3isp_stat_request_statistics(stat, &data64);
>> >> > +
>> >> > + data->ts.tv_sec = data64.ts.tv_sec;
>> >> > + data->ts.tv_usec = data64.ts.tv_usec;
>> >> > + memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
>> >> > +
>> >> > + return ret;
>> >>
>> >> We could return immediately after omap3isp_stat_request_statistics() if the
>> >> function fails, but that's no big deal, the error path is clearly a cold path.
>>
>> I looked at it again and briefly thought that it would leak kernel stack
>> data in my version and changing it would be required to avoid that,
>> but I do see now that the absence of the INFO_FL_ALWAYS_COPY
>> flag makes it safe after all.
>>
>> I agree that returning early here would be nicer here, I'll leave it up to
>> Sakari to fold in that change if he likes.
>
> I agree with the change; actually the data64 struct will be left untouched
> if there's an error so changing this doesn't seem to make a difference.
> Private IOCTLs have always_copy == false, so the argument struct isn't
> copied back to the kernel.
>
> The diff is here. Let me know if something went wrong...
>
> diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
> index dfee8eaf2226..47353fee26c3 100644
> --- a/drivers/media/platform/omap3isp/ispstat.c
> +++ b/drivers/media/platform/omap3isp/ispstat.c
> @@ -519,12 +519,14 @@ int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> int ret;
>
> ret = omap3isp_stat_request_statistics(stat, &data64);
> + if (ret)
> + return ret;
>
> data->ts.tv_sec = data64.ts.tv_sec;
> data->ts.tv_usec = data64.ts.tv_usec;
> memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
>
> - return ret;
> + return 0;
> }
Yes, that's exactly what I had in mind. Thanks for fixing it up!
Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-05-08 0:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-25 21:30 [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data Arnd Bergmann
2018-05-03 12:56 ` Sakari Ailus
2018-05-03 22:06 ` Arnd Bergmann
2018-05-07 10:18 ` Sakari Ailus
2018-05-07 13:17 ` Laurent Pinchart
2018-05-07 13:19 ` Sakari Ailus
2018-05-07 20:36 ` Arnd Bergmann
2018-05-07 21:33 ` Sakari Ailus
2018-05-08 0:08 ` Arnd Bergmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.