Re: [PATCH v2 1/2] console: Remove dummy con_font_op() callback implementations

From: Peilin Ye <yepeilin.cs@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>, Jiri Slaby <jirislaby@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thomas Winischhofer <thomas@winischhofer.net>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Nicolas Pitre <nico@fluxnic.net>,
	"Gustavo A . R . Silva" <gustavoars@kernel.org>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	George Kennedy <george.kennedy@oracle.com>,
	Nathan Chancellor <natechancellor@gmail.com>,
	Peter Rosin <peda@axentia.se>,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
	dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org
Subject: Re: [PATCH v2 1/2] console: Remove dummy con_font_op() callback implementations
Date: Mon, 2 Nov 2020 05:52:28 -0500	[thread overview]
Message-ID: <20201102105228.GA1558395@PWN> (raw)
In-Reply-To: <20201102101347.GN401619@phenom.ffwll.local>

On Mon, Nov 02, 2020 at 11:13:47AM +0100, Daniel Vetter wrote:
> On Mon, Nov 02, 2020 at 10:47:55AM +0100, Jiri Slaby wrote:
> > On 02. 11. 20, 10:36, Peilin Ye wrote:
> > > `struct console_font` is a UAPI structure, thus ideally should not be
> > > used for kernel internal abstraction. Remove some dummy .con_font_set,
> > > .con_font_default and .con_font_copy `struct consw` callback
> > > implementations, to make it cleaner.
> > 
> > ESEMANTIC_ERROR.
> > 
> > 1) What do you refer to with the last "it"?
> > 
> > 2) What's the purpose of mentioning struct console_font at all?
> > 
> > 3) Could you clarify whether you checked it is safe to remove the hooks?

I see. I will try to elaborate in future patches, thank you!

> > 4) All the hooks now return ENOSYS for both consoles (and not 0). Is this
> > intentional?
> > 
> > I know answers to the first 3 questions, but you need to elaborate a bit in
> > the commit log to connect those sentences. Esp. for people not dealing with
> > the code on a daily basis. Ad 4) I am not sure.
> 
> Yup the behaviour change from 4) needs to be called out. I think this
> should then also be done as part of the large patch series to remove the
> dummy functions from all console drivers.
> 
> I don't expect the errno change to cause trouble, and it's the more honest
> errno - changing fonts not supported is the truth. But if it is, we can
> patch that up appropriately when we get a regression report. That's kinda
> unavoidable with old crufty uapi like this one here.
> 
> Also a bikeshed: Additional information like the patch changelog or
> reasons why you do something is imo best to include in the commit message
> itself. It ends up looking a bit less tidy sometimes, but often there's
> crucial information in these parts that was accidentally left out from the
> commit message.

Sure, I will try to include sufficient information for one to easily
understand what I'm trying to do with a patch. Thank you,

Peilin