Message ID | 20231024191027.305622-2-detlev.casanova@collabora.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1qvMnL-006Tgf-5R; Tue, 24 Oct 2023 19:10:27 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344227AbjJXTKZ (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 24 Oct 2023 15:10:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52990 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1343884AbjJXTKX (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 24 Oct 2023 15:10:23 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 91AE710C3; Tue, 24 Oct 2023 12:10:21 -0700 (PDT) Received: from arisu.hitronhub.home (unknown [23.233.251.139]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: detlev) by madras.collabora.co.uk (Postfix) with ESMTPSA id B43936607333; Tue, 24 Oct 2023 20:10:19 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1698174620; bh=QzXZQ5qW45emjNZOoZBacDZXLZ1f0TmfU80OiA4TC/E=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=UYvc3HSAZvat0g51dUgwMnhIxMzTeig7f+ZRkCB+EbDN/SJLW34szjzL56d22Gp/J szJIoheeTE8w2mXrAj2lyu7Gs5PPUpJ/03vc3Zb5qfghWf7C9NeHSOtjksAVyrKQrR NV9V9dOUrSoRItv3hi/Xs8gKC1PDCuxI1ZlfRe8m9oZoyMgTDod5PB3QabOGuTv0U1 0miFeOdpiydJDk33YtmXLbzJPbaYNsoPanqP+2dzbzWOifdDbBHaQwN/y6ulr62xOi C0WGxZ4/TeZhRYumDIDXRoZNN6W/srooulskWly3Qq3MJ7Jmcx6GThxBXn8m+3ab6o pSFaRocKP16Og== From: Detlev Casanova <detlev.casanova@collabora.com> To: linux-kernel@vger.kernel.org Cc: linux-media@vger.kernel.org, Daniel Almeida <daniel.almeida@collabora.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Detlev Casanova <detlev.casanova@collabora.com> Subject: [PATCH v2 1/5] media: visl: Fix params permissions/defaults mismatch Date: Tue, 24 Oct 2023 15:09:46 -0400 Message-ID: <20231024191027.305622-2-detlev.casanova@collabora.com> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231024191027.305622-1-detlev.casanova@collabora.com> References: <20231024191027.305622-1-detlev.casanova@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -4.8 (----) X-LSpam-Report: No, score=-4.8 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3 autolearn=ham autolearn_force=no |
Series |
visl: Adapt output frames for reference comparison
|
|
Commit Message
Detlev Casanova
Oct. 24, 2023, 7:09 p.m. UTC
`false` was used as the keep_bitstream_buffers parameter permissions. This looks more like a default value for the parameter, so change it to 0 to avoid confusion. Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> --- drivers/media/test-drivers/visl/visl-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 24/10/2023 21:09, Detlev Casanova wrote: > `false` was used as the keep_bitstream_buffers parameter permissions. > This looks more like a default value for the parameter, so change it to > 0 to avoid confusion. > > Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > --- > drivers/media/test-drivers/visl/visl-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c > index 9970dc739ca5..df6515530fbf 100644 > --- a/drivers/media/test-drivers/visl/visl-core.c > +++ b/drivers/media/test-drivers/visl/visl-core.c > @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes, > " the number of frames to trace with dprintk"); > > bool keep_bitstream_buffers; > -module_param(keep_bitstream_buffers, bool, false); > +module_param(keep_bitstream_buffers, bool, 0); ??? This last parameter is the permission, it makes no sense that this is either 0 or false: then nobody can see it in /sys/modules/. Typically this is 0444 if it is readable only, or 0644 if it can be written by root. Regards, Hans > MODULE_PARM_DESC(keep_bitstream_buffers, > " keep bitstream buffers in debugfs after streaming is stopped"); >
On Wednesday, November 22, 2023 10:56:20 A.M. EST Hans Verkuil wrote: > On 24/10/2023 21:09, Detlev Casanova wrote: > > `false` was used as the keep_bitstream_buffers parameter permissions. > > This looks more like a default value for the parameter, so change it to > > 0 to avoid confusion. > > > > Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com> > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > > --- > > > > drivers/media/test-drivers/visl/visl-core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/test-drivers/visl/visl-core.c > > b/drivers/media/test-drivers/visl/visl-core.c index > > 9970dc739ca5..df6515530fbf 100644 > > --- a/drivers/media/test-drivers/visl/visl-core.c > > +++ b/drivers/media/test-drivers/visl/visl-core.c > > @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes, > > > > " the number of frames to trace with dprintk"); > > > > bool keep_bitstream_buffers; > > > > -module_param(keep_bitstream_buffers, bool, false); > > +module_param(keep_bitstream_buffers, bool, 0); > > ??? > > This last parameter is the permission, it makes no sense that this > is either 0 or false: then nobody can see it in /sys/modules/. It makes sense if we want it set when the module is loaded only. This way, we don't have to manage the parameters values changing while work is being done and keep it simple. It could be made readable if that looks better, but there is no real need for it to be read either. > Typically this is 0444 if it is readable only, or 0644 if it can be > written by root. > > Regards, > > Hans > > > MODULE_PARM_DESC(keep_bitstream_buffers, > > > > " keep bitstream buffers in debugfs after streaming is stopped");
On 22/11/2023 17:38, Detlev Casanova wrote: > On Wednesday, November 22, 2023 10:56:20 A.M. EST Hans Verkuil wrote: >> On 24/10/2023 21:09, Detlev Casanova wrote: >>> `false` was used as the keep_bitstream_buffers parameter permissions. >>> This looks more like a default value for the parameter, so change it to >>> 0 to avoid confusion. >>> >>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com> >>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> >>> --- >>> >>> drivers/media/test-drivers/visl/visl-core.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/test-drivers/visl/visl-core.c >>> b/drivers/media/test-drivers/visl/visl-core.c index >>> 9970dc739ca5..df6515530fbf 100644 >>> --- a/drivers/media/test-drivers/visl/visl-core.c >>> +++ b/drivers/media/test-drivers/visl/visl-core.c >>> @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes, >>> >>> " the number of frames to trace with dprintk"); >>> >>> bool keep_bitstream_buffers; >>> >>> -module_param(keep_bitstream_buffers, bool, false); >>> +module_param(keep_bitstream_buffers, bool, 0); >> >> ??? >> >> This last parameter is the permission, it makes no sense that this >> is either 0 or false: then nobody can see it in /sys/modules/. > > It makes sense if we want it set when the module is loaded only. This way, we > don't have to manage the parameters values changing while work is being done > and keep it simple. > It could be made readable if that looks better, but there is no real need for > it to be read either. Why not? It makes it easy to read what this module option's value is. I now see that both visl and vidtv uses 0 a lot, so I'm OK with this patch for consistency. But I think especially these test-drivers should use 0444 instead of 0 so you can see how the test driver is configured. That might actually be relevant when writing tests using these drivers. Perhaps separate patches for visl and vidtv that change 0 to 0444 for all the module parameters? Regards, Hans > >> Typically this is 0444 if it is readable only, or 0644 if it can be >> written by root. >> >> Regards, >> >> Hans >> >>> MODULE_PARM_DESC(keep_bitstream_buffers, >>> >>> " keep bitstream buffers in debugfs after streaming is > stopped"); >
diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c index 9970dc739ca5..df6515530fbf 100644 --- a/drivers/media/test-drivers/visl/visl-core.c +++ b/drivers/media/test-drivers/visl/visl-core.c @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes, " the number of frames to trace with dprintk"); bool keep_bitstream_buffers; -module_param(keep_bitstream_buffers, bool, false); +module_param(keep_bitstream_buffers, bool, 0); MODULE_PARM_DESC(keep_bitstream_buffers, " keep bitstream buffers in debugfs after streaming is stopped");