Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: WeiXiong Liao <liaoweixiong@allwinnertech.com>
Cc: Rob Herring <robh@kernel.org>, Tony Luck <tony.luck@intel.com>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Richard Weinberger <richard@nod.at>,
	Anton Vorontsov <anton@enomsg.org>,
	linux-doc@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, Colin Cross <ccross@android.com>,
	linux-mtd@lists.infradead.org,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v2 08/11] blkoops: respect for device to pick recorders
Date: Wed, 18 Mar 2020 11:42:50 -0700
Message-ID: <202003181136.5F115BFCC1@keescook> (raw)
In-Reply-To: <1581078355-19647-9-git-send-email-liaoweixiong@allwinnertech.com>

In the subject and through-out:

s/recorders/pstore front-ends/

On Fri, Feb 07, 2020 at 08:25:52PM +0800, WeiXiong Liao wrote:
> It's one of a series of patches for adaptive to MTD device.

typo: adapting

> 
> MTD device is not block device. The sector of flash (MTD device) will be
> broken if erase over limited cycles. Avoid damaging block so fast, we
> can not write to a sector frequently. So, the recorders of pstore/blk
> like console and ftrace recorder should not be supported.
> 
> Besides, mtd device need aligned write/erase size. To avoid
> over-erasing/writing flash, we should keep a aligned cache and read old
> data to cache before write/erase, which make codes more complex. So,
> pmsg do not be supported now because it writes misaligned.
> 
> How about dmesg? Luckly, pstore/blk keeps several aligned chunks for
> dmesg and uses one by one for wear balance.
> 
> So, MTD device for pstore should pick recorders, that is why the patch
> here.
> 
> Signed-off-by: WeiXiong Liao <liaoweixiong@allwinnertech.com>
> ---
>  Documentation/admin-guide/pstore-block.rst |  9 +++++++++
>  fs/pstore/blkoops.c                        | 29 +++++++++++++++++++++--------
>  include/linux/blkoops.h                    | 14 +++++++++++++-
>  3 files changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/admin-guide/pstore-block.rst b/Documentation/admin-guide/pstore-block.rst
> index be865dfc1a28..299142b3d8e6 100644
> --- a/Documentation/admin-guide/pstore-block.rst
> +++ b/Documentation/admin-guide/pstore-block.rst
> @@ -166,6 +166,15 @@ It is only required by block device which is registered by
>  ``blkoops_register_blkdev``.  It's the major device number of registered
>  devices, by which blkoops can get the matching driver for @blkdev.
>  
> +flags
> +~~~~~
> +
> +Refer to macro starting with *BLKOOPS_DEV_SUPPORT_* which is defined in
> +*linux/blkoops.h*. They tell us that which pstore/blk recorders this device
> +supports. Default zero means all recorders for compatible, witch is the same

typo: witch -> which

> +as BLKOOPS_DEV_SUPPORT_ALL. Recorder works only when chunk size is not zero
> +and device support.

There are already flags for this, please see "Supported frontends"
in include/linux/pstore.h

