linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Alastair D'Silva" <alastair@d-silva.org>
To: Joe Perches <joe@perches.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Karsten Keil <isdn@linux-pingi.de>,
	Jassi Brar <jassisinghbrar@gmail.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jose Abreu <Jose.Abreu@synopsys.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	Stanislaw Gruszka <sgruszka@redhat.com>,
	Benson Leung <bleung@chromium.org>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Petr Mladek <pmladek@suse.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	David Laight <David.Laight@ACULAB.COM>,
	Andrew Morton <akpm@linux-foundation.org>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	ath10k@lists.infradead.org, linux-wireless@vger.kernel.org,
	linux-scsi@vger.kernel.org, linux-fbdev@vger.kernel.org,
	devel@driverdev.osuosl.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v4 4/7] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags
Date: Wed, 26 Jun 2019 11:27:48 +1000	[thread overview]
Message-ID: <1166c97d5f0ff750e5871937eb1d7e3c0423bbdd.camel@d-silva.org> (raw)
In-Reply-To: <3340b520a57e00a483eae170be97316c8d18c22c.camel@perches.com>

On Mon, 2019-06-24 at 22:01 -0700, Joe Perches wrote:
> On Tue, 2019-06-25 at 13:17 +1000, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> > 
> > In order to support additional features, rename hex_dump_to_buffer
> > to
> > hex_dump_to_buffer_ext, and replace the ascii bool parameter with
> > flags.
> []
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> > b/drivers/gpu/drm/i915/intel_engine_cs.c
> []
> > @@ -1338,9 +1338,8 @@ static void hexdump(struct drm_printer *m,
> > const void *buf, size_t len)
> >  		}
> >  
> >  		WARN_ON_ONCE(hex_dump_to_buffer(buf + pos, len - pos,
> > -						rowsize, sizeof(u32),
> > -						line, sizeof(line),
> > -						false) >=
> > sizeof(line));
> > +						rowsize, sizeof(u32),
> > line,
> > +						sizeof(line)) >=
> > sizeof(line));
> 
> Huh?  Why do this?

The ascii parameter was removed from the simple API as per Jani's
suggestion. The remainder was reformatted to avoid exceeding the line
length limits.

> 
> > diff --git a/drivers/isdn/hardware/mISDN/mISDNisar.c
> > b/drivers/isdn/hardware/mISDN/mISDNisar.c
> []
> > @@ -70,8 +70,9 @@ send_mbox(struct isar_hw *isar, u8 his, u8 creg,
> > u8 len, u8 *msg)
> >  			int l = 0;
> >  
> >  			while (l < (int)len) {
> > -				hex_dump_to_buffer(msg + l, len - l,
> > 32, 1,
> > -						   isar->log, 256, 1);
> > +				hex_dump_to_buffer_ext(msg + l, len -
> > l, 32, 1,
> > +						       isar->log, 256,
> > +						       HEXDUMP_ASCII);
> 
> Again, why do any of these?
> 
> The point of the wrapper is to avoid changing these.

Jani made a pretty good point that about half the callers didn't want
an ASCII dump, and presenting a simplified API makes sense.

I would actually put forward that we consider dropping rowsize from the
simplified API too, as most callers use 32, and those that use 16 would
probably be OK with 32.

Your proposal, on the other hand, only makes sense if there were many
callers, and even so, not in the form that you presented, since that
result in a mix of booleans & bitfields that you were critical of.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org



  parent reply	other threads:[~2019-06-26  1:28 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-25  3:17 [PATCH v4 0/7] Hexdump Enhancements Alastair D'Silva
2019-06-25  3:17 ` [PATCH v4 1/7] lib/hexdump.c: Fix selftests Alastair D'Silva
2019-06-25  3:17 ` [PATCH v4 2/7] lib/hexdump.c: Relax rowsize checks in hex_dump_to_buffer Alastair D'Silva
2019-06-25  3:17 ` [PATCH v4 3/7] lib/hexdump.c: Optionally suppress lines of repeated bytes Alastair D'Silva
2019-06-25 19:13   ` kbuild test robot
2019-06-25  3:17 ` [PATCH v4 4/7] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags Alastair D'Silva
2019-06-25  5:01   ` Joe Perches
2019-06-25  5:06     ` Alastair D'Silva
2019-06-25  5:17       ` Joe Perches
2019-06-25  5:19       ` Joe Perches
2019-06-26  1:27         ` Alastair D'Silva
2019-06-26  1:27     ` Alastair D'Silva [this message]
2019-06-25 18:00   ` kbuild test robot
2019-06-25 22:52   ` kbuild test robot
2019-06-25  3:17 ` [PATCH v4 5/7] lib/hexdump.c: Allow multiple groups to be separated by lines '|' Alastair D'Silva
2019-06-25  5:37   ` Joe Perches
2019-06-26  1:28     ` Alastair D'Silva
2019-06-25  3:17 ` [PATCH v4 6/7] lib/hexdump.c: Allow multiple groups to be separated by spaces Alastair D'Silva
2019-06-25  3:17 ` [PATCH v4 7/7] lib/hexdump.c: Optionally retain byte ordering Alastair D'Silva
2019-06-25  5:01 ` [PATCH v4 0/7] Hexdump Enhancements Joe Perches
2019-06-26  1:02   ` Alastair D'Silva

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1166c97d5f0ff750e5871937eb1d7e3c0423bbdd.camel@d-silva.org \
    --to=alastair@d-silva.org \
    --cc=David.Laight@ACULAB.COM \
    --cc=Jose.Abreu@synopsys.com \
    --cc=airlied@linux.ie \
    --cc=akpm@linux-foundation.org \
    --cc=ath10k@lists.infradead.org \
    --cc=bleung@chromium.org \
    --cc=dan.carpenter@oracle.com \
    --cc=daniel@ffwll.ch \
    --cc=davem@davemloft.net \
    --cc=devel@driverdev.osuosl.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=enric.balletbo@collabora.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=isdn@linux-pingi.de \
    --cc=jani.nikula@linux.intel.com \
    --cc=jassisinghbrar@gmail.com \
    --cc=jejb@linux.ibm.com \
    --cc=joe@perches.com \
    --cc=joonas.lahtinen@linux.intel.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=netdev@vger.kernel.org \
    --cc=pmladek@suse.com \
    --cc=rodrigo.vivi@intel.com \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=sgruszka@redhat.com \
    --cc=thomas.lendacky@amd.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).