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 07/11] pstore/blk: skip broken zone for mtd device
Date: Wed, 18 Mar 2020 11:35:43 -0700
Message-ID: <202003181131.3A8F861F@keescook> (raw)
In-Reply-To: <1581078355-19647-8-git-send-email-liaoweixiong@allwinnertech.com>

On Fri, Feb 07, 2020 at 08:25:51PM +0800, WeiXiong Liao wrote:
> It's one of a series of patches for adaptive to MTD device.
> 
> MTD device is not block device. As the block of flash (MTD device) will
> be broken, it's necessary for pstore/blk to skip the broken block
> (bad block).
> 
> If device drivers return -ENEXT, pstore/blk will try next zone of dmesg.
> 
> Signed-off-by: WeiXiong Liao <liaoweixiong@allwinnertech.com>
> ---
>  Documentation/admin-guide/pstore-block.rst |  3 +-
>  fs/pstore/blkzone.c                        | 74 +++++++++++++++++++++++-------
>  include/linux/blkoops.h                    |  4 +-
>  include/linux/pstore_blk.h                 |  4 ++
>  4 files changed, 66 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/admin-guide/pstore-block.rst b/Documentation/admin-guide/pstore-block.rst
> index c8a5f68960c3..be865dfc1a28 100644
> --- a/Documentation/admin-guide/pstore-block.rst
> +++ b/Documentation/admin-guide/pstore-block.rst
> @@ -188,7 +188,8 @@ The parameter @offset of these interface is the relative position of the device.
>  Normally the number of bytes read/written should be returned, while for error,
>  negative number will be returned. The following return numbers mean more:
>  
> --EBUSY: pstore/blk should try again later.
> +1. -EBUSY: pstore/blk should try again later.
> +#. -ENEXT: this zone is used or broken, pstore/blk should try next one.
>  
>  panic_write (for non-block device)
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> diff --git a/fs/pstore/blkzone.c b/fs/pstore/blkzone.c
> index 442e5a5bbfda..205aeff28992 100644
> --- a/fs/pstore/blkzone.c
> +++ b/fs/pstore/blkzone.c
> @@ -207,6 +207,9 @@ static int blkz_zone_write(struct blkz_zone *zone,
>  
>  	return 0;
>  set_dirty:
> +	/* no need to mark dirty if going to try next zone */
> +	if (wcnt == -ENEXT)
> +		return -ENEXT;
>  	atomic_set(&zone->dirty, true);
>  	/* flush dirty zones nicely */
>  	if (wcnt == -EBUSY && !is_on_panic())
> @@ -360,7 +363,11 @@ static int blkz_recover_dmesg_meta(struct blkz_context *cxt)
>  			return -EINVAL;
>  
>  		rcnt = info->read((char *)buf, len, zone->off);
> -		if (rcnt != len) {
> +		if (rcnt == -ENEXT) {
> +			pr_debug("%s with id %lu may be broken, skip\n",
> +					zone->name, i);
> +			continue;
> +		} else if (rcnt != len) {
>  			pr_err("read %s with id %lu failed\n", zone->name, i);
>  			return (int)rcnt < 0 ? (int)rcnt : -EIO;
>  		}
> @@ -650,24 +657,58 @@ static void blkz_write_kmsg_hdr(struct blkz_zone *zone,
>  		hdr->counter = 0;
>  }
>  
> +/*
> + * In case zone is broken, which may occur to MTD device, we try each zones,
> + * start at cxt->dmesg_write_cnt.
> + */
>  static inline int notrace blkz_dmesg_write_do(struct blkz_context *cxt,
>  		struct pstore_record *record)
>  {
> +	int ret = -EBUSY;
>  	size_t size, hlen;
>  	struct blkz_zone *zone;
> -	unsigned int zonenum;
> +	unsigned int i;
>  
> -	zonenum = cxt->dmesg_write_cnt;
> -	zone = cxt->dbzs[zonenum];
> -	if (unlikely(!zone))
> -		return -ENOSPC;
> -	cxt->dmesg_write_cnt = (zonenum + 1) % cxt->dmesg_max_cnt;
> +	for (i = 0; i < cxt->dmesg_max_cnt; i++) {
> +		unsigned int zonenum, len;
> +
> +		zonenum = (cxt->dmesg_write_cnt + i) % cxt->dmesg_max_cnt;
> +		zone = cxt->dbzs[zonenum];
> +		if (unlikely(!zone))
> +			return -ENOSPC;
>  
> -	pr_debug("write %s to zone id %d\n", zone->name, zonenum);
> -	blkz_write_kmsg_hdr(zone, record);
> -	hlen = sizeof(struct blkz_dmesg_header);
> -	size = min_t(size_t, record->size, zone->buffer_size - hlen);
> -	return blkz_zone_write(zone, FLUSH_ALL, record->buf, size, hlen);
> +		/* avoid destorying old data, allocate a new one */
> +		len = zone->buffer_size + sizeof(*zone->buffer);
> +		zone->oldbuf = zone->buffer;
> +		zone->buffer = kzalloc(len, GFP_KERNEL);
> +		if (!zone->buffer) {
> +			zone->buffer = zone->oldbuf;
> +			return -ENOMEM;
> +		}
> +		zone->buffer->sig = zone->oldbuf->sig;
> +
> +		pr_debug("write %s to zone id %d\n", zone->name, zonenum);
> +		blkz_write_kmsg_hdr(zone, record);
> +		hlen = sizeof(struct blkz_dmesg_header);
> +		size = min_t(size_t, record->size, zone->buffer_size - hlen);
> +		ret = blkz_zone_write(zone, FLUSH_ALL, record->buf, size, hlen);
> +		if (likely(!ret || ret != -ENEXT)) {
> +			cxt->dmesg_write_cnt = zonenum + 1;
> +			cxt->dmesg_write_cnt %= cxt->dmesg_max_cnt;
> +			/* no need to try next zone, free last zone buffer */
> +			kfree(zone->oldbuf);
> +			zone->oldbuf = NULL;
> +			return ret;
> +		}
> +
> +		pr_debug("zone %u may be broken, try next dmesg zone\n",
> +				zonenum);
> +		kfree(zone->buffer);
> +		zone->buffer = zone->oldbuf;
> +		zone->oldbuf = NULL;
> +	}
> +
> +	return -EBUSY;
>  }
>  
>  static int notrace blkz_dmesg_write(struct blkz_context *cxt,
> @@ -791,7 +832,6 @@ static int notrace blkz_pstore_write(struct pstore_record *record)
>  	}
>  }
>  
> -#define READ_NEXT_ZONE ((ssize_t)(-1024))
>  static struct blkz_zone *blkz_read_next_zone(struct blkz_context *cxt)
>  {
>  	struct blkz_zone *zone = NULL;
> @@ -852,7 +892,7 @@ static ssize_t blkz_dmesg_read(struct blkz_zone *zone,
>  	if (blkz_read_dmesg_hdr(zone, record)) {
>  		atomic_set(&zone->buffer->datalen, 0);
>  		atomic_set(&zone->dirty, 0);
> -		return READ_NEXT_ZONE;
> +		return -ENEXT;
>  	}
>  	size -= sizeof(struct blkz_dmesg_header);
>  
> @@ -877,7 +917,7 @@ static ssize_t blkz_dmesg_read(struct blkz_zone *zone,
>  	if (unlikely(blkz_zone_read(zone, record->buf + hlen, size,
>  				sizeof(struct blkz_dmesg_header)) < 0)) {
>  		kfree(record->buf);
> -		return READ_NEXT_ZONE;
> +		return -ENEXT;
>  	}
>  
>  	return size + hlen;
> @@ -891,7 +931,7 @@ static ssize_t blkz_record_read(struct blkz_zone *zone,
>  
>  	buf = (struct blkz_buffer *)zone->oldbuf;
>  	if (!buf)
> -		return READ_NEXT_ZONE;
> +		return -ENEXT;
>  
>  	size = atomic_read(&buf->datalen);
>  	start = atomic_read(&buf->start);
> @@ -943,7 +983,7 @@ static ssize_t blkz_pstore_read(struct pstore_record *record)
>  	}
>  
>  	ret = readop(zone, record);
> -	if (ret == READ_NEXT_ZONE)
> +	if (ret == -ENEXT)
>  		goto next_zone;
>  	return ret;
>  }
> diff --git a/include/linux/blkoops.h b/include/linux/blkoops.h
> index 8f40f225545d..71c596fd4cc8 100644
> --- a/include/linux/blkoops.h
> +++ b/include/linux/blkoops.h
> @@ -27,6 +27,7 @@
>   *	On error, negative number should be returned. The following returning
>   *	number means more:
>   *	  -EBUSY: pstore/blk should try again later.
> + *	  -ENEXT: this zone is used or broken, pstore/blk should try next one.
>   * @panic_write:
>   *	The write operation only used for panic.
>   *
> @@ -45,7 +46,8 @@ struct blkoops_device {
>  
>  /*
>   * Panic write for block device who should write alignmemt to SECTOR_SIZE.
> - * On success, zero should be returned. Others mean error.
> + * On success, zero should be returned. Others mean error except that -ENEXT
> + * means the zone is used or broken, pstore/blk should try next one.
>   */
>  typedef int (*blkoops_blk_panic_write_op)(const char *buf, sector_t start_sect,
>  		sector_t sects);
> diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
> index 77704c1b404a..bbbe4fe37f7c 100644
> --- a/include/linux/pstore_blk.h
> +++ b/include/linux/pstore_blk.h
> @@ -6,6 +6,9 @@
>  #include <linux/types.h>
>  #include <linux/blkdev.h>
>  
> +/* read/write function return -ENEXT means try next zone */
> +#define ENEXT ((ssize_t)(1024))

I really don't like inventing errno numbers. Can you just reuse an
existing (but non-block) errno like ESRCH or ENOMSG or something?

> +
>  /**
>   * struct blkz_info - backend blkzone driver structure
>   *
> @@ -42,6 +45,7 @@
>   *	On error, negative number should be returned. The following returning
>   *	number means more:
>   *	  -EBUSY: pstore/blk should try again later.
> + *	  -ENEXT: this zone is used or broken, pstore/blk should try next one.
>   * @panic_write:
>   *	The write operation only used for panic. It's optional if you do not
>   *	care panic record. If panic occur but blkzone do not recover yet, the
> -- 
> 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 " 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 [this message]
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
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=202003181131.3A8F861F@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