> +
>  total_size
>  ~~~~~~~~~~
>  
> diff --git a/fs/pstore/blkoops.c b/fs/pstore/blkoops.c
> index c76bab671b0b..01170b344f00 100644
> --- a/fs/pstore/blkoops.c
> +++ b/fs/pstore/blkoops.c
> @@ -128,9 +128,16 @@ int blkoops_register_device(struct blkoops_device *bo_dev)
>  		return -ENOMEM;
>  	}
>  
> -#define verify_size(name, defsize, alignsize) {				\
> -		long _##name_ = (name);					\
> -		if (_##name_ < 0)					\
> +	/* zero means all recorders for compatible */
> +	if (bo_dev->flags == BLKOOPS_DEV_SUPPORT_DEFAULT)
> +		bo_dev->flags = BLKOOPS_DEV_SUPPORT_ALL;
> +#define verify_size(name, defsize, alignsize, enable) {			\
> +		long _##name_;						\
> +		if (!(enable))						\
> +			_##name_ = 0;					\
> +		else if ((name) >= 0)					\
> +			_##name_ = (name);				\
> +		else							\
>  			_##name_ = (defsize);				\
>  		_##name_ = _##name_ <= 0 ? 0 : (_##name_ * 1024);	\
>  		if (_##name_ & ((alignsize) - 1)) {			\
> @@ -142,10 +149,14 @@ int blkoops_register_device(struct blkoops_device *bo_dev)
>  		bzinfo->name = _##name_;				\
>  	}
>  
> -	verify_size(dmesg_size, DEFAULT_DMESG_SIZE, 4096);
> -	verify_size(pmsg_size, DEFAULT_PMSG_SIZE, 4096);
> -	verify_size(console_size, DEFAULT_CONSOLE_SIZE, 4096);
> -	verify_size(ftrace_size, DEFAULT_FTRACE_SIZE, 4096);
> +	verify_size(dmesg_size, DEFAULT_DMESG_SIZE, 4096,
> +			bo_dev->flags & BLKOOPS_DEV_SUPPORT_DMESG);
> +	verify_size(pmsg_size, DEFAULT_PMSG_SIZE, 4096,
> +			bo_dev->flags & BLKOOPS_DEV_SUPPORT_PMSG);
> +	verify_size(console_size, DEFAULT_CONSOLE_SIZE, 4096,
> +			bo_dev->flags & BLKOOPS_DEV_SUPPORT_CONSOLE);
> +	verify_size(ftrace_size, DEFAULT_FTRACE_SIZE, 4096,
> +			bo_dev->flags & BLKOOPS_DEV_SUPPORT_FTRACE);

I'd kind of prefer this patch be moved much earlier in the series so
that the later additions of front-end support doesn't have to be touched
twice. i.e. when PMSG support is added, it is added as a whole here and
does the flag check in that patch, etc.

>  #undef verify_size
>  	dump_oops = !!(dump_oops < 0 ? DEFAULT_DUMP_OOPS : dump_oops);
>  
> @@ -336,6 +347,7 @@ static ssize_t blkoops_blk_panic_write(const char *buf, size_t size,
>   * register block device to blkoops
>   * @major: the major device number of registering device
>   * @panic_write: the write interface for panic case.
> + * @flags: Refer to macro starting with BLKOOPS_DEV_SUPPORT.
>   *
>   * It is ONLY used for block device to register to blkoops. In this case,
>   * the module parameter @blkdev must be valid. Generic read/write interfaces
> @@ -349,7 +361,7 @@ static ssize_t blkoops_blk_panic_write(const char *buf, size_t size,
>   * panic occurs but pstore/blk does not recover yet, the first zone of dmesg
>   * will be used.
>   */
> -int blkoops_register_blkdev(unsigned int major,
> +int blkoops_register_blkdev(unsigned int major, unsigned int flags,
>  		blkoops_blk_panic_write_op panic_write)
>  {
>  	struct block_device *bdev;
> @@ -372,6 +384,7 @@ int blkoops_register_blkdev(unsigned int major,
>  	if (bo_dev.total_size == 0)
>  		goto err_put_bdev;
>  	bo_dev.panic_write = panic_write ? blkoops_blk_panic_write : NULL;
> +	bo_dev.flags = flags;
>  	bo_dev.read = blkoops_generic_blk_read;
>  	bo_dev.write = blkoops_generic_blk_write;
>  
> diff --git a/include/linux/blkoops.h b/include/linux/blkoops.h
> index 71c596fd4cc8..bc7665d14a98 100644
> --- a/include/linux/blkoops.h
> +++ b/include/linux/blkoops.h
> @@ -6,6 +6,7 @@
>  #include <linux/types.h>
>  #include <linux/blkdev.h>
>  #include <linux/pstore_blk.h>
> +#include <linux/bitops.h>
>  
>  /**
>   * struct blkoops_device - backend blkoops driver structure.
> @@ -14,6 +15,10 @@
>   * blkoops_register_device(). If block device, you are strongly recommended
>   * to use blkoops_register_blkdev().
>   *
> + * @flags:
> + *	Refer to macro starting with BLKOOPS_DEV_SUPPORT_. These macros tell
> + *	us that which pstore/blk recorders this device supports. Zero means
> + *	all recorders for compatible.
>   * @total_size:
>   *	The total size in bytes pstore/blk can use. It must be greater than
>   *	4096 and be multiple of 4096.
> @@ -38,6 +43,13 @@
>   *	On error, negative number should be returned.
>   */
>  struct blkoops_device {
> +	unsigned int flags;
> +#define BLKOOPS_DEV_SUPPORT_ALL		UINT_MAX
> +#define BLKOOPS_DEV_SUPPORT_DEFAULT	(0)
> +#define BLKOOPS_DEV_SUPPORT_DMESG	BIT(0)
> +#define BLKOOPS_DEV_SUPPORT_PMSG	BIT(1)
> +#define BLKOOPS_DEV_SUPPORT_CONSOLE	BIT(2)
> +#define BLKOOPS_DEV_SUPPORT_FTRACE	BIT(3)
>  	unsigned long total_size;
>  	blkz_read_op read;
>  	blkz_write_op write;
> @@ -54,7 +66,7 @@ typedef int (*blkoops_blk_panic_write_op)(const char *buf, sector_t start_sect,
>  
>  int  blkoops_register_device(struct blkoops_device *bo_dev);
>  void blkoops_unregister_device(struct blkoops_device *bo_dev);
> -int  blkoops_register_blkdev(unsigned int major,
> +int  blkoops_register_blkdev(unsigned int major, unsigned int flags,
>  		blkoops_blk_panic_write_op panic_write);
>  void blkoops_unregister_blkdev(unsigned int major);
>  int  blkoops_blkdev_info(dev_t *devt, sector_t *nr_sects, sector_t *start_sect);
> -- 
> 1.9.1
> 

-- 
Kees Cook

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-07 12:25 [PATCH v2 00/11] pstore: mtd: support crash log to block and mtd device WeiXiong Liao
2020-02-07 12:25 ` [PATCH v2 01/11] pstore/blk: new support logger for block devices WeiXiong Liao
2020-02-26  0:52   ` Kees Cook
2020-02-27  8:21     ` liaoweixiong
2020-03-18 17:23       ` Kees Cook
2020-03-20  1:50         ` WeiXiong Liao
2020-03-20 18:20           ` Kees Cook
2020-03-22 10:28             ` WeiXiong Liao
2020-03-09  0:52     ` WeiXiong Liao
2020-02-07 12:25 ` [PATCH v2 02/11] blkoops: add blkoops, a warpper for pstore/blk WeiXiong Liao
2020-03-18 18:06   ` Kees Cook
2020-03-22 10:00     ` WeiXiong Liao
2020-03-22 15:44       ` Kees Cook
2020-02-07 12:25 ` [PATCH v2 03/11] pstore/blk: blkoops: support pmsg recorder WeiXiong Liao
2020-03-18 18:13   ` Kees Cook
2020-03-22 11:14     ` WeiXiong Liao
2020-03-22 15:59       ` Kees Cook
2020-02-07 12:25 ` [PATCH v2 04/11] pstore/blk: blkoops: support console recorder WeiXiong Liao
2020-03-18 18:16   ` Kees Cook
2020-03-22 11:35     ` WeiXiong Liao
2020-02-07 12:25 ` [PATCH v2 05/11] pstore/blk: blkoops: support ftrace recorder WeiXiong Liao
2020-03-18 18:19   ` Kees Cook
2020-03-22 11:42     ` WeiXiong Liao
2020-03-22 15:16       ` Kees Cook
2020-02-07 12:25 ` [PATCH v2 06/11] Documentation: pstore/blk: blkoops: create document for pstore_blk WeiXiong Liao
2020-03-18 18:31   ` Kees Cook
2020-03-22 12:20     ` WeiXiong Liao
2020-02-07 12:25 ` [PATCH v2 07/11] pstore/blk: skip broken zone for mtd device WeiXiong Liao
2020-03-18 18:35   ` Kees Cook
2020-03-22 12:27     ` WeiXiong Liao
2020-02-07 12:25 ` [PATCH v2 08/11] blkoops: respect for device to pick recorders WeiXiong Liao
2020-03-18 18:42   ` Kees Cook [this message]
2020-03-22 13:06     ` WeiXiong Liao
2020-02-07 12:25 ` [PATCH v2 09/11] pstore/blk: blkoops: support special removing jobs for dmesg WeiXiong Liao
2020-03-18 18:47   ` Kees Cook
2020-03-22 13:03     ` WeiXiong Liao
2020-02-07 12:25 ` [PATCH v2 10/11] blkoops: add interface for dirver to get information of blkoops WeiXiong Liao
2020-02-07 12:25 ` [PATCH v2 11/11] mtd: new support oops logger based on pstore/blk WeiXiong Liao
2020-02-18 10:34   ` Miquel Raynal
2020-02-19  1:13     ` liaoweixiong
2020-03-18 18:57   ` Kees Cook
2020-03-22 13:51     ` WeiXiong Liao
2020-03-22 15:13       ` Kees Cook

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=202003181136.5F115BFCC1@keescook \
    --to=keescook@chromium.org \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=anton@enomsg.org \
    --cc=ccross@android.com \
    --cc=corbet@lwn.net \
    --cc=davem@davemloft.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=liaoweixiong@allwinnertech.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=mchehab+samsung@kernel.org \
    --cc=miquel.raynal@bootlin.com \
    --cc=richard@nod.at \
    --cc=robh@kernel.org \
    --cc=tony.luck@intel.com \
    --cc=vigneshr@ti.com \
    /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

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git