From patchwork Mon May 7 21:33:05 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sakari Ailus X-Patchwork-Id: 10384827 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 477CF60318 for ; Mon, 7 May 2018 21:33:24 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4BB3E28AE6 for ; Mon, 7 May 2018 21:33:24 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4091628B02; Mon, 7 May 2018 21:33:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D9D2E28AE6 for ; Mon, 7 May 2018 21:33:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752737AbeEGVdJ (ORCPT ); Mon, 7 May 2018 17:33:09 -0400 Received: from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:41734 "EHLO hillosipuli.retiisi.org.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752081AbeEGVdI (ORCPT ); Mon, 7 May 2018 17:33:08 -0400 Received: from valkosipuli.localdomain (valkosipuli.retiisi.org.uk [IPv6:2001:1bc8:1a6:d3d5::80:2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by hillosipuli.retiisi.org.uk (Postfix) with ESMTPS id 36E0D634C4E; Tue, 8 May 2018 00:33:06 +0300 (EEST) Received: from sakke by valkosipuli.localdomain with local (Exim 4.89) (envelope-from ) id 1fFnkn-0003jZ-SE; Tue, 08 May 2018 00:33:05 +0300 Date: Tue, 8 May 2018 00:33:05 +0300 From: Sakari Ailus To: Arnd Bergmann Cc: Laurent Pinchart , Mauro Carvalho Chehab , Linux Media Mailing List , Linux Kernel Mailing List Subject: Re: [PATCH] [RESEND] [media] omap3isp: support 64-bit version of omap3isp_stat_data Message-ID: <20180507213305.kfxabgkdvf7xwt7m@valkosipuli.retiisi.org.uk> References: <20180425213044.1535393-1-arnd@arndb.de> <2922276.lKgGZtlCEW@avalon> <20180507131906.rdvcmvim5gvi5odk@valkosipuli.retiisi.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, May 07, 2018 at 04:36:45PM -0400, Arnd Bergmann wrote: > On Mon, May 7, 2018 at 9:19 AM, Sakari Ailus 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... > > >> > @@ -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 > >> > >> 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! 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; } /